Repository navigation
fix: harden database URL handling, TLS verification and password rotation - #128
Open
thereisnotime wants to merge 9 commits into
Open
thereisnotime wants to merge 9 commits into
thereisnotime wants to merge 9 commits into
Conversation
spec.databaseURL was interpolated straight into the sh -c init script,
so a value containing quotes, $(...) or backticks ran arbitrary commands
in the init container. Pass it as DATABASE_URL and reference
"${DATABASE_URL}" in the script, the same way ADMIN_PASSWORD is handled.
The external database URL (usually with a password in it) was written in plain text into the generated warpgate.yaml ConfigMap. Warpgate has no env override for database_url, so the ConfigMap now leaves it out and the init container appends it to /data/warpgate.yaml from DATABASE_URL, quoted as a YAML single-quoted scalar. Add spec.databaseURLSecretRef so the URL can come from a Secret. It takes precedence over spec.databaseURL, which keeps working but is now deprecated and gets an admission warning. externalHost is emitted through a YAML marshaller instead of printf, and the init script is folded into the pod template hash so existing Deployments roll onto the new script.
TLSConfigSpec.verify was a plain bool with omitempty, so a tls block that only set a mode (e.g. mode: Required) was sent to Warpgate with verification off, and certificate checks were silently skipped. verify is now a *bool with a CRD default of true, the webhook defaults it as well, and toWarpgateTLS treats nil as true. Verification is only turned off when the spec sets verify to false explicitly. BREAKING CHANGE: WarpgateTarget tls blocks without a verify field now verify the target certificate. Targets with self-signed or otherwise untrusted certificates that relied on the old implicit false need an explicit verify: false.
The password credential controller only read its Secret on reconcile and never watched it, and once a credential existed it was never touched again. Rotating the Secret left the old password valid in Warpgate. Watch Secrets and enqueue the credentials in the same namespace that reference them. Track the applied Secret revision in status.appliedSecretVersion; when it changes (or the user changed), delete the old Warpgate credential and create a new one with the current password. Warpgate has no in-place password update, so the credential ID changes on rotation.
The WarpgateConnection created for a WarpgateInstance always had insecureSkipVerify set, so the operator sent the admin password to whatever answered on the Service address, even when the instance served a user-provided certificate. WarpgateConnection gets an optional caSecretRef (PEM bundle, key defaults to ca.crt) that is used as the client's RootCAs. When an instance sets tls.secretName, its connection now verifies against that Secret's ca.crt, or tls.crt if there is no ca.crt. Verification is still skipped for the self-signed certificate the pod generates itself, since the operator never sees it. The new tls.verifyConnection field makes that explicit: true fails reconciliation instead of creating an unverified connection, false always skips verification. BREAKING CHANGE: instances with tls.secretName now get a verifying connection. The certificate must be valid for <name>-http.<namespace>.svc; set tls.verifyConnection to false to keep the old behaviour if it isn't.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…ation The pod template hash includes spec.databaseURL, which usually embeds a password, and the hash is visible to anyone who can read pods. Derive the database URL's contribution with PBKDF2 so the annotation still changes on a new URL but can't be used to cheaply test password guesses.
The pod rollout key only detects changes to the config and the generated init script, which references secrets by env var name only. FNV-1a makes that intent explicit instead of reading as password hashing.
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
3 participants
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.
Description
Fixes five security issues found in an audit of the controllers.
WarpgateInstance:
spec.databaseURLwas interpolated into the init container'ssh -cscript. It now goes in asDATABASE_URLand the script only references"${DATABASE_URL}".WarpgateInstance: the database URL (usually with a password) was written in plain text to the generated
warpgate.yamlConfigMap. The ConfigMap now omits it and the init container appends it to/data/warpgate.yaml, quoted. Addedspec.databaseURLSecretRef, which takes precedence overspec.databaseURL.databaseURLstill works but is deprecated and gets an admission warning.externalHostis now emitted through a YAML marshaller.WarpgateTarget: a
tlsblock that only setmodewas sent withverify: false.verifyis now a*booldefaulting to true (CRD default, webhook and converter).WarpgatePasswordCredential: rotating the password Secret never reached Warpgate. The controller now watches Secrets, tracks
status.appliedSecretVersion, and replaces the credential (delete old, create new) when the Secret changes.WarpgateInstance: the auto-created WarpgateConnection always skipped TLS verification. WarpgateConnection gets an optional
caSecretRef. Instances withtls.secretNamenow get a connection that verifies againstca.crt(ortls.crt). The pod's own self-signed cert still skips verification, made explicit with the newtls.verifyConnectionfield.WarpgateInstance: the pod rollout hash included the raw
spec.databaseURLand is visible in the pod template annotation to anyone who can read pods. The URL's contribution is now derived with PBKDF2, and the rollout key itself uses FNV-1a, since it only detects changes.Behaviour changes to note:
tlsblocks withoutverifynow verify. Setverify: falseexplicitly for targets with untrusted certs.tls.secretNameget a verifying connection, so the cert must cover<name>-http.<namespace>.svc.tls.verifyConnection: falserestores the old behaviour.Go Vulnerability CheckandTrivy Vulnerability Scanfail on main as well (go.opentelemetry.io/otel v1.44.0, GO-2026-6505, fixed in 1.45.0); that bump is left to the dependency update rather than this PR.Two related issues are left for follow-up:
tls.certManageralone never mounts the cert-manager Secret into the pod, and the password credential controller can create a duplicate credential when the finalizer-triggered reconcile reads stale cache.Type of Change
Checklist
just test)just lint)just manifests)