Conversation
GetDbURI hardcoded "?sslmode=disable", so there was no way to run the service against a TLS-only Postgres. Add four config keys -- database.ssl_mode plus optional ssl_root_cert / ssl_cert / ssl_key -- with ssl_mode defaulting to "disable" so existing deployments are unchanged. The cert paths are appended to the DSN only when set, so libpq/pgx can locate the CA and client material for require and stricter modes. Document the four CSERVICE_DATABASE_SSL_* environment variables in the README and config.yml.example, and state plainly that "require" encrypts without verifying the server certificate -- it does not stop an active man-in-the-middle; verify-full is the safe choice when the server's chain is available. TestGetDbURI, refactored to a struct table, covers the default (byte-for-byte unchanged), verify-full with all cert paths URL-encoded, and require with only a root cert set.
GetDbURI built the connection string with fmt.Sprintf and interpolated
the username and password raw ("postgres://%s:%s@..."). pgx parses
postgres:// DSNs with url.Parse, which splits the authority on the first
"/", so an unescaped "/" in the password truncates the host and the
service fails to start with a misleading "invalid port" error.
This is not hypothetical: the README tells operators to generate the
password with "openssl rand -base64 32", and the base64 alphabet includes
"/", so roughly half of the passwords produced by the documented recipe
break startup. "@", ":", "?" and "#" fail the same way.
Build the URL with url.URL and url.UserPassword, which escapes userinfo
per RFC 3986 (and net.JoinHostPort, which also brackets IPv6 hosts). The
default DSN is unchanged byte-for-byte -- url.URL.String() escapes nothing
in the default credentials -- so the existing regression case still holds.
Add table cases for base64 and URI-delimiter passwords, each asserting the
DSN survives a url.Parse round-trip with the password intact.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two related changes to
GetDbURI, as separate commits:feat: make Postgres sslmode and TLS material configurable
GetDbURIhardcoded?sslmode=disable, so there was no way to run against a TLS-only Postgres. Addsdatabase.ssl_modeplus optionalssl_root_cert/ssl_cert/ssl_key(ssl_modedefaults todisable, so existing deployments are unchanged); cert paths are appended only when set. Documents theCSERVICE_DATABASE_SSL_*env vars andrequire's MITM caveat in the README andconfig.yml.example.fix: escape userinfo when building the database DSN
GetDbURIinterpolated the username/password raw, so an unescaped/in a base64 password (per the README'sopenssl rand -base64recipe) truncated the host underurl.Parseand broke startup for ~half of generated passwords. Rebuilds the DSN withurl.URL/url.UserPassword/net.JoinHostPort(RFC-3986 userinfo escaping, plus IPv6 bracketing). The default DSN is byte-for-byte unchanged.Testing
TestGetDbURIcovers default (byte-for-byte), verify-full with certs, require + root cert, and base64 / URI-delimiter passwords, each asserting aurl.Parseround-trip.go build,go test,go vet, andmake lintall pass; both commits build independently.