Skip to content

Add Support Extracting DN From X500Principal #16984

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

franticticktick
Copy link
Contributor

Closes gh-16980

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 23, 2025
@jzheaux jzheaux added this to the 7.0.x milestone Apr 23, 2025
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 1, 2025
@rwinch
Copy link
Member

rwinch commented May 6, 2025

Thanks for the PR @franticticktick

Upon review, I think that it makes more sense to deprecate and create a SubjectX500PrincipalExtractor class that extracts using getSubjectX500Principal.

What are your thoughts @jzheaux ?

@rwinch rwinch self-assigned this May 6, 2025
@jzheaux
Copy link
Contributor

jzheaux commented May 7, 2025

Yes, @rwinch, I agree. Originally, I was trying to be a bit more conservative and reuse what we have, but I believe a new implementation will allow for a name that clearly indicates what it's for.

@franticticktick
Copy link
Contributor Author

franticticktick commented May 9, 2025

@rwinch @jzheaux there is one problem I would like to discuss. By default, the getName() method of the X500Principal class uses the RFC2253 format. That is, in tests, the dn will be generated:

1.2.840.113549.1.9.1=#16126c756b65406d6f6e6b65796d616368696e65,CN=Luke Taylor,OU=Open Source Development Lab.,O=Monkey Machine Ltd,L=Glasgow,ST=Scotland,C=UK

We see an oid instead of an emailaddress, which is in line with the format. At the same time, we see an encoded string instead of a value. As the specification says:

If the AttributeValue is of a type which does not have a string
representation defined for it, then it is simply encoded as an
octothorpe character ('#' ASCII 35) followed by the hexadecimal
representation of each of the bytes of the BER encoding of the X.500
AttributeValue.  This form SHOULD be used if the AttributeType is of
the dotted-decimal form.

To get a text string instead of an encoded value, we need to explicitly specify the format, this is RFC1779.

String subjectDN = clientCert.getSubjectX500Principal().getName("RFC1779");

In this case, the generated dn will look like this:

OID.1.2.840.113549.1.9.1=luke@monkeymachine, CN=Luke Taylor, OU=Open Source Development Lab., O=Monkey Machine Ltd, L=Glasgow, ST=Scotland, C=UK

In this format we can get email, but no matter what format we specify, we will still get oid in the string. Even if we leave the dn extraction by pattern, it will be very non-obvious. For example:

extractor.setFormat("RFC1779");
extractor.setSubjectDnRegex("OID.1.2.840.113549.1.9.1=(.*?),");

This is a pretty hard to understand API. At the same time, the toString() gives this result:

EMAILADDRESS=luke@monkeymachine, CN=Luke Taylor, OU=Open Source Development Lab., O=Monkey Machine Ltd, L=Glasgow, ST=Scotland, C=UK

And this will give the same result as the current implementation. If you have any ideas on how to better implement this feature, I'm ready to listen.

@rwinch
Copy link
Member

rwinch commented May 13, 2025

@franticticktick Thanks for reaching out.

I do not think that we can use the toString method because it is implementation dependent. I think that we should probably allow for the format and regex to be injected. We can have an additional boolean property for extracting the email address that in turn sets the format and regex to extract the email address. This will simplify the usage for users that do not want to dig into the details of how things work under the covers. Does that work?

@franticticktick
Copy link
Contributor Author

@rwinch I considered this solution. Provided that we provide the ability to extract dn via cn and email, this will work. Frankly speaking, I don't really like flags, they sometimes confuse, but as I understand it, in our situation there is no other solution. I will prepare a pr.

@rwinch rwinch added the status: waiting-for-feedback We need additional information before we can continue label May 14, 2025
@franticticktick franticticktick force-pushed the gh-16980 branch 2 times, most recently from c44bf81 to fa9c24b Compare May 16, 2025 12:06
@franticticktick
Copy link
Contributor Author

@rwinch @jzheaux Could you please review this PR? I hope I haven't missed anything.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 16, 2025
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @franticticktick! I think this is getting close. I'd like to preserve the idea of having a regex for extracting the principal name and allowing the user to specify the format. The difference would be that the extractPrincipalNameFromEmail would set the correct format + the correct regex without needing to understand how it works.

I'd argue that we probably don't need any properties on the DSLs but instead just allow the user to set the properties on SubjectX500PrincipalExtractor and then the extractor on the DSL (or exposed as a Bean...however it works now)

@rwinch rwinch added the status: waiting-for-feedback We need additional information before we can continue label May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: feedback-provided Feedback has been provided status: waiting-for-feedback We need additional information before we can continue type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SubjectDnX509PrincipalExtractor should update to getSubjectX500Principal
4 participants