[keycloak-dev] User TLS client certificate authentication - inconsistent DN string representation with LDAP
Marek Posolda
mposolda at redhat.com
Tue Jan 29 06:30:46 EST 2019
The reason for intoduce LdapDn class instead of LdapName was the
compatibility issue, but I can't recall the details... Some more below
On 19/01/2019 05:04, Nalyvayko, Peter wrote:
>> I am not very keen on uppercasing attribute type in the proposed patch though it might be the most pragmatic way to go. Is it supported by any DN > RFC? In any case, any such a normalization would have to take place consistently in both places.
> An alternative is to modify X500NameRDNExtractor.extractUserIdentity in keycloak/services/src/main/java/org/keycloak/authentication/authenticators/x509/UserIdentityExtractor.java to return new LdapName(IETFUtils.valueToString(cn.getFirst().getValue())).toString() instead of IETFUtils.valueToString(cn.getFirst().getValue()) (line 92), thus in essence normalizing the user identity from subjectDN/issuerDN to the representation conformant to a variant of RFC2253 format which can then be safely compared to LDAP_ENTRY_DN attribute.
I think that either this change or the proposed change in LdapDn will
work. However change in X500NameRDNExtractor has the advantage, that it
will work immediately for the existing users imported from LDAP.
I mean that if we do the change in the LdapDn class, then the existing
users in Keycloak DB will still have the LDAP_ENTRY_DN attribute on them
in the old format like "uid=john,dc=example,dc=com" . So until the
particular user is re-synced from LDAP to Keycloak DB, it won't be
successfully lookup when it is searched by authenticator for the
attribute in new format like "UID=john, DC=example, DC=com" . As the
change of the LDAP_ENTRY_DN to new format will be triggered by re-sync
from LDAP.
Marek
>
>
> ________________________________________
> From: Hynek Mlnarik [hmlnarik at redhat.com]
> Sent: Tuesday, January 15, 2019 5:59 AM
> To: Nalyvayko, Peter
> Cc: Sebastian Laskawiec; John Dennis; keycloak-dev at lists.jboss.org
> Subject: Re: [keycloak-dev] User TLS client certificate authentication - inconsistent DN string representation with LDAP
>
> We should adhere to the standard implementations as much as possible. However there likely was a reason for introducing LDAPDn class, possibly due to some supported LDAP server having different implementation than allowed by Java javax.naming.ldap.LdapName and similar. @Marek, could you please comment here?
>
> Sidenote: Currently the javax.naming.ldap.LdapName class respects RFC 2253 which was obsoleted by RFC 4514. I believe the differences are irrelevant in the domain of user ids but comments are welcome.
>
> Since a DN is stored as string user attribute value and searched for as such, we have to stick to canonical string representation. This can be obtained via javax.naming.ldap.LdapName.toString method which should be used both in the LDAPDn and the authenticator. It is in the latter, but not in the former.
>
> I am not very keen on uppercasing attribute type in the proposed patch though it might be the most pragmatic way to go. Is it supported by any DN RFC? In any case, any such a normalization would have to take place consistently in both places.
>
> Ideally the comparison would be able to recognize that the following inputs are just the same:
> - DC=example,DC=com
> - DC=example, DC=com
> - dc=example,dc=com
> - dc=ex\61mple,dc=com
> - 0.9.2342.19200300.100.1.25=example,0.9.2342.19200300.100.1.25=com
>
>
> On Thu, Jan 10, 2019 at 3:21 AM Nalyvayko, Peter <pnalyvayko at agi.com<mailto:pnalyvayko at agi.com>> wrote:
> Hi Sebastian,
>
> Short of trying to extend the authenticator itself to handle such cases, one possible short term approach can be to implement an attribute mapper responsible for importing and translating the LDAP's DN into representation that matches the canonical representation of the subject DN returned by X509Cert.getSubjectDN().getName() and configuring the x509 authenticator to compare the subjectDN against that attribute.
>
>
> ________________________________________
> From: Sebastian Laskawiec [slaskawi at redhat.com<mailto:slaskawi at redhat.com>]
> Sent: Wednesday, January 9, 2019 10:11 AM
> To: John Dennis
> Cc: Nalyvayko, Peter; Peck, Michael A; sblanc at redhat.com<mailto:sblanc at redhat.com>; keycloak-dev at lists.jboss.org<mailto:keycloak-dev at lists.jboss.org>
> Subject: Re: [keycloak-dev] User TLS client certificate authentication - inconsistent DN string representation with LDAP
>
> Hey guys,
>
> Answering a few email in a row below.
>
> Thanks,
> Sebastian
>
> On Wed, Jan 9, 2019 at 12:19 AM John Dennis <jdennis at redhat.com<mailto:jdennis at redhat.com><mailto:jdennis at redhat.com<mailto:jdennis at redhat.com>>> wrote:
> On 1/8/19 5:02 PM, Nalyvayko, Peter wrote:
>> Hi,
>>
>>>> I believe that's a bug. The `X509ClientCertificateAuthenticator` should ignore those extra spaces. May I kindly ask you to create a ticket for us and assign it either to me or Sebastien?
>> Sebastian/Michael,
>>
>> According to https://tools.ietf.org/html/rfc1779, BNF for distinguished name allows for optional space before and after the separator. Do you know of any reason why the DN returned by LDAP and the DN returned by calling to X509Certificate.getSubjectDN().getName() should or expected be identical? It seems to me BNF allows for some discrepancies in representation thus comparing two strings verbatim may not be a good idea, no?
> Let me try to reiterate the whole flow to make sure I understand everything correctly:
> - X509ClientCertificateAuthenticator extracts certificates from an incoming HTTP request using one of the implementations of X509ClientCertificateLookup.
> - Obtained certificate chain is of a type X509Certificate[].
> - Then we extract a User identity based on his certificate (certs[0].getSubjectDN().getName()).
> - Once we have a User identity in our hands, we can extract a UserModel, our representation of a User in Keycloak. This user might be imported from LDAP. Here's where those two pieces play together.
> - If we find a user and he's valid, we end the flow.
> So the root problem is that certificate obtained from the request doesn't match the one from LDAP, even though they are maintained in a single place (somehow, details omitted).
>
> So, yes, this situation may happen since in the essence, we do just a DN comparison by strings. The easiest workaround in my opinion is to normalize those DNs across Keycloak codebase. The simplest solution is to modify LDAPDn class as Michel suggested. I believe it should also work if we modify a user certificate attribute and put spaces there (", ").
>
> At first I though the fix should go X509ClientCertificateAuthenticator but obviously, I was wrong. It's just a mismatch problem.
>
> RFC 1779 (A String Representation of Distinguished Names) also mentions different separators like for example ";", "," or ", " (comma with or without a space). In order to support all this usecases, we'd probably need to implement a custom DN comparator and modify our database queries that extract users based on DN. At this point I'm not 100% convinced we need this. Maybe we should just say in the docs that if you synchronize users with LDAP, you should use ", " as a separator when specifying user certificates?
>
> @Michael, @Peter - WDYT about that? Would that solution solve your problem?
>
> @Hynek, @Marek - maybe you guys have some thoughts around this?
>
>
> On previous projects I did a lot of work with DN handling, I believe the
> relevant RFC is 4514 (unless it's been superseded). The short answer is
> it's never correct to compare DN's via string comparison unless the DN
> was normalized to a consistent representation. The reason is there are
> multiple valid string representations of the same DN. Aside from issues
> related to whitespace, encoding, etc. you need to remember that there is
> such a thing as multi-valued RDN's (e.g. an RDN with an UNORDERED set of
> AVA's), any ordering of the AVA's in the RDN yield a semantically
> equivalent RDN.
>
> Most libraries that compare DN's do so by parsing the string
> representation and building a data structure consisting of the
> individual components which are then iterated over during comparison.
> Once the DN has been parsed into it's constituent components there is
> usually a routine to convert it back into a string representation. This
> can be used to normalize the string representation thus permitting a
> simple string compare to check for equality.
>
>> Kindly,
>> Peter
>>
>>
>> -----Original Message-----
>> From: keycloak-dev-bounces at lists.jboss.org<mailto:keycloak-dev-bounces at lists.jboss.org><mailto:keycloak-dev-bounces at lists.jboss.org<mailto:keycloak-dev-bounces at lists.jboss.org>> <keycloak-dev-bounces at lists.jboss.org<mailto:keycloak-dev-bounces at lists.jboss.org><mailto:keycloak-dev-bounces at lists.jboss.org<mailto:keycloak-dev-bounces at lists.jboss.org>>> On Behalf Of Sebastian Laskawiec
>> Sent: Monday, January 7, 2019 7:36 AM
>> To: Peck, Michael A <mpeck at mitre.org<mailto:mpeck at mitre.org><mailto:mpeck at mitre.org<mailto:mpeck at mitre.org>>>; sblanc at redhat.com<mailto:sblanc at redhat.com><mailto:sblanc at redhat.com<mailto:sblanc at redhat.com>>
>> Cc: keycloak-dev at lists.jboss.org<mailto:keycloak-dev at lists.jboss.org><mailto:keycloak-dev at lists.jboss.org<mailto:keycloak-dev at lists.jboss.org>>
>> Subject: Re: [keycloak-dev] User TLS client certificate authentication - inconsistent DN string representation with LDAP
>>
>> Hey Michael,
>>
>> Adding +Sebastien Blanc <sblanc at redhat.com<mailto:sblanc at redhat.com><mailto:sblanc at redhat.com<mailto:sblanc at redhat.com>>> for visibility.
>>
>> I believe that's a bug. The `X509ClientCertificateAuthenticator` should ignore those extra spaces. May I kindly ask you to create a ticket for us and assign it either to me or Sebastien?
>>
>> Thanks,
>> Sebastian
>>
>> On Sun, Dec 23, 2018 at 6:49 PM Peck, Michael A <mpeck at mitre.org<mailto:mpeck at mitre.org><mailto:mpeck at mitre.org<mailto:mpeck at mitre.org>>> wrote:
>>
>>> Hello,
>>>
>>> I’ve configured Keycloak to authenticate users using TLS client
>>> certificate authentication.
>>> I’ve also configured Keycloak to synchronize users with my LDAP server.
>>>
>>> I’d like to match the TLS client certificate’s Subject DN to the
>>> Subject DNs synchronized from my LDAP server (which are stored by
>>> Keycloak in each user’s LDAP_ENTRY_DN attribute).
>>>
>>> I’ve set that up, but am running into an issue that Keycloak appears
>>> to have inconsistent string representations of DNs between those two
>>> methods - so the Subject DNs from the TLS client certificate and the
>>> LDAP server aren’t matching as I was expecting.
>>>
>>> The TLS client certificate DNs look like this:
>>> CN=Peck Michael, OU=People, DC=test, DC=net
>>>
>>> While the LDAP_ENTRY_DN attribute is formatted like this:
>>> cn=Peck Michael,ou=People,dc=test,dc=net
>>>
>>> It looks to me that the TLS client certificate DN string
>>> representation is coming from the standard Java X500Principal class
>>> used by calls to
>>> X509Certificate.getSubjectDN().getName() in
>>> keycloak/services/src/main/java/org/keycloak/authentication/authentica
>>> tors/x509/X509ClientCertificateAuthenticator.java
>>> and the LDAP_ENTRY_DN string representation is coming from the
>>> toString method in
>>> keycloak/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/model/LDAPDn.java.
>>>
>>> I modified the LDAPDn class’s toString method to follow the same
>>> format as used in the TLS client certificate DNs, and authentication works for me now.
>>> Would the Keycloak project consider accepting a pull request to change
>>> the way LDAPDn formats DNs as strings?
>>> (However I have not checked if this would impact other uses of the
>>> LDAPDn class within Keycloak or cause problems with upgrading existing
>>> deployments?)
>>>
>>> The suggested change follows:
>>> diff --git
>>> a/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/model/LD
>>> APDn.java
>>> b/federation/ldap/src/main/
>>> index 39e7d97..2f8c805 100644
>>> ---
>>> a/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/model/LD
>>> APDn.java
>>> +++
>>> b/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/model/LD
>>> APDn.java @@ -87,9 +87,9 @@ public class LDAPDn {
>>> if (first) {
>>> first = false;
>>> } else {
>>> - builder.append(",");
>>> + builder.append(", ");
>>> }
>>> -
>>> builder.append(rdn.attrName).append("=").append(rdn.attrValue);
>>> +
>>> builder.append(rdn.attrName.toUpperCase()).append("=").append(rdn.attrValue);
>>> }
>>>
>>> return builder.toString();
>>>
>>>
>>>
>>> Thank you,
>>> Michael Peck
>>> The MITRE Corporation
>>> _______________________________________________
>>> keycloak-dev mailing list
>>> keycloak-dev at lists.jboss.org<mailto:keycloak-dev at lists.jboss.org><mailto:keycloak-dev at lists.jboss.org<mailto:keycloak-dev at lists.jboss.org>>
>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>> _______________________________________________
>> keycloak-dev mailing list
>> keycloak-dev at lists.jboss.org<mailto:keycloak-dev at lists.jboss.org><mailto:keycloak-dev at lists.jboss.org<mailto:keycloak-dev at lists.jboss.org>>
>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>
>> _______________________________________________
>> keycloak-dev mailing list
>> keycloak-dev at lists.jboss.org<mailto:keycloak-dev at lists.jboss.org><mailto:keycloak-dev at lists.jboss.org<mailto:keycloak-dev at lists.jboss.org>>
>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>
>
> --
> John Dennis
>
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev at lists.jboss.org<mailto:keycloak-dev at lists.jboss.org>
> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-dev
More information about the keycloak-dev
mailing list