dex, oauth2-proxy: refuse to render placeholder credentials - #3569
dex, oauth2-proxy: refuse to render placeholder credentials#3569danish9039 wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
|
/retest |
There was a problem hiding this comment.
Pull request overview
Adds Helm rendering guards against placeholder or malformed credentials for Dex and OAuth2 Proxy.
Changes:
- Validates OIDC, bcrypt, and cookie secrets.
- Documents credential requirements.
- Adds automated coverage and valid test fixtures.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
common/dex/helm/templates/validate-credentials.yaml |
Validates Dex credentials. |
common/dex/helm/README.md |
Documents Dex requirements. |
common/oauth2-proxy/helm/templates/validate-credentials.yaml |
Validates OAuth2 Proxy credentials. |
common/oauth2-proxy/helm/README.md |
Documents cookie-secret requirements. |
tests/test_helm_credential_guards.py |
Tests credential guards. |
tests/test_dex_helm_rollout_checksums.py |
Supplies valid credential fixtures. |
.github/workflows/helm-kustomize-comparison.yml |
Runs the new tests. |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
e265180 to
459d736
Compare
|
/retest |
Copilot review follow-up: the placeholder-hash check fired even with config.enablePasswordDB=false, where the hash is unused, blocking the external-connector configuration. Also names the shared workflow step for both components and gives the credential fixtures explicit names. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Several core guard branches and the padded-secret path remain untested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
tests/helm_credential_guards_test.py:69
- This default render exits at the client-secret guard, leaving the cookie-placeholder guard untested. Render again with a valid client secret and assert the cookie-specific failure so the second guard cannot regress unnoticed.
def test_oauth2_proxy_defaults_are_rejected(self):
result = self.render_oauth2_proxy()
self.assertNotEqual(result.returncode, 0)
self.assertIn(
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
| def test_dex_defaults_are_rejected(self): | ||
| result = self.render_dex() | ||
|
|
||
| self.assertNotEqual(result.returncode, 0) | ||
| self.assertIn("oidcClient.secret is still the placeholder", result.stderr) |
| ("123456789012345678901234", "raw 24"), | ||
| ("12345678901234567890123456789012", "raw 32"), | ||
| ("AAECAwQFBgcICQoLDA0ODw", "unpadded URL-safe, decodes to 16"), | ||
| ("Zm9vYmFyZm9vYmFyZm9vYmFyZm9vYmFy", "padded, decodes to 24"), |
Pull Request Template for Kubeflow Manifests
✏️ Summary of Changes
Both charts ship
REPLACE_MEcredential defaults, and nothing stopped aninstallation from using them.
Today both succeed, and produce:
The cookie secret signs oauth2-proxy session cookies. A value anyone can read out
of this repository means anyone can forge a session.
The charts now fail before rendering while a placeholder is in place.
Two related checks
staticPassword.hashmust look like bcrypt. Dex compares the submittedpassword against this hash; any other value matches nothing, so the account
silently cannot log in. The chart checks the
$2a$/$2b$/$2y$prefix.credentials.cookieSecretmust decode to 16, 24 or 32 bytes. oauth2-proxyuses it as an AES key. Any other length is accepted by Kubernetes and then
crash-loops the workload, which is a much worse place to find out.
Every message names the value and how to generate a correct one, for example:
Test change
tests/test_dex_helm_rollout_checksums.pyrendered with the placeholder defaultsand, in one case, with
staticPassword.hash=changed-password-hash, a value nopassword could ever match. It now supplies real-shaped credentials. The guard
found this, which is the point of it.
📦 Dependencies
None. Applies to two merged charts and is independent of every open pull request.
#3568 also touches
common/dex/helm; the two do not overlap in any file exceptthe chart README, and whichever merges second may need a trivial rebase.
🐛 Related Issues
Found while reviewing the Dex configuration surface for #3568.
Validation
The Kustomize comparison is unaffected: the CI values files supply real
credentials, so every scenario renders exactly as before.
✅ Contributor Checklist