[keycloak-user] SAML Advice assertion with signature

Thu Jul 5 03:49:58 EDT 2018

Sounds great Arjan!

Feel free to ping me in case you hit any problems :)

On Thu, Jul 5, 2018 at 8:39 AM Arjan Lamers <a.lamers at first8.nl> wrote:

> Hi Sebastian,
>
> Nice to meet again! ;) Everything well, here! How’s life over there?
>
> Wrt our issue: currently we managed to get the X.509’s of the missing
> signatures so the issue is (fortunately) less urgent.
>
> We are using a custom extension to the SAML provider to support
> eHerkenning since KeyCloak seems to make some assumptions that do not hold
> (e.g. entityId is based on the uri). I’ll try to make some time in the next
> weeks to see if I can offer some patches in this area, or if we can share
> the eHerkenning stuff in some other way as well.
>
> (FYI: eHerkenning is a Dutch company identification provider and is part
> of the European eIDAS scheme. May thus be interesting to support out of the
> box for Keycloak.)
>
> Kind regards,
> Arjan
>
>
> On 04-07-2018, at 13:40, Sebastian Laskawiec <slaskawi at redhat.com> wrote:
>
> Hey Arjan,
>
> Long time no see! I hope you're well!
>
> More comment inlined.
>
> Thanks,
> Sebastian
>
> On Thu, Jun 28, 2018 at 4:53 PM Arjan Lamers <a.lamers at first8.nl> wrote:
>
>> Hi,
>>
>> We are running KeyCloak 3.4.3-Final for a client  and are running into
>> trouble with an identity provider (the dutch eHerkenning) that is using
>> SAML Advice tags.
>>
>> We were running an older version of KeyCloak and recently that identity
>> provider started to use <saml:Advice> tags in their responses. We found
>> https://issues.jboss.org/browse/KEYCLOAK-5644, adding support for the
>> Advice tag and that made us upgrade to 3.4.3. However, this patch does not
>> seem to be complete.
>>
>> The patch there ignores the Advice tag when parsing the document. This is
>> fine. However, in our case, the Advice contains two Assertions, both of
>> which are signed (have a Signature tag). The document verification seems to
>> also validate these signatures. This is a problem, since we do not have the
>> keys for these advices, hence the validation fails.
>>
>> We have been advised to fully ignore the Advice tag, including the
>> underlying signatures. I am not a SAML expert but that feels a bit wrong.
>> Any thoughts on that?
>>
>
> Perhaps Hynek or Stian could correct me here but the spec says the
> `Advice` elements can be completely ignored:
>
> "The <Advice> element contains any additional information that the SAML
> authority wishes to provide.
> This information MAY be ignored by applications without affecting either
> the semantics or the validity of
> the assertion."
>
> [1] Page 26
> https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf
>
>
>>
>> However, if we do want to go down this road, we would probably patch this
>> in
>>
>>  org.keycloak.saml.processing.core.util.XMLSignatureUtil.validate(Document
>> signedDoc, final KeyLocator locator)
>> by skipping over nodes that have an ‘Advice’ parent.
>>
>
> This situation seems to be very tricky here. Looking through the spec I
> found that `Advice` tags need to be validated using lax XML validation
> (just as a reminder, lax validation doesn't fail if the schema is not
> found) [2]:
>
> "<Advice> and AdviceType: In addition to SAML-native elements, allows
> elements from other
> namespaces with lax schema validation processing."
>
> There's also a note on signature verification [3]:
>
> "The SAML assertion MAY be signed, which provides both
> authentication of the issuer and integrity protection.
> If such a signature is used, then the <ds:Signature> element MUST be
> present, and a relying party
> MUST verify that the signature is valid (that is, that the assertion has
> not been tampered with) in
> accordance with [XMLSig]. If it is invalid, then the relying party MUST
> NOT rely on the contents of the
> assertion. If it is valid, then the relying party SHOULD evaluate the
> signature to determine the identity and
> appropriateness of the issuer and may continue to process the assertion in
> accordance with this
> specification and as it deems appropriate (for example, evaluating
> conditions, advice, following profile specific
> rules, and so on)."
>
> So to sum it up:
>
>    - Advice tag can be ignored completely
>    - Advice tag needs only lax validation, so it just needs to be a well
>    formed XML, and that's basically it.
>    - If a verifier finds a signature, it needs to validate it. If the
>    signature doesn't pass the validation, we can not rely on the assertion.
>
> From this point, I guess patching the SignatureUtil to skip verification
> of signatures located in `Advice` element, seems not to violate anything.
> That's what we should do in my opinion.
>
> [2] Page 75
> https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf
> [3] Page 16
> https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf
>
>
>>
>> Would that be an appropriate approach? Would you be interested in such a
>> patch?
>>
>
> Yes, definitely! Let's start with a patch for master branch and later on,
> we could cherry-pick it to 3.x if needed.
>
>
>>
>>
>> Met vriendelijke groet,
>>
>> Arjan Lamers
>> Software Architect
>> +31 (0)6 23 82 24 05
>>
>>
>>
>> a.lamers at first8.nl
>> https://www.first8.nl <http://www.first8.nl/>
>> Linkedin https://www.linkedin.com/in/arjanl <
>> https://www.linkedin.com/in/profiel-id>
>> Kerkenbos 1059b
>> <https://maps.google.com/?q=Kerkenbos+1059b+6546+BB+Nijmegen&entry=gmail&source=g>
>> 6546 BB Nijmegen
>> <https://maps.google.com/?q=Kerkenbos+1059b+6546+BB+Nijmegen&entry=gmail&source=g>
>>
>> Bekijk hier de algemene voorwaarden van Conclusion <
>> https://www.conclusion.nl/kleine-lettertjes/algemene-voorwaarden>
>>
>>
>> _______________________________________________
>> keycloak-user mailing list
>> keycloak-user at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/keycloak-user
>
>
> Met vriendelijke groet,
>
> Arjan Lamers
>
> Software Architect
>
> +31 (0)6 23 82 24 05
>
>
> a.lamers at first8.nl
>
> https://www.first8.nl <http://www.first8.nl>
>
> Linkedin https://www.linkedin.com/in/arjanl
> <https://www.linkedin.com/in/profiel-id>
>
> Kerkenbos 1059b
> <https://maps.google.com/?q=Kerkenbos+1059b+%0D%0A+6546+BB+Nijmegen&entry=gmail&source=g>
>
> 6546 BB Nijmegen
> <https://maps.google.com/?q=Kerkenbos+1059b+%0D%0A+6546+BB+Nijmegen&entry=gmail&source=g>
>
> Bekijk hier de algemene voorwaarden van Conclusion
> <https://www.conclusion.nl/kleine-lettertjes/algemene-voorwaarden>
>
> <https://maps.google.com/?q=Kerkenbos+1059b+%0D%0A+6546+BB+Nijmegen&entry=gmail&source=g>
>
>