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

fix c14n canonization #15

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

zuckschwerdt
Copy link

omit a default namespace if it is not visibly utilized; the specs suggest that we need to remove even a default namespace, if it is no visibly utilized on the parent. I.e. omit a default ns if the parent is prefixed.

@arekinath
Copy link
Owner

Would you mind adding a test for this? Also, it'd be great if you could double-check that this is what libxml does as well. It's probably more important for esaml that we do what libxml does than what is actually in the spec -- pretty much all of the SAML implementations we need to interoperate with use it.

@zuckschwerdt zuckschwerdt force-pushed the fix-c14n-omit-ns branch 2 times, most recently from 6e62d62 to 403022f Compare September 22, 2015 18:57
@zuckschwerdt
Copy link
Author

I added a quick test to illustrate the problem.
A quick check with libxml2 2.9.2 (using lxml 3.4.1) doesn't agree:/ I noticed the problem with some java library – I'll post a comparison shortly.

@arekinath
Copy link
Owner

Any update on this?

@srenatus
Copy link

👍 This change actually makes esaml play nice with Azure AD, and thus fixes #20.

Comparing the c14n of the Reponse's SignedInfo with the output of another tool turned out that this is the only difference (this is git diff --word-diff)

diff --git a/sigInfoCann.xml b/sigInfoCann.xml
index 77b4807..75f4b23 100644
--- a/sigInfoCann.xml
+++ b/sigInfoCann.xml
@@ -1 +1 @@
[-<ds:SignedInfo xmlns="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:ds="http://www.w3.org/20
00/09/xmldsig#"><ds:CanonicalizationMethod-]{+<ds:SignedInfo><ds:CanonicalizationMethod+} Alg
orithm="http://www.w3.org/2001/10/xml-exc-c14n#"></ds:CanonicalizationMethod><ds:SignatureMet
hod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"></ds:SignatureMethod><ds:Re
ference URI="#_4e59a276-9960-4fc1-9826-3982374a921e"><ds:Transforms><ds:Transform Algorithm="
http://www.w3.org/2000/09/xmldsig#enveloped-signature"></ds:Transform><ds:Transform Algorithm
="http://www.w3.org/2001/10/xml-exc-c14n#"></ds:Transform></ds:Transforms><ds:DigestMethod Al
gorithm="http://www.w3.org/2001/04/xmlenc#sha256"></ds:DigestMethod><ds:DigestValue>oOBhXzKPK
yKrJe0tdqRo4D0WL/Pk1KnWihZewS7eJ/8=</ds:DigestValue></ds:Reference></ds:SignedInfo>

Thanks!

@srenatus srenatus mentioned this pull request May 13, 2016
@zuckschwerdt
Copy link
Author

I could only verify with Apache Axis that this change provides compatible behavior. Especially to libxml this is an incompatible change.
I'd either keep this change in an "axis-compat" branch for Axis (and posssibly Azure AD) users, or extend the change to be an optional runtime setting.
(A third option could be to check if any of the two c14n styles matches with the supplied digest. I guess this would be too much work and a rather dirty approach, though.)

I'm really unhappy the specs are unclear on this and big libraries implement it differently.

@srenatus
Copy link

Especially to libxml this is an incompatible change.

I'm not sure I follow: so current esaml (without your fix) matches what libxml does?

Do we know of an IdP that expects this behaviour?

You're right, c14n shouldn't leave any variance, or "styles", though...it'd be a bad spec for this use case...

@zuckschwerdt
Copy link
Author

@srenatus I only use esaml with Apache-Axis at this time. Hard to say how other people are using this.
The change will only break libxml compatibility (and provide Axis compatibility) if there are unused default namespaces in the document. The provider can just leave those out and everything will be fine.
But if there are unused default namespaces Axis (and in my opinon the c14n spec) tells us to remove those before computing the digest. libxml however will compute a digest including the unneeded namespaces.

Summing up:

  • libxml gives you xml that should digest "as is" but I'd say the authors didn't read c14n careful enough.
  • Axis gives you xml and wants you to remove bits before digest, one could argue according to c14n.
  • But both shouldn't produce XML that is ambiguous in the first place, especially Axis, why send XML that is significantly different to the canonical form?

Going forward:

  • if we know the provider type we can adapt our canonicalization behaviour, or
  • we can try both digests (no harm there in my opinion, but it's rather unexpected), or
  • we keep a seperate, up-to-date axis-compat branch for implementors to choose.

…utilized; the specs suggest that we need to remove even a default namespace, if it is no visibly utilized on the parent. I.e. omit a default ns id the parent is prefixed.
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.

None yet

3 participants