Add WantAssertionsSigned

[HendrikJan]

Description

This is related to #533

This change adds the option "wantAssertionsSigned" to the SamlStrategy options.
When this option is set to true, WantAssertionsSigned="true" is added to the metadata when calling samlStrategy.generateServiceProviderMetadata(...).
When this option is set to false, WantAssertionsSigned is not added to the metadata, which is de default.

This feature is described in the SAML Spec:

WantAssertionsSigned [Optional]
Optional attribute that indicates a requirement for the saml:Assertion elements received bythis service provider to be signed.If omitted, the value is assumed to be false. This requirementis in addition to any requirement for signing derived from the use of a particular profile/bindingcombination. [E7]Note that an enclosing signature at the SAML binding or protocol layer does notsuffice to meet this requirement, for example signing a samlp:Response containing theassertion(s) or a TLS connection.

(Line 828 of this document: sstc-saml-metadata-errata-2.0-wd-04-diff.pdf )

The use-case is that some IdP's require WantAssertionsSigned="true" in the metadata that you send them.

Please include tests. Doing so will ensure that the changes made in this PR are not undone or otherwise corrupted by future changes.

Checklist:

• Issue Addressed: [x]
• Link to SAML spec: [x]
• Tests included? [x]
• Documentation updated? [x]

[cjbarth]

A small change needed for the readme, otherwise this looks good. Thank you for your contribution!

[srd90]

Prior to this pull request passport-saml's certificate check seems to work like this:

1. If cert configuration option is not present at all response's and assertions' signature are not checked at all (and message can be altered in any way before submitted to passport-saml's assertion consuming service endpoint)
2. If cert contains certificate or array of certificates (i.e. value of cert is truthy) passport-saml checks whether response has been signed:
   

   passport-saml/src/passport-saml/saml.ts

   Lines 849 to 852 in 54704de

   	let validSignature = false;
   	if (this.options.cert && this.validateSignature(xml, doc.documentElement, certs!)) {
   	validSignature = true;
   	}

   
   and sets validSignature = true if reponse's signature was valid.
   If response's signature was valid passport-saml skips assertion's signature:
   

   passport-saml/src/passport-saml/saml.ts

   Lines 870 to 877 in 54704de

   	if (
   	this.options.cert &&
   	!validSignature &&
   	!this.validateSignature(xml, assertions[0], certs!)
   	) {
   	throw new Error("Invalid signature");
   	}
   	return this.processValidlySignedAssertionAsync(

   
   and
   

   passport-saml/src/passport-saml/saml.ts

   Lines 902 to 909 in 54704de

   	if (
   	this.options.cert &&
   	!validSignature &&
   	!this.validateSignature(decryptedXml, decryptedAssertions[0], certs!)
   	)
   	throw new Error("Invalid signature from encrypted assertion");
   	return await this.processValidlySignedAssertionAsync(

   
   i.e. passport-saml is happy if response's signature was valid and it does not try to validate assertion's signature at all.

IMHO WantAssertionsSigned = true at SAML SP metadata means what referenced spec says:

Note that an enclosing signature at the SAML binding or protocol layer does not suffice to meet this requirement, for example signing a <samlp:Response> containing the assertion(s) or a TLS connection.

i.e. that even if response was signed and even if response's signature was valid SP expects that assertion is also signed and that SP shall verify assertion's signature before consuming it.

It looks like this pull request adds the possibility to setWantAssertionsSigned = true to medata generated by passport-saml just to satisfy some IdP administrator(s) requirements that SP connecting to their IdP must validate signatures of assertions or not use their IdP at all. I.e. passport-saml's internal implementation of WantAssertionsSigned = true case is missing and it - passport-saml - would not work the way its metadata implicates.

[cjbarth]

@srd90 , you make a good point. So it seems then for this PR to be truly compliant, it should actually perform more validations instead of just saying it does. @HendrikJan , are you able to add this additional validation. @srd90 any ideas how we might test that we performed the check?

[HendrikJan]

@srd90 , that makes a lot of sense. Thank you.
@cjbarth. I will look into this later this week or in the weekend. With help of the comments already given by @srd90 I may be able to pull this off.

[HendrikJan]

I've updated the pieces of code where @srd90 noticed the assertions' validation being skipped.

Testing with SimpleSamlPHP confirms that we now get the error message "Invalid signature" when the IdP does not sign the assertions.

I did not yet think about how to write a test for this.

Issue Addressed: [x]
Link to SAML spec: [x]
Tests included? [ ]
Documentation updated? [x]

[srd90]

There seems to be at least following scenarios which should be covered with new tests when passport-saml stack is configured with WantAssertionsSigned = true:

1. signed response with unsigned unencrypted assertion
   • expected result is invalid signature
2. signed response with unsigned encrypted assertion
   • because program logic ("if" statement which determines when assetion signature should be checked) is and has been duplicated to code blocks which handle encrypted and unencrypted assertions
   • expected result is invalid signature
3. unsigned response with signed unencrypted assertion and cert option not provided at all
   • because passport-saml can be configured without cert by not defining that configuration attribute at all (see issues 180 and 523) in which case passport-saml shall allow anyone to manipulate login response. This would be against WantAssertionsSigned = true and passport-saml should assume that desired behaviour is validly signed assertion (i.e. if passport-saml implementation is not modified to fail during stack setup phase due conflicting configuration parameters assertion validation should fail at least during validation of signature by XML signature library due undefined cert)
   • expected result is invalid signature
4. unsigned response with with signed encrypted assertion and certoption not provided at all (see 3.)
   • reason behind this is duplicated program logic (aforementioned "if" statement)
   • expected result invalid signature
5. signed response with signed unencrypted assertion so that assertions content (e.g. nameid) is modified after calculation of assertion's signature but prior to signing response (i.e. response's signature is valid but assertion's signature would not be)
   • expected result invalid signature
6. signed response with signed encrypted assertion so that assertion's content doesn't match with assertion's signature
   • i.e. same as 5. but for encrypted assertion (e.g. by modifying assertion's nameid after assertion's signature is calculated but prior to encrypting assertion and signing response)
   • expected result invalid signature

[HendrikJan]

@srd90 thanks for thinking about all those scenarios.
I'll try to get at least some of this to work this weekend.

[HendrikJan]

@cjbarth and @srd90

I've added test 1 and 3 and an extra test to catch a missing cert when initializing saml.

Now this branch cannot merge anymore. How should I proceed to fix the merge-conflict?

• Can I merge with the current master and fix conflicts
• Or should I rebase (I tried that, but I'm not experience with rebasing and after the rebase I got the error "Failed at the passport-saml@2.0.5 lint script." and I don't know how to fix this.)

(The rebased branch can be found here: https://github.com/HendrikJan/passport-saml/tree/wantassertionssigned_rebase)

[HendrikJan]

@cjbarth and @srd90

I've updated the test.

In the following piece of code:

  // Check if this document has a valid top-level signature
  let validSignature = false;
  if (this.options.cert && this.validateSignature(xml, doc.documentElement, certs!)) {
    validSignature = true;
  }

The line validSignature = true; is now run for every test and this seems correct to me because all tests have a signed root which should be valid.

Also all tests have amountOfSignaturesChecks = 2. One check is for the root and one for the assertion which should always be checked if wantAssertionsSigned = true

[cjbarth]

@HendrikJan This looks pretty good to me. I have a PR, #548 , that I'm eger to land first since it addresses a security concern, and would actually force changes in this PR. I wouldn't mind helping with merge conflicts if that is needed, but I'll hold off on approving this until I can manage to get the unexpectedly complicated #548 landed.

I'm also interested to see if @srd90 has anything to say about your latest changes or what is in #548 which addresses the concerns that you and he raised in #523.

[cjbarth]

@HendrikJan Would you like to do your resolve your merge conflicts so we can get this landed as part of the upcoming 3.x release? If you need help, please let me know.

[HendrikJan]

@cjbarth I'll have a look this weekend.

[HendrikJan]

@cjbarth I've resolved the merge conflict.

[cjbarth]

Some minor things, but the overall code looks good. Thanks for be patient and cooperative with getting this landed.