-
Notifications
You must be signed in to change notification settings - Fork 305
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
FISH-5646: Client Certificate Validator API #5398
Conversation
if (!validators.isEmpty()) { | ||
for (ClientCertificateValidator validator : validators) { | ||
if (!validator.isValid(subject, principal, certificate)) { | ||
failed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop could stop when first validation fails depending on how expensive validations are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx. There should only be 1 or max 2 validators but added the a break
statement.
} | ||
} | ||
if (failed) { | ||
throw new LoginException("Certificate Validation Failed via API"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add some information about which validation failed. Classname?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added logging statement on line 207 as this message could potentially be returned to clients which is not wanted (don't expose internal info around security to clients in case of failure)
jenkins test please |
if (!validators.isEmpty()) { | ||
for (ClientCertificateValidator validator : validators) { | ||
if (!validator.isValid(subject, principal, certificate)) { | ||
_logger.info(() -> String.format("Client Certificate validation failed for (subject=%s, principal=%s) by %s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this worth a warning instead of an info log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. There is also not a Warning when an invalid username/password is presented.
FISH-5646: Client Certificate Validator API
Description
New Feature, limited functionality of #5241 as it requires other PRs that can't be moved to Enterprise yet.
Important Info
Testing
Testing Performed
Used reproducer application for feature to check if it works as intended.
Interop EJB runs successfully (was issue with previous attempt)
Testing Environment
Zulu 8.52.0.23-CA-macosx (build 1.8.0_282-b08) on Mac 11.5 with Maven 3.6.3
Documentation
TODO
Notes for Reviewers