Skip to content

Validate MCPOIDCConfig inline issuer and JWKS URLs#5936

Open
lopster568 wants to merge 1 commit into
stacklok:mainfrom
lopster568:add-mcpoidcconfig-url-validation
Open

Validate MCPOIDCConfig inline issuer and JWKS URLs#5936
lopster568 wants to merge 1 commit into
stacklok:mainfrom
lopster568:add-mcpoidcconfig-url-validation

Conversation

@lopster568

Copy link
Copy Markdown

Summary

MCPOIDCConfig resources with a malformed or plain-HTTP issuer or jwksUrl are accepted today. The reconciler's only spec gate, MCPOIDCConfig.Validate(), checks type/config consistency but never inspects the URLs, so a bad issuer surfaces later as an opaque OIDC verification failure. The equivalent checks ran for inline OIDC configs on MCPRemoteProxy until #4820 removed that field.

  • Wire the existing validation.ValidateOIDCIssuerURL() and validation.ValidateJWKSURL() into the MCPOIDCConfig controller through a new validateOIDCConfigSpec() helper.
  • The checks run in the controller rather than in Validate() because pkg/validation imports the v1beta1 package, so the types package cannot call it without an import cycle. This mirrors MCPRemoteProxy's validateSpec.
  • spec.inline.insecureAllowHTTP maps to the issuer validator's allowInsecure, so HTTP issuers that opt in keep working.
  • Add insecureAllowHTTP: true to deploy/keycloak/mcpserver-with-auth.yaml, whose plain-HTTP issuer would otherwise fail the new check.

Fixes #4823

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

TestValidateOIDCConfigSpec covers issuer and JWKS accept/reject cases including the insecureAllowHTTP opt-in and the empty-JWKS discovery case. TestMCPOIDCConfigReconciler_URLValidationFailureSetsCondition asserts a bad URL sets Valid=False. Existing MCPOIDCConfig controller tests pass unmodified.

Two pkg/api tests (TestNewServer_ReadTimeoutConfigured, TestSecurityHeaders) fail on my machine for want of a container runtime, and task lint-fix reports one gosec finding in cmd/thv/app/upgrade.go. Both reproduce unchanged on main and neither touches this PR's files.

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

No CRD schema change; this is controller-level validation only.

Does this introduce a user-facing change?

Yes. Inline configs with a malformed issuer, an HTTP issuer without insecureAllowHTTP: true, or a non-HTTPS JWKS URL now get Valid=False at reconcile time instead of failing later during OIDC verification. Stored objects previously accepted with such values flip to invalid on their next reconcile.

Special notes for reviewers

Scope is limited to inline configs. kubernetesServiceAccount configs default their issuer to the in-cluster URL and have no insecureAllowHTTP escape hatch, so validating them here could break clusters using custom internal issuers. Happy to follow up separately.

deploy/keycloak/mcpserver-with-auth.yaml's jwksUrl is plain HTTP and ValidateJWKSURL requires HTTPS with no escape hatch, so that manifest still fails validation after this PR. Dropping the explicit jwksUrl would fall back to discovery, but its comment says the explicit URL exists to avoid discovery issues. Which would you prefer?

The MCPOIDCConfig reconciler only checked type/config consistency, so
inline configs with malformed or plain-HTTP issuer and JWKS URLs were
accepted and failed later in the OIDC verification path. Wire the
existing validation.ValidateOIDCIssuerURL and validation.ValidateJWKSURL
helpers into the controller's validation step. The checks run at the
controller layer because pkg/validation imports the v1beta1 package, so
the types package cannot call it without an import cycle.

Add insecureAllowHTTP: true to deploy/keycloak/mcpserver-with-auth.yaml,
whose plain-HTTP issuer would otherwise fail the new check.

Signed-off-by: Roshan <rosh.s568@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add URL validation to MCPOIDCConfig controller

1 participant