Improving the code of node-saml

[forty]

I might have some time later this year (not August for sure) to work on this project a bit.

Question: what's the status of the node-saml separation? I See that on master the module has been moved to another repo, but no passport-saml version has been published with the separation I believe. Where should node-saml improvements be done? In the 3.x branch?

Thanks!

[markstos]

The separate has happened on the master branch, you can see the dependency in package.json.

node-saml changes should be done in the node-saml repo: https://github.com/node-saml/node-saml

[cjbarth]

We are trying to finish up some breaking changes before we release cut a 4.0.0 release for both node-saml and passport-saml. If there are some changes that you see coming that would be breaking changes, please bring them up so we can plan to get as many of the breaking changes to land in 4.0.0 as possible.

[forty]

@cjbarth I mostly had in mind internal changes which I think can be made without breaking things.

Things I think would be nice to change during a breaking change:

• stop defaulting to sha1 (sha256 is a better default)
• remove insecure options (I only have the option to allow unsigned assertions, which cannot be a good idea and made me slightly nervous when reviewing the code - but I guess some people have a use case for that?)

[srd90]

... I only have the option to allow unsigned assertions, which cannot be a good idea and made me slightly nervous when reviewing the code ...

@forty passport-saml has somewhat sad history regarding signature checking. passport-saml (at least up to 3.x) was happy if saml response was signed OR if assertion was signed BUT up to 3.x neither of these were checked if IdP certificate was not provided at configuration (see #180 and #523 ).

3.x finally requires IdP certificate as mandatory configuration value. Now passport-saml considers authentication response validly signed if response level signature is valid. If it is not or if it is missing then passport-saml requires that assertion signature is valid.

Issue #533 and PR #536 (see also PR's discussions) changed situation so that even if response was signed passport-saml can be configured to require that embedded assertion is also validly signed (with WantAssertionsSigned = false passport-saml shall consider message validly signed if response level signature was valid).

Some SP implementations let developers to configure SP to require also valid response level signature ("want response signed"). From authn response security point of view response or assertion must be validly signed. All security is lost of both signature validations are skipped.

Side notes:

passport-samlversions < 3.x can still be configured without IdP certificate which opens up authentication bypass vulnerability ( see #523 and especially comment: #523 (comment) ). Download statistics of https://www.npmjs.com/package/passport-saml (see versions tab) show that versions < 3.x are still quite popular and it is safe to assume that there are quite a few services which has skipped IdP certificate configuration because it has not been required configuration value.

Regarding removing insecure options: see #137 (comment) . Some SPs default to use authnrequest issuer as default value for audience checking where-as passport-saml just skips audience checking if it is falsy or missing.

[forty]

Absolutely, mandatory audience checking if indeed another thing I noticed should be improved!

[forty]

And thanks for the explanation on WantAssertionsSigned, it makes much more sense now (having that rather complex if on this critical line of code still makes me nervous, but now I understand why it is that way ^^)

[tomgiddingsjisc]

Just to chime in, I'm able to spend some time on this also - so if there is anything I can do to help, please shout.

[markstos]

@tomgiddingsjisc Thanks! One thing you can do is to "subscribe" to the repo so you get notified of new issues. Triaging issues is helpful, gather more information, confirm if they are real issues are not. As a security-sensitive project, more peer review of PRs is always welcome. Having more than one reviewer of PRs is is quite welcome. Also, there are issues labeled "pr-welcome" if one interests you.

[tomgiddingsjisc]

Of course! I'll keep an eye and pick stuff up whenever I can.

[forty]

Some draft ideas in random order (let me know what you think):

• Updating passport-saml options to only take a SAML Object + the passport specific options. This way we can avoid (1) duplicating the documentation which would live only in node-saml (2) having passport only options in node-saml
• Improving default (no sha1, mandatory audience checking, etc)
• stop using short names for digest and signing algorithms. They have whole names, we can use them directly for better clarity (ie RSA-SHA1 != SHA1). This also avoid the ugly default to SHA1 (or other) when we don't know the short name. If people really like short names, we can still expose them as constants. Also we use the full names for xmlSignatureTransforms, so that would be more consistent.
• Stop using buffer or strings for keys and cert. Node has crypto.KeyObject, we should use that, it would avoid doing PEMing and such ourselves for example. And we could take the opportunity to check that it's the kind of key we expect (public/private/cert, rsa)
• Stop using weird/old JS construct. I don't know what other think, but I hadn't seen break labels used anywhere ever :) I don't think it's the best way for the code to be easily understood/reviewed.
• Try to never serialize anything we parse. This is what this article https://mattermost.com/blog/securing-xml-implementations-across-the-web/ calls "XML round-trip". I think we should parse XML once, then only work on the DOM, without ever re-serializing. It would protect again this class of vulns.
• Reduce the size of saml.ts. I have a few ideas, but it's not clear for now.
• Group option in a single properties when it makes sense. For example I feel "InResponseTo Validation" options should be under a common key, because if you don't do that validation, you should provide none of those options. Same for single logout, etc
• Remove the options for callbackUrl. I'm not sure it makes sense to have those host/path/protocol options. I'm not sure who would want to use the default, and the default to http (no s) for example is not great. I feel having callbackUrl mandatory is simpler and clearer-
• Have disableRequestedAuthnContext true by default (I'm not sure what's the purpose of RAC to be honest, but it's certainly annoying with many IDP - personally I feel the type of auth should be controlled from the IDP, but there might be reasons to do differently)

Those are just ideas, let me know what you think

[forty]

Note that many of those are breaking changes actually :)

[cjbarth]

@tomgiddingsjisc we are welcoming breaking changes right now as we prepare the 4.0 release. Feel free to select something and get a PR up. Right now we are focusing on getting #619 landed, so reviews and comments on that would be helpful. After that lands, we'll get through the rest of the PR backlog.

[forty]

@cjbarth what do you think of this item:

Updating passport-saml options to only take a SAML Object + the passport specific options. This way we can avoid (1) duplicating the documentation which would live only in node-saml (2) having passport only options in node-saml

I feel that would be nice to have it before moving on with the split.

[cjbarth]

I'm on-board with that in concept. I'd love to see a quick-and-dirty draft PR put up for this so I have something more concrete to comment on.

[forty]

@cjbarth I'm discussing with my employer (which is generally open the the idea) to spend x amount of my time working on improving the project (things mentioned here #629 (comment) and this PR node-saml/node-saml#19 for example).

However I won't be able to make it alone, I'd need some of your time (and possible others') to help me merge things. Is that something you would be interested in?

[cjbarth]

I'm more than happy to review code. I like to try to get stuff merged and landed in the order the PR is posted to prevent more merge conflicts. I'd be really thankful if you're first way of helping could be to review and comment on some outstanding PRs. Recently we've been trying to increase the standard of the code by making sure that even maintainers code is reviewed.