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

PAYARA-3789 Unify DN Representation #4042

Merged

Conversation

Pandrex247
Copy link
Member

Should also stop requiring OIDs for most known fields (e.g. EMAILADDRESS)

@Pandrex247
Copy link
Member Author

Jenkins test please

Copy link

@rdebusscher rdebusscher left a comment

Choose a reason for hiding this comment

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

Some duplicates in Map

oidMapInitialiser.put("0.9.2342.19200300.100.1.1", "UID");
oidMapInitialiser.put("0.9.2342.19200300.100.1.25", "DC");
oidMapInitialiser.put("1.2.840.113549.1.9.1", "EMAIL");
oidMapInitialiser.put("1.2.840.113549.1.9.1", "EMAILADDRESS");

Choose a reason for hiding this comment

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

Duplicate, so always EMAILADDRESS used and never EMAIL.

@Pandrex247
Copy link
Member Author

I seem to have broken it with my recent changes, will reopen once it's all working again.

@Pandrex247 Pandrex247 closed this Jun 13, 2019
@Pandrex247 Pandrex247 reopened this Jun 14, 2019
@Pandrex247
Copy link
Member Author

Also tested using IAIK provider and seems to work

@Pandrex247
Copy link
Member Author

Jenkins test please

@@ -101,7 +103,30 @@

// Descriptive string of the authentication type of this realm.
public static final String AUTH_TYPE = "certificate";

public static final Map<String, String> oidMap;

Choose a reason for hiding this comment

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

OID_MAP as name?

@@ -173,7 +176,7 @@ private static String getPrincipalName(X509Certificate[] certificates, SecurityC
// Use the full DN name from the certificates. This should normally be the same as
// context.getCallerPrincipal(), but a realm could have decided to map the name in which
// case they will be different.
return certificates[0].getSubjectX500Principal().getName();
return certificates[0].getSubjectX500Principal().getName(X500Principal.RFC2253, CertificateRealm.oidMap);

Choose a reason for hiding this comment

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

We are using getName(X500Principal.RFC2253, CertificateRealm.oidMap) several times. Make a utility method so that X500Principal representation is only in 1 place for future?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that's really worth it? Unless I'm misunderstanding you we'd just be replacing certificates[0].getSubjectX500Principal().getName(X500Principal.RFC2253, CertificateRealm.oidMap) with CertificateRealm.getX500PrincipalSubjectName(certificates[0]).

It seems fragile to me to store the name of the certificate somewhere outside of the actual certificate itself.

Also, it isn't strictly necessary for me to put the full X500Principal.RFC2253 stuff everywhere, it's me just being safe. From my testing as long as you get the name in this manner from the CertificateRealm it propagates outward such that simply doing getName() will still have the OIDs translated.

Copy link
Contributor

@cubastanley cubastanley left a comment

Choose a reason for hiding this comment

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

I second Rudy's requested changes but other than those I think it's good

@Pandrex247
Copy link
Member Author

Jenkins test please

@Pandrex247 Pandrex247 merged commit e0dc919 into payara:master Jun 17, 2019
@Pandrex247 Pandrex247 deleted the PAYARA-3789-Unify-DN-Representation branch June 17, 2019 09:50
@Pandrex247
Copy link
Member Author

PAYARA-3937 Created to track code improvement request.

Cousjava pushed a commit to Cousjava/Payara that referenced this pull request Aug 21, 2019
…epresentation

PAYARA-3789 Unify DN Representation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants