Enable TLS support for PostgreSQL (#3831)#484
Conversation
This PR adds TLS support for PostgreSQL connections in the Trillian server/signer. The key changes include: Added new flags: postgresql_tls_ca: Path to the CA certificate file for PostgreSQL TLS connection. postgresql_verify_full: Enable full TLS verification for PostgreSQL (sslmode=verify-full). If false, only sslmode=verify-ca is used. If no TLS configuration is provided, the connection defaults to non-TLS, ensuring backward compatibility. Tracking issue: google#3830 --------- Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>
Reviewer's GuideImplements configurable TLS support for PostgreSQL connections by adding new flags, a URI-rewriting helper that injects TLS parameters, wiring it into the PostgreSQL provider initialization, and covering the behavior with unit tests and a CHANGELOG entry. Sequence diagram for PostgreSQL connection initialization with TLS URI rewritingsequenceDiagram
participant TrillianServer
participant postgresqlProvider
participant getPostgreSQLDatabaseLocked
participant BuildPostgresTLSURI
participant os
participant url
participant OpenDB
participant PostgreSQLServer
TrillianServer->>postgresqlProvider: New()
postgresqlProvider->>getPostgreSQLDatabaseLocked: getPostgreSQLDatabaseLocked()
getPostgreSQLDatabaseLocked->>getPostgreSQLDatabaseLocked: check cached postgresqlDB, postgresqlErr
getPostgreSQLDatabaseLocked->>BuildPostgresTLSURI: BuildPostgresTLSURI(postgresql_uri)
alt TLS CA not configured
BuildPostgresTLSURI-->>getPostgreSQLDatabaseLocked: return original uri
else TLS CA configured
BuildPostgresTLSURI->>os: Stat(postgresql_tls_ca)
alt CA file error
os-->>BuildPostgresTLSURI: error
BuildPostgresTLSURI-->>getPostgreSQLDatabaseLocked: error
getPostgreSQLDatabaseLocked-->>postgresqlProvider: postgresqlErr
postgresqlProvider-->>TrillianServer: error
else CA file ok
os-->>BuildPostgresTLSURI: ok
BuildPostgresTLSURI->>url: Parse(uri)
alt invalid uri
url-->>BuildPostgresTLSURI: error
BuildPostgresTLSURI-->>getPostgreSQLDatabaseLocked: error
getPostgreSQLDatabaseLocked-->>postgresqlProvider: postgresqlErr
postgresqlProvider-->>TrillianServer: error
else valid uri
url-->>BuildPostgresTLSURI: parsed url
BuildPostgresTLSURI->>BuildPostgresTLSURI: set sslrootcert, sslmode
BuildPostgresTLSURI-->>getPostgreSQLDatabaseLocked: modified uri
end
end
end
getPostgreSQLDatabaseLocked->>OpenDB: OpenDB(uri)
OpenDB->>PostgreSQLServer: establish connection (TLS or non TLS)
PostgreSQLServer-->>OpenDB: connection established
OpenDB-->>getPostgreSQLDatabaseLocked: *pgxpool.Pool
getPostgreSQLDatabaseLocked-->>postgresqlProvider: postgresqlDB
postgresqlProvider-->>TrillianServer: ready
Class diagram for PostgreSQL provider and TLS URI helperclassDiagram
class postgresqlProvider {
-db *pgxpool.Pool
+Close() error
}
class Flags {
<<global>>
+postgreSQLURI *string
+postgresqlTLSCA *string
+postgresqlVerifyFull *bool
}
class TLSURIHelper {
+BuildPostgresTLSURI(uri string) (string, error)
}
class PostgreSQLStorageState {
<<package_level>>
+postgresqlMu sync.Mutex
+postgresqlErr error
+postgresqlDB *pgxpool.Pool
}
postgresqlProvider --> PostgreSQLStorageState : uses
postgresqlProvider --> Flags : reads
postgresqlProvider --> TLSURIHelper : calls
TLSURIHelper --> Flags : reads
TLSURIHelper --> PostgreSQLStorageState : sets postgresqlErr
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider making
BuildPostgresTLSURIunexported (e.g.buildPostgresTLSURI) since it’s only used within this package, which keeps the external API surface smaller while still allowing it to be tested. - In
BuildPostgresTLSURI, you’re mutating the package-levelpostgresqlErrinside a helper; it would be cleaner and less surprising to just returnfmt.Errorf(...)from this function and only setpostgresqlErringetPostgreSQLDatabaseLocked.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making `BuildPostgresTLSURI` unexported (e.g. `buildPostgresTLSURI`) since it’s only used within this package, which keeps the external API surface smaller while still allowing it to be tested.
- In `BuildPostgresTLSURI`, you’re mutating the package-level `postgresqlErr` inside a helper; it would be cleaner and less surprising to just return `fmt.Errorf(...)` from this function and only set `postgresqlErr` in `getPostgreSQLDatabaseLocked`.
## Individual Comments
### Comment 1
<location> `storage/postgresql/provider.go:115-124` </location>
<code_context>
+
+// BuildPostgresTLSURI modifies the given PostgreSQL URI to include TLS parameters based on flags.
+// It returns the modified URI and any error encountered.
+func BuildPostgresTLSURI(uri string) (string, error) {
+ if *postgresqlTLSCA == "" {
+ return uri, nil
+ }
+ if _, err := os.Stat(*postgresqlTLSCA); err != nil {
+ postgresqlErr = fmt.Errorf("postgresql CA file error: %w", err)
+ return "", postgresqlErr
+ }
+ u, err := url.Parse(uri)
+ if err != nil {
+ postgresqlErr = fmt.Errorf("invalid postgresql URI %q: %w", uri, err)
+ return "", postgresqlErr
+ }
+ q := u.Query()
+ q.Set("sslrootcert", *postgresqlTLSCA)
+ if *postgresqlVerifyFull {
+ q.Set("sslmode", "verify-full")
+ } else {
+ if q.Get("sslmode") == "" {
+ q.Set("sslmode", "verify-ca")
+ }
+ }
+ u.RawQuery = q.Encode()
+ return u.String(), nil
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid mutating the global `postgresqlErr` inside this helper to reduce hidden side effects and possible races.
This helper both returns an error and mutates the package-level `postgresqlErr`. Because `BuildPostgresTLSURI` is exported and not clearly tied to the `getPostgreSQLDatabaseLocked` mutex, it could race on `postgresqlErr` or clobber an existing error when called from elsewhere. Prefer making this function side‑effect‑free (returning only `(string, error)`) and have the caller update `postgresqlErr` under the appropriate lock if needed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| func BuildPostgresTLSURI(uri string) (string, error) { | ||
| if *postgresqlTLSCA == "" { | ||
| return uri, nil | ||
| } | ||
| if _, err := os.Stat(*postgresqlTLSCA); err != nil { | ||
| postgresqlErr = fmt.Errorf("postgresql CA file error: %w", err) | ||
| return "", postgresqlErr | ||
| } | ||
| u, err := url.Parse(uri) | ||
| if err != nil { |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid mutating the global postgresqlErr inside this helper to reduce hidden side effects and possible races.
This helper both returns an error and mutates the package-level postgresqlErr. Because BuildPostgresTLSURI is exported and not clearly tied to the getPostgreSQLDatabaseLocked mutex, it could race on postgresqlErr or clobber an existing error when called from elsewhere. Prefer making this function side‑effect‑free (returning only (string, error)) and have the caller update postgresqlErr under the appropriate lock if needed.
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>
This PR adds TLS support for PostgreSQL connections in the Trillian server/signer.
The key changes include:
Added new flags:
postgresql_tls_ca: Path to the CA certificate file for PostgreSQL TLS connection.postgresql_verify_full: Enable full TLS verification for PostgreSQL (sslmode=verify-full). If false, only sslmode=verify-ca is used. If no TLS configuration is provided, the connection defaults to non-TLS, ensuring backward compatibility.Tracking issue: google#3830
Checklist