[keycloak-user] SAML Advice assertion with signature

Wed Jul 11 04:36:32 EDT 2018

This is an interesting case. Could you in the first place create a JIRA and
attach some example to it? Feel free to remove any sensitive information
from the SAML document and/or set the JIRA as security-sensitive issue so
that the attachments remain confidential.

Patch that would ignore any dsig:Signature that occurs below Advice tag -
just like you mentioned - would be most likely the one to follow but I'd
like to inspect the SAML document first to understand the context fully.

We don't provide patches to older upstream versions though, the patch would
land into the newest version only.

Thanks

--Hynek

On Wed, Jul 4, 2018 at 1:41 PM 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
>> 6546 BB Nijmegen
>>
>> 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
>
>