Skip to content
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.

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

Fix CRL verification failing due to client cert not being in chain #29582

Merged
merged 2 commits into from
May 17, 2024

Conversation

rmartinc
Copy link
Contributor

Closes #19853

Just superseding PR #24760 to include a test with a single certificate with no CA trust anchor in the chain.

Signed-off-by: Micah Algard <micahalgard@gmail.com>
jonkoops
jonkoops previously approved these changes May 16, 2024
services/src/main/java/org/keycloak/utils/CRLUtils.java Outdated Show resolved Hide resolved
services/src/main/java/org/keycloak/utils/CRLUtils.java Outdated Show resolved Hide resolved

@Drone
@PhantomJSBrowser
private WebDriver phantomJS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we'd try to avoid introducing more PhantomJS (see #9979), but since AbstractX509AuthenticationTest is heavily based on it there is very little we can do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All x509 tests use the phantom, so I just followed the same idea. So when the x509 tests were changed to use another thing this one will be just one more test to change. Not doing anything here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, nothing that can be done about it until the testing team gets to it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 , FYI. There is WiP by @Aboullos from QA team for refactoring of X.509 tests to make them get rid of PhantomJS

@mposolda mposolda self-assigned this May 16, 2024
Closes keycloak#19853

Signed-off-by: rmartinc <rmartinc@redhat.com>
Copy link
Contributor

@jonkoops jonkoops left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

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

@rmartinc @jonkoops Thanks for the fix and review!

@mposolda mposolda merged commit 74a8099 into keycloak:main May 17, 2024
70 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CRL Verification failing due to client certificate not being in a chain
4 participants