Skip to content

Client cert lookup provider compliant to RFC 9440 #36161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
vmuzikar merged 3 commits into from
Dec 19, 2025
Merged

Client cert lookup provider compliant to RFC 9440 #36161

vmuzikar merged 3 commits into from
Dec 19, 2025

Conversation

@seiferma
Copy link
Contributor

@seiferma seiferma commented Dec 29, 2024 •
edited by ahus1
Loading

Closes #20761

This pull request addresses the issue brought up in #20761. I decided to build a certificate lookup provider that is compliant to RFC 9440 instead of just building a caddy-compliant provider. This is the solution that has been suggested in the issue.

Design decisions / implementation details:

  • All configuration options are optional because the RFC already provides reasonable default values
  • The length option for certificate chains defines the amount of certificates that are processed and exceeding the limit is treated as an error
  • I created unit tests for the actual lookup but none for the factory because I could not find examples of tests for the other providers
  • Test data is the data provided in RFC 9440

Checklist from the contribution guidelines:

@seiferma seiferma marked this pull request as ready for review December 29, 2024 17:59
@seiferma seiferma requested review from a team as code owners December 29, 2024 17:59
@ahus1 ahus1 changed the title Client cert lookup provider compliant to RFC 9440 (#20761) Client cert lookup provider compliant to RFC 9440 Jan 2, 2025
@mposolda mposolda self-assigned this Jan 6, 2025
@seiferma seiferma mentioned this pull request Feb 8, 2025
Closed
@erichayth
Copy link

erichayth commented Mar 23, 2025

Hoping this gets implemented soon. Cloudflare can forward a client certificate in DER format only which causes the request to fail on client certificate lookup.

@mposolda
Copy link
Contributor

mposolda commented Apr 29, 2025

@shawkins @vmuzikar Removing myself as an assignee. Discussed with Stian earlier, that this PR is likely more in the area of cloud-native.

Just pointing, that if we add this, it maybe makes sense to wrap this behind feature flag? See EnvironmentDependentProviderFactory and it's usage in various providers.

@mposolda mposolda assigned vmuzikar and shawkins and unassigned mposolda Apr 29, 2025
@seiferma seiferma force-pushed the seiferma-client-cert-lookup-rfc9440 branch from db1b231 to d3630b1 Compare May 2, 2025 16:58
@seiferma
Copy link
Contributor Author

seiferma commented May 2, 2025

Just pointing, that if we add this, it maybe makes sense to wrap this behind feature flag? See EnvironmentDependentProviderFactory and it's usage in various providers.

I wasn't aware of this factory because non of the existing X509 cert lookup providers used it. I assume you suggest to use feature flags for all lookup providers and not just the one that I am proposing here, right? If so: Should this be part of my merge request or another merge request, which's main purpose is introducing feature flags for the lookup providers?

@shawkins shawkins requested review from shawkins and vmuzikar August 13, 2025 17:14
@shawkins
Copy link
Contributor

shawkins commented Aug 13, 2025

@seiferma sorry for the delay in having more consideration of this pr. My only real concern was addressed.

which's main purpose is introducing feature flags for the lookup providers?

I don't think we need to have this behind a feature flag. Usage will already be opt-in.

In general there's also #33159 and #33818 - there should be more follow-up about making the x509cert-lookup SPI public cc @mposolda

@shawkins shawkins force-pushed the seiferma-client-cert-lookup-rfc9440 branch from d3630b1 to d83e4b0 Compare August 13, 2025 17:27
@seiferma seiferma force-pushed the seiferma-client-cert-lookup-rfc9440 branch from d83e4b0 to 8032d3d Compare August 21, 2025 18:56
@seiferma
Copy link
Contributor Author

seiferma commented Aug 21, 2025 •
edited
Loading

@seiferma sorry for the delay in having more consideration of this pr. My only real concern was addressed.

No worries @shawkins . I rebased my branch and removed the null check that you mentioned. If there should be more changes, just let me know.

In general there's also #33159 and #33818 - there should be more follow-up about making the x509cert-lookup SPI public cc @mposolda

That would be nice. My temporary solution was to build the provider separately and integrate it into my keycloak docker image. The warning about using a non-public API sounded pretty dramatic but if the API is fairly stable, it might be worth to remove that restriction.

@shawkins shawkins added this to the 26.5.0 milestone Sep 23, 2025
@ahus1 ahus1 force-pushed the seiferma-client-cert-lookup-rfc9440 branch from 8032d3d to e31b936 Compare December 4, 2025 18:15
@shawkins shawkins mentioned this pull request Dec 18, 2025
Open
@vmuzikar vmuzikar force-pushed the seiferma-client-cert-lookup-rfc9440 branch from e31b936 to 6be997c Compare December 18, 2025 17:24
vmuzikar
vmuzikar previously approved these changes Dec 18, 2025
View reviewed changes
Copy link
Contributor

@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seiferma Thank you for the contribution! I just added some release notes, other than that LGTM. @shawkins Could you please re-review?

shawkins
shawkins previously approved these changes Dec 18, 2025
View reviewed changes
Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks again @seiferma

@vmuzikar vmuzikar enabled auto-merge (squash) December 19, 2025 08:56
seiferma and others added 3 commits December 19, 2025 11:35
@seiferma @vmuzikar
Client cert lookup provider compliant to RFC 9440 (keycloak#20761)
4c58707 
Signed-off-by: Stephan Seifermann <[email protected]>
@vmuzikar
Release notes
c0f37f5 
Signed-off-by: Václav Muzikář <[email protected]>
@vmuzikar
Fix spotless
a9df284 
Signed-off-by: Václav Muzikář <[email protected]>
@vmuzikar vmuzikar dismissed stale reviews from shawkins and themself via a9df284 December 19, 2025 10:36
@vmuzikar vmuzikar force-pushed the seiferma-client-cert-lookup-rfc9440 branch from 3abc5cf to a9df284 Compare December 19, 2025 10:36
@vmuzikar
Copy link
Contributor

vmuzikar commented Dec 19, 2025

I took the liberty of fixing spotless and rebasing again to make the Operator tests pass.

@vmuzikar
Copy link
Contributor

vmuzikar commented Dec 19, 2025

Failing external docs tests are unrelated.

@vmuzikar vmuzikar merged commit aefecad into keycloak:main Dec 19, 2025
80 of 81 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmuzikar vmuzikar vmuzikar approved these changes

@shawkins shawkins shawkins left review comments

Assignees

@shawkins shawkins

@vmuzikar vmuzikar

Labels

team/cloud-native team/core-clients team/core-iam

Projects

None yet

Milestone

26.5.0

Development

Successfully merging this pull request may close these issues.

Support Caddy as a Reverse Proxy Provider for Client Certificate Authentication

5 participants

@seiferma @erichayth @mposolda @shawkins @vmuzikar