Feature: add facility in config to add <Extensions> element in SAML request

[kdhttps]

Issue

close node-saml/passport-saml#602
Linked PR: node-saml/passport-saml#607

Description

Add support in passport-saml where it will allow admin to configure fully customise <Extensions> element and add it into SAML Request.

<Extensions> element are use to request custom attributes and setting. Its structure and values different as as IDP configurations and requests.

Use cases

1. Use <Extensions> element to request attributes/data. More details, oasis-open.org SAML V2.0 Protocol Extension for Requesting Attributes per Request
2. Use <Extensions> element to request with some custom config. For Example: In suomi.fi idp, its allows SP to request with custom language code by passing it <Extensions> element. More details

Changes

• src/node-saml/types.ts : added extensions optional variable. The type is any and it accept XMLBuilder type values because as per use cases and IDPs', there are many cases and each use cases have different request format/data. Please let me know what is your view on this.
• src/node-saml/saml.ts: added it in request. As it has already XMLBuilder values so no need to do any processing.
• Test cases added
• Docs updated
• No Breaking Changes

Checklist:

• Issue Addressed
• Link to SAML spec
• Tests included
• Documentation updated

[kdhttps]

I think @cjbarth we can start discussion from here node-saml/passport-saml#607 (comment)

[cjbarth]

What do you think about taking the object as JSON or XML literal and then, in the ctor, making sure it can be converted to an XMLBuilder object for internal usage? That would allow a user to pass in what they like, but it means that we get strong typing and can validate that the XML structure is correct and throw an error while constructing if they passed in something bad.

[kdhttps]

Thank you @cjbarth for quick response. Currently user can pass json which is directly accepted by XMLBuilder

samlExtensions: {
  "md:RequestedAttribute": {
    "@isRequired": "true",
    "@Name": "Lastname",
  },
  vetuma: {
    "@xmlns": "urn:vetuma:SAML:2.0:extensions",
    LG: {
      "#text": "sv",
    },
  },
},

So could you please describe, instead of above one what json/xml format you like to pass?

samlExtensions: {
     ????
}

[cjbarth]

I'm not suggesting a different format. I'm suggesting that you adjust the ctor so that we validate the incoming data and throw if it doesn't conform to some basic checks. I'd really prefer not to blindly take whatever is put there and just add it to the outgoing SAML document.

[srd90]

If I remember correctly extensions type in schema is ”sequence of any” so there isn’t much to check except that provided xml elements are valid.

When those two example xml elements provided by @kdhttps would be added/injected to authn request generated by node-saml resulting authn request saml protocol message would not be valid because value for md namespace prefix is not defined (side note: it refers propably to urn:oasis:names:tc:SAML:2.0:metadata and RequestedAttribute sample is propably copied from some SAML Service metadata example).

node-saml module could for example validate generated authnrequest XML message against SAML Schemas just before it is delivered to IdP. This way erronous/non-valid XML issues would be more easily detected by application developers at application side instead of receiving some cryptic error message from IdP.

In fact validation of SAML messages against SAML schema is proposed by OWASP (although following cheat sheet seems to be written from receiver point of view): https://cheatsheetseries.owasp.org/cheatsheets/SAML_Security_Cheat_Sheet.html

FWIW: Here is also example of European Union’s eIDAS specification which also uses extensions (in service metadata and authn request):
https://ec.europa.eu/cefdigital/wiki/display/CEFDIGITAL/eIDAS+eID+Profile
and
eIDAS SAML Message Format
https://ec.europa.eu/cefdigital/wiki/download/attachments/82773108/eIDAS%20SAML%20Message%20Format%20v.1.2%20Final.pdf?version=3&modificationDate=1571068651727&api=v2
see chapter 6 from pdf to concrete examples.

BTW. @kdhttps if I interpreted current content of this PR correctly samlExtensions are fixed during SAML object creation (i.e. same values are used for each authnrequest). If I have understood correctly values at authn request’s extensions could change per authnrequest (e.g. vetuma language which is used to signal end user’s preferred language to be used at IdP side UI).

[kdhttps]

BTW. @kdhttps if I interpreted current content of this PR correctly samlExtensions are fixed during SAML object creation (i.e. same values are used for each authnrequest). If I have understood correctly values at authn request’s extensions could change per authnrequest (e.g. vetuma language which is used to signal end user’s preferred language to be used at IdP side UI).

No sure Mate, Currently I have a requirement to keep it fixed. But I can see in suomi.fi docs that there are some other ways also to pass language element.

About schema validation, I think we don't have a fixed schema to validate it with SAML Request XML, as you can see there are many saml extensions possibilities. I have gone through the owasp.org saml secuirty cheat sheet docs, it is very generic and mostly about TLS and HTTPs network security. There is some point about SAML Schema validation but they didn't provide any schema, it is up to us to make the first schema and validate it with SAML Request XML but again the same situation many saml extensions possibilities.

So I guess, the application developer has to manually verify saml request. Please let me know if I missed something and Let me know your suggestions.

Thank you, Team!

[srd90]

@kdhttps as I said there isn’t any schema which could be used to validate content which is put to extensions block

If I remember correctly extensions type in schema is ”sequence of any” so there isn’t much to check except that provided xml elements are valid

I should have used word well-formed instead of valid in that particular paragraph.

On third paragraph my proposal was to use validating XML parser to validate e.g. produced authnrequest. This would implicitly check that extensions element doesn’t contain something that would not pass authnrequest validation from XML document validation point of view (for example elements which use unbound namespace prefixes or malformed xml which are pretty much only validations that could be done to content of extensions element because it - the content of extensions - can be sequence of anything).

SAML protocol schema is available e.g. from here https://docs.oasis-open.org/security/saml/v2.0/saml-schema-protocol-2.0.xsd

About enabling different extensions element content per request. IMHO if authnrequest extensions content is meant to be used to provide different content per authnrequest then node-saml library should enable it instead of forcing developers to instantiate new SAML object per request (of course implementation could merge some values from stack configuration options but it should also allow real per request content). Not directly related to this case but passport-saml has already one ”half baked” per request thing see node-saml/passport-saml#541 (comment)

btw. you can use e.g. this https://www.samltool.com/validate_xml.php to debug authnrequests produced by node-saml. It shall produce error if extensions contains elements which namespace prefix is unbound / undefined (I am referring to this pull request’s README.md content and specifically to md:RequestedAttribute stuff at that example).

i.e. Validating Authnrequest with this extensions content

  <md:RequestedAttribute isRequired="true" Name="Lastname" />
  <vetuma xmlns="urn:vetuma:SAML:2.0:extensions">
    <LG>sv</LG>
  </vetuma>

Produces

Namespace prefix md on RequestedAttribute is not defined

In this particular case content should be

  <md:RequestedAttribute xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata"  isRequired="true" Name="Lastname" />
  <vetuma xmlns="urn:vetuma:SAML:2.0:extensions">
    <LG>sv</LG>
  </vetuma>

[kdhttps]

SAML protocol schema is available e.g. from here https://docs.oasis-open.org/security/saml/v2.0/saml-schema-protocol-2.0.xsd

About, XML Schema validation, I found libxmljs which provide facility to validate xml again schema. I need your suggestion on this lib. May I use it for validation?

I can see, node-saml is using xml2js, xmlbuilder and xmldom but no one have validation feature. Iet me if you have any other option or use libxmljs??

Thank you!

[srd90]

I need your suggestion on this lib. May I use it for validation?

@kdhttps just in case this question was directed for me: if node ecosystem does not have native javascript xml library which understand xml schemas I would not introduce yet another xml library dependency (which needs native library). I would just let developers to make sure that they don't mess up with extensions and if they do then they just have to communicate with their IdP provider whats wrong with authnrequests syntax.

For the record: I am not using passport-saml or node-saml and not going to use these. Checking out passport-saml every now and then is more like a hobby. So you should wait @cjbarth and @markstos etc. comments before proceeding with those content checks requested by @cjbarth

[kdhttps]

For the record: I am not using passport-saml or node-saml and not going to use these. Checking out passport-saml every now and then is more like a hobby.

Great 💯 @srd90

So you should wait @cjbarth and @markstos etc. comments before proceeding with those content checks requested by @cjbarth

Sure, let's wait!!!

[cjbarth]

I'm not a big fan of using libraries that require node-gyp; I'd rather see pure JavaScript solutions. When I'm talking about validation, I'm looking for two things, primarily:

1. A value that is passed in, which node-saml can't accept, should return a clear error.
2. A value which is known to be wrong, or at least obviously malicious, shouldn't be accepted; see rule 1.

I understand that rule 2 would be a little hard to determine without validating against some schema. I'm not opposed to a schema validation, in fact, I think that would yield some real long-term benefits to the system. However, I feel like there is an opportunity for some low-hanging fruit here, short of such a full validation system. If we simply check the incoming <extension> payload to make sure it is valid XML and that it contains the basic structure (test a few keys and attributes), I think we will hit a minimum acceptable level of fitness for use.

[markstos]

Thanks for working on this @kdhttps. I just had a customer ask if we supported this feature as well, so I'm also interested to see this get merged.

[kdhttps]

@cjbarth

About,

1. A value that is passed in, which node-saml can't accept, should return a clear error.

After some research I found that we can add one validation where element must have namespace like @srd90 maintained here. I am not finding any other validation check for <extensions> element.

About,

2. A value which is known to be wrong, or at least obviously malicious, shouldn't be accepted; see rule 1.

I found this XML Security Cheet sheet. Most problem should be handle by parser(at IDP side) and we are not parsing xml. we are just taking JSON from user and passes it to XMLBuilder to make XML request. But we need to restrict malicious data like this. To restrict this, I didn't add any code as It is already handled by xmlbuilder npm. In fact, it is apply many validations like this examples.

Please review code and let me know if you want me to add/change anything.

Thank you!!!

[kdhttps]

@srd90 @cjbarth added samlLogoutRequestExtensions feature, please review

[cjbarth]

Why do you add the empty value here and then delete it later when not needed? The pattern you used previously just adds the value if you need it and skips the extra logic to delete if not needed.

[kdhttps]

it is because strick type of NameID here

node-saml/src/types.ts

Line 60 in 71d9fe5

"saml:NameID": XMLInput;

and samlp:Extensions element should be placed before NameID. There is other way also but I choose and Like to keep this one for safe type.

[kdhttps]

@cjbarth once you get time, please review it

[cjbarth]

@kdhttps Can you please clean up the merge conflicts before I review?

[kdhttps]

@cjbarth done, please review it again

[cjbarth]

Looking forward to the follow-on PR with the additional feature :)