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

Support certificate role mappings #37269

Merged

Conversation

sberyozkin
Copy link
Member

Fixes #23683.

Tests to follow, also I thought I could avoid registering an augmentor and just pass the roles directly to the default X509 identity provider.

If these mappings are not comping from the file/classpath resource then a custom SecurityIdentityAugmentor will help.

I'll open for review later but early feedback is welcome

@sberyozkin sberyozkin force-pushed the certificate-role-mapping-support branch 2 times, most recently from 350ff1b to 0f7deec Compare November 24, 2023 20:15
@sberyozkin sberyozkin marked this pull request as ready for review November 24, 2023 20:16
@sberyozkin
Copy link
Member Author

sberyozkin commented Nov 24, 2023

I wanted to modify integration-tests/vertx-http but it is the only test which tests a truststore alias option which I thought was important to retain while for this PR tests the whole truststore should be trusted to test the client certificates with different mappings

This comment has been minimized.

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

LGTM, few notes, two important, others you can ignore

@sberyozkin
Copy link
Member Author

Thanks @michalvavrik, let me deal with the comments, all of them make sense

@sberyozkin sberyozkin force-pushed the certificate-role-mapping-support branch from 0f7deec to c91f800 Compare November 25, 2023 17:39

This comment has been minimized.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 28, 2023

I'm sure someone will come in the future asking to use a different certificate attribute :-), but CN seems good enough for a start.

This comment has been minimized.

Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

LGTM, for what that's worth, except for one problem around loading the properties file.

(In case anyone is wondering: I reviewed this code mainly because I was curious having tinkered with this kind of thing a bit back in my days contributing to the WildFly Elytron project.)

@sberyozkin
Copy link
Member Author

Thanks @dmlloyd, your feedback is very welcome

@sberyozkin sberyozkin force-pushed the certificate-role-mapping-support branch from f546eab to 217511a Compare December 5, 2023 13:11
@sberyozkin
Copy link
Member Author

@dmlloyd Hi David, I think I've addressed your comments, which was easy since your typed how to address them :-), if there is anything else that you'd like to change, let me know please

Copy link

quarkus-bot bot commented Dec 5, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@sberyozkin
Copy link
Member Author

Thanks @dmlloyd @michalvavrik and @Ladicek for the reviews. Let me merge and as always, things can be improved/optimized going forward

@sberyozkin sberyozkin merged commit 2aeab7f into quarkusio:main Dec 5, 2023
49 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Dec 5, 2023
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 5, 2023
@sberyozkin sberyozkin deleted the certificate-role-mapping-support branch December 28, 2023 19:59
@cescoffier
Copy link
Member

@sberyozkin Just made aware of this. Why didn't you reuse the mechanism defined in https://datatracker.ietf.org/doc/html/rfc5755#section-4.4.5 instead of using a custom attribute (which lead to interoperability issue)

@cescoffier
Copy link
Member

At least it should be X-Roles (or a valid syntax).

@sberyozkin
Copy link
Member Author

@cescoffier Sorry, I have missed it.

Why didn't you reuse the mechanism defined in https://datatracker.ietf.org/doc/html/rfc5755#section-4.4.5 instead of using a custom attribute (which lead to interoperability issue)

We don't expect here CName to contain any roles, the requirement this PR addresses is only about using Cname as a key to pick up the deployment specific roles from the local file.

As far as expecting the certificate already have the roles, in the X-Roles attribute, please open an enhancement request if you'd like, interesting idea, it won't be in conflict with this PR

@cescoffier
Copy link
Member

Shouldn't it use SAN and not the CN? Like for SNI.

@sberyozkin
Copy link
Member Author

@cescoffier This PR addresses a specific requirement, use CName as a map key.

But sure, we can make the attribute name configurable, sounds good to me.

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.

Support role mapping for Client certificates
5 participants