fix(gateway): gate unsafe auth deployment modes - #1871
Conversation
BlockedGator is blocked because GitHub reports this PR has merge conflicts with Head SHA: Next action: @alangou needs to rebase or merge |
|
@alangou bumping this for a rebase so we can re-check |
bf0663e to
580e58b
Compare
PR Review StatusValidation: This PR is project-valid for OpenShell because it tightens gateway authentication posture for shared deployments, updates the Helm and server configuration surfaces, adds matching tests, and updates the affected documentation. Review findings:
Docs: Updated in the affected Kubernetes, reference, security, Helm, and architecture docs; no new Fern navigation entry appears necessary. Next state: |
580e58b to
9656e9e
Compare
PR Review StatusValidation: This PR is project-valid for OpenShell because it tightens gateway authentication posture for shared deployments, updates the Helm and server configuration surfaces, adds matching tests, and updates the affected documentation. Review findings:
Docs: Updated in the affected Kubernetes, reference, security, Helm, and architecture docs; no new Fern navigation entry appears necessary. Next state: |
df74473 to
5419723
Compare
34b67c2 to
541b617
Compare
elezar
left a comment
There was a problem hiding this comment.
Overall, the security behavior looks correct and I did not find another authentication bypass. I have two requested changes before merge:
- Authentication-only mode still enforces configured scopes, so the repeated statement that "any valid issuer token can call user and admin APIs" is inaccurate. Please clarify that it skips RBAC role checks while scope checks continue to apply.
- This is an intentional upgrade break: existing OIDC deployments with empty roles will fail Helm rendering or gateway startup. Please add a short migration note telling operators to configure both roles or explicitly enable authentication-only mode before upgrading.
Additional non-blocking hardening opportunities:
- Warn at startup when
allow_oidc_auth_onlyis active. - Reject dormant opt-ins such as
allow_oidc_auth_only=truewhen OIDC is absent or RBAC roles are configured; otherwise a later accidental removal of the roles silently activates the weaker mode. - Consider a focused server-boundary test. The current configuration, router, and Helm tests cover the pieces well, but do not exercise their complete wiring.
The second auth-policy boolean also suggests that a small preparatory refactor ("prefactor") could make this change easier to audit. A behavior-preserving commit could introduce internal typed states while retaining the existing serialized configuration:
enum MissingUserPolicy {
PassThrough,
Reject,
DevelopmentUser,
}
enum OidcAuthorization {
Rbac { admin_role: String, user_role: String },
AuthenticationOnly,
}The security commit would then only change PassThrough to Reject and require explicit permission for AuthenticationOnly. A subsequent refactor could replace the external booleans with typed policy fields and resolve them—together with the configured authenticators and effective listener addresses—into a single GatewayAuthPlan consumed by the router.
I would not block this security fix on the broader GatewayAuthPlan redesign, but I recommend doing it before adding another authentication-policy flag.
9c0723e to
4f0fb3e
Compare
|
Thanks @elezar Addressed the two requested changes: Clarified that auth-only skips role checks but still enforces configured scopes. |
4f0fb3e to
92ec043
Compare
elezar
left a comment
There was a problem hiding this comment.
Looks good to me. The fail-closed auth posture checks and Helm/TOML validation cover the cases I was worried about, and the focused core/Helm tests passed.
Non-blocking follow-up: it would be worth extracting the shared OIDC role/auth-only posture logic into one helper/type so Config::validate_gateway_auth_posture, AuthzPolicy::validate, and the Helm/docs vocabulary stay aligned as these auth modes evolve.
elezar
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR is project-valid for OpenShell because it tightens gateway authentication posture for shared deployments, updates the Helm and server configuration surfaces, adds matching tests, and updates the affected documentation.
Head SHA: 92ec04308e557439cb187355ed6f06296d08b971
Review findings:
- One blocking Kubernetes posture-detection gap remains. The inline finding is anchored on the current diff.
Thanks @elezar, I checked your approval note that the fail-closed auth posture checks and Helm/TOML validation cover the cases you were worried about. Those earlier gateway_jwt-only and driver-added listener concerns appear addressed, but the independent review found a remaining raw-driver-name/autodetect bypass in the Kubernetes detector.
Docs: Updated in the affected Kubernetes, reference, security, Helm, and architecture docs; no new Fern navigation entry appears necessary.
Next state: gator:in-review
|
|
||
| #[must_use] | ||
| fn is_kubernetes_gateway_deployment(&self) -> bool { | ||
| self.compute_drivers |
There was a problem hiding this comment.
gator-agent
Critical: is_kubernetes_gateway_deployment() compares raw compute_drivers strings, but runtime startup normalizes driver names and can auto-detect Kubernetes when the list is empty. A config file value like "Kubernetes", or an in-cluster startup that relies on autodetect, can bypass the new Kubernetes mTLS rejection and allow the unsupported transport-certificate-as-user-auth posture this PR is meant to block. This is CWE-269. Please normalize/parse configured names here and handle the empty-list autodetect case, or pass the resolved ConfiguredComputeDriver into posture validation before accepting mTLS. Add regression coverage for empty/autodetected Kubernetes and mixed-case TOML compute_drivers = ["Kubernetes"] with mtls_auth.enabled = true.
There was a problem hiding this comment.
This gator finding seems to indicate that we may need to rething how we do validation. Even if compute_drivers is EMPTY, it may RESOLVE to kubernetes which would conflict with the configured auth deployment mode.
How can we better split auth-config only validation from deployment dependent validation?
92ec043 to
722d332
Compare
Require explicit opt-in for OIDC authentication-only mode on shared gateway deployments and fail closed when gRPC user requests have no auth path. Align Helm validation, tests, and docs so weak auth modes are intentional and visible. Signed-off-by: Adrien Langou <alangou@nvidia.com>
722d332 to
7e4de08
Compare
|
I have fixed the issue ! |
Summary
Gate unsafe gateway authentication postures for shared deployments. OIDC authentication-only mode now requires an explicit opt-in, and gRPC user requests without an auth path fail closed instead of passing through.
Related Issue
Closes OS-186
Changes
allow_oidc_auth_onlyconfig and Helm value for OIDC authentication-only mode.UNAUTHENTICATED.Testing
mise run pre-commitpassesAdditional validation run:
CARGO_TARGET_DIR="/home/op/OpenShell/target" mise run testCARGO_TARGET_DIR="/home/op/OpenShell/target" mise exec -- cargo test -p openshell-servermise run helm:testmise run helm:lintmise run helm:docs:checkgit diff --checkNote:
mise run pre-commitcould not be confirmed because the shell executor stopped returning statuses.Checklist