[keycloak-user] Validation of IdP SAML signatures using KeyInfo

[Mark Pardijs]

Ah, that makes sense, thanks! 

> Op 31 jan. 2017, om 15:05 heeft Hynek Mlnarik <hmlnarik at redhat.com> het volgende geschreven:
> 
> That's because with Keycloak on both server and client side, key ID
> can be used to look up the particular signing key without attempting
> to validate using other irrelevant keys, see [1, Optimize REDIRECT
> signing key lookup option] and [2].
> 
> [1] https://keycloak.gitbooks.io/server-adminstration-guide/content/topics/clients/client-saml.html
> [2] https://keycloak.gitbooks.io/securing-client-applications-guide/content/v/2.5/topics/saml/java/general-config/idp_keys_subelement.html.
> 
> On Tue, Jan 31, 2017 at 9:43 AM, Mark Pardijs <mark.pardijs at topicus.nl> wrote:
>> Yep, that’s what I mean ;) That still leaves me curious why the XmlSignatureUtil is looking up the keyName when in the end this keyName is never used...
>> 
>>> Op 30 jan. 2017, om 13:39 heeft Hynek Mlnarik <hmlnarik at redhat.com> het volgende geschreven:
>>> 
>>> Keys specified in admin console are checked regardless of key ID. This applies just the same to the case when there is only a single key.
>>> 
>>> On 01/30/2017 12:54 PM, Mark Pardijs wrote:
>>>> Ah OK, I see what you mean, so the idea is, when no key is found using the key hint all keys are checked. But what if I do provide a KeyName hint in the SAML, then I still see a mismatch between the code and the Keycloak admin frontend, the code is returning the first key regardless which key id is provided, but in the frontend, no key id’s can be specified, just a comma seperated list. Can you clarify this?
>>>> 
>>>> Op 30 jan. 2017, om 12:09 heeft Hynek Mlnarik <hmlnarik at redhat.com<mailto:hmlnarik at redhat.com>> het volgende geschreven:
>>>> 
>>>> Thanks for the report. Fix for item 1 is on the way [1]. Item 2 - validation - goes enumerating all available keys if getKey() returns null so that part should work fine.
>>>> 
>>>> --Hynek
>>>> 
>>>> On 01/30/2017 10:55 AM, Mark Pardijs wrote:
>>>> Hi,
>>>> 
>>>> Ad 1: Just created the issue: https://issues.jboss.org/browse/KEYCLOAK-4329
>>>> Ad 2: Multiple keys can be provided to the HardcodedKeyLocator, but  I see the following code for checking a specific key:
>>>> 
>>>> public Key getKey(String kid) {
>>>>  if (this.keys.size() == 1) {
>>>>      return this.keys.iterator().next();
>>>>  } else {
>>>>      return null;
>>>>  }
>>>> }
>>>> 
>>>> And the XMLSignatureUtil is using locator.getKey(keyName) for looking up the keys.
>>>> 
>>>> So even if I would provide a KeyName in my SAML, it would return the first configured SAML certificate right?
>>>> 
>>>> Op 30 jan. 2017, om 10:42 heeft Hynek Mlnarik <hmlnarik at redhat.com<mailto:hmlnarik at redhat.com><mailto:hmlnarik at redhat.com>> het volgende geschreven:
>>>> 
>>>> Hi,
>>>> 
>>>> Ad 1: Could you file a JIRA with more details (NPE stacktrace, Keycloak version) for this? Keycloak handles cases where KeyName is not present by checking all available keys.
>>>> 
>>>> Ad 2: HardcodedKeyLocator works with a collection of keys so it matches multiple keys configuration. Maybe the cause of this question is related to Item 1, let's resolve that issue first.
>>>> 
>>>> --Hynek
>>>> 
>>>> On 01/30/2017 10:09 AM, Mark Pardijs wrote:
>>>> Hi,
>>>> 
>>>> Originally posted at the keycloak-dev list, Hynek Mlnarik asked me to post this here.
>>>> 
>>>> We use a SAML IdP which is configured in Keycloak as federated IdP, and I’ve a question concerning the validation of SAML signatures. In Keycloaks Identity provider config page, the validating X509 Certificates can be configured, with description “The certificate in PEM format that must be used to check for signatures. Multiple certificates can be entered, separated by comma (,).” but in the code, I see that for checking the signatures a “HardcodedKeyLocator" is used, which does not use the keyName provided in the SAML but always returns the first configured certificate. See org.keycloak.broker.saml.SAMLEndpoint.Binding#getIDPKeyLocator which returns a HardcodedKeyLocator for details.
>>>> 
>>>> This code is recently added to solve https://issues.jboss.org/browse/KEYCLOAK-1881, see commit https://github.com/keycloak/keycloak/commit/70a8255eae0af64628f07326df1c73d86c1b9fd2.
>>>> 
>>>> My two questions concerning this approach:
>>>> 
>>>> 
>>>> 1.  Keycloak is currently expecting a <KeyInfo> element with a <KeyName> in the  incoming SAML message, while this is not a required element in the SAML specs. Are there plans to check the signature against the configured X509 certificates without having to provide a KeyInfo element? Currently I”m facing a NullPointer exception when sending a SAMLResponse without KeyInfo
>>>> 
>>>> 2.  What’s the idea behind the HardcodedKeyLocator, it doesn’t seem to match with the multiple keys configuration option in Keycloaks frontend. Is this a preliminary approach which should be extended?
>>>> 
>>>> _______________________________________________
>>>> keycloak-user mailing list
>>>> keycloak-user at lists.jboss.org<mailto:keycloak-user at lists.jboss.org>
>>>> https://lists.jboss.org/mailman/listinfo/keycloak-user
>>>> 
>>>> _______________________________________________
>>>> keycloak-user mailing list
>>>> keycloak-user at lists.jboss.org
>>>> https://lists.jboss.org/mailman/listinfo/keycloak-user
>>>> 
>> 
>> 
>> _______________________________________________
>> keycloak-user mailing list
>> keycloak-user at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/keycloak-user
> 
> 
> 
> -- 
> 
> --Hynek