Updated fork status into Readme.md #37
Conversation
RopoMen
force-pushed
the
update-readme-fork-status-update
branch
from
98d8381
to
983f690
Compare
|
|
||
| Then you should be able to start testing upstream version of `@node-saml/passport-saml` with public key support. Public key can be provided through `cert` configuration property in PEM format. | ||
| - Hardening `Enforce audience checking`: Provide `audience` configuration parameter. | ||
| - Hardening `Enforce validteInResponseTo checking`: Set `validateInResponseTo: true` in configuration parameters. |
There was a problem hiding this comment.
@node-saml/passport-saml and @node-saml/node-saml versions >= 4.x (currently latest published vesion) has a bug which disables in responso to validation if configuration value is set to true (or anything else except one of the enum values introduced at PR node-saml/node-saml#40 ). See following bug report for further information:
Upcoming @node-saml/node-saml (5.x) has this fix node-saml/node-saml#314 which would throw exception if value is not one of the enum values. You might have pinned to some version prior to that bug fix because otherwise you would have encountered exception with value true.
There was a problem hiding this comment.
@srd90 Thank your for addressing this issue!
| - Hardening `Enforce audience checking`: Provide `audience` configuration parameter. | ||
| - Hardening `Enforce validteInResponseTo checking`: Set `validateInResponseTo: true` in configuration parameters. | ||
| - Hardening `Enforce assertion's signature validation`: Set `wantAssertionsSigned: true` in configuration parameters. | ||
| - Hardening `Enforce (by default) encrypted assertion(s) only policy`: Set `wantAssertionsSigned: true` **and** provide private key using configuration parameter `decryptionPvk`. |
There was a problem hiding this comment.
@node-saml/node-saml doesn't have such feature (blocking unencrypted assertions). What you are describing above (setting of decryptionPvk) is a configuration to enable handling of encrypted assertions (shortened version what README.md already says) BUT it doesn't block unencrypted assertions. I.e. if some IdP or other entity ends up posting authn response which has plain text assertion and if that authn reponse passes other checks then @node-saml/passport-saml / @node-saml/node-saml consumes plain text assertion from that response. wantAssertionsSigned: true just means that encryped or plain text assertion must be validly signed.
There was a problem hiding this comment.
@srd90 Thank your for addressing this issue too! Is it too much to asked if you could open up an issue to fix this in @node-saml/node-saml and describe what needs to be done and why?
Conversation about the solution must be opened with the repository maintainer so we can get consensus about the solution before making implementation with PR.
Sometimes PR process on upstream takes extremely long time to handle and thereof I want that solution is clear on both sides.
We definitely want these security hardenings into upstream library, maintaining this fork is not what we want todo.
There was a problem hiding this comment.
...Is it too much to asked if you could open up an issue to fix this in @node-saml/node-saml and describe what needs to be done and why?
@RopoMen Well actually it would be (too much to asked) :)
I don't use any variant of passport-saml or node-saml so I don't have any incintive to do so. I'm just checking out of curiosity every now and then whats happening at passport-saml / node-saml.
I spotted these changes when I checked out of curiosity whats happening at the passport-saml / node-saml forks. Things that were written in this PR did not match with reality so I wrote few comments. About "why" part of your comment: I guess that your business case is that you want to ignore plain text assertions because your IdP is configured to encrypt assertions sent to your SP and plain text assertions would mean that source of authn response is not your IdP and you don't want to even try to process such authn response.
Conversation about the solution must be opened with the repository maintainer so we can get consensus about the solution before making implementation with PR.
I don't understand what there is to conversate about. Some "blockUnencryptedAssertions" option is quite self explaining when combined with some business case description. Dunno if this would be niche option because signature(s) validation combined with recipient, audience, inresponseto and notonorafter checks should block possible replayed authn responses (responses targeted to some other SP even if that SP uses same IdP because at least recipient address should be different). Having said that at least this SAML SP stack seems to have similar feature:
https://github.com/Clever/saml2/blob/0f40b833f55ca227702f864eb17947dd5b78c4be/README.md?plain=1#L58
Sometimes PR process on upstream takes extremely long time to handle and thereof I want that solution is clear on both sides.
At the moment (and for quite some time now) there is/has been only one active passport-saml / node-saml / xml-crypto maintainer and I bet that the maintainer has some other projects / life also.
We definitely want these security hardenings into upstream library, maintaining this fork is not what we want todo.
Maintainer of passport-saml / node-saml has signaled that there is going to be new major version upgrade at near future. If you want to have this feature at upstream consider introducing PR ASAP.
If you are after as hardened as possible passport-saml / node-saml be aware that it doesn't implement mandatory recipient validation (see node-saml#509 and node-saml/node-saml#335 )
No description provided.