consider upgrading passport-saml to 3.x

[christian-hawk]

Passport SAML has been updated to 3.x.

Changelog is not clear about the current breaking changes, opened an issue about it: node-saml/passport-saml#597

There is already a PR ( node-saml/passport-saml#605 ) to fix changelog, currently offering the following changes: https://github.com/node-saml/passport-saml/blob/changelog/CHANGELOG.md

On debendabot automatic PR we can see that tests break because cert now is required. This is also not specified in their changelog.

My suggestion is lower priority in this till breaking changes are clear.

[christian-hawk]

We need to understand more about the major change "node-saml separation" and its impact

[srd90]

@christian-hawk you wrote that cert requirement is not specified in changelog.

Fixed pasport-saml 3.0.0 changelog (node-saml/passport-saml#605) says:

• Require cert for every strategy #548

Which is related to issue node-saml/passport-saml#523 (see also node-saml/passport-saml#523 (comment))

BTW. I hope that https://gluu.org/docs/gluu-server/4.2/authn-guide/inbound-saml-passport/ page's picture https://gluu.org/docs/gluu-server/4.2/img/user-authn/passport/saml_provider.png does not implicate that administrator could remove cert property (see that X at the end of cert line). If he/she can remove that property I hope that gluu-passport does not initialize instance of passport-saml so that configuration properties provided to passport-saml's strategy does not contain cert property at all because in that case anyone could intercept - due front-channel communication - and modify login response's assertion content and instance of passport-saml would consume those modified values due lack of signature checking.

[christian-hawk]

@srd90 thanks so much for the contribution.

Which is related to issue node-saml/passport-saml#523 (see also node-saml/passport-saml#523 (comment))

thanks for that mate.

BTW. I hope that https://gluu.org/docs/gluu-server/4.2/authn-guide/inbound-saml-passport/ page's picture https://gluu.org/docs/gluu-server/4.2/img/user-authn/passport/saml_provider.png does not implicate that administrator could remove cert property (see that X at the end of cert line). If he/she can remove that property I hope that gluu-passport does not initialize instance of passport-saml so that configuration properties provided to passport-saml's strategy does not contain cert property at all because in that case anyone could intercept - due front-channel communication - and modify login response's assertion content and instance of passport-saml would consume those modified values due lack of signature checking.

What you mean here is that you hope we keep cert a required field?

Your contribution is really appreciated @srd90 .

[srd90]

What you mean here is that you hope we keep cert a required field?

@christian-hawk I don't know whether it was or wasn't already required field (i.e. whether Gluu would refuse to accept configuration without cert).

UI screenshot https://gluu.org/docs/gluu-server/4.2/img/user-authn/passport/saml_provider.png gives an impression that one could remove cert by clicking X at the end of cert parameter line.

If removal of cert parameter is possible in the UI by clicking X and if Gluu accepts passport-saml configuration without cert property and if passport-saml version is < 3.0.0 and if this particular line of code

gluu-passport/server/providers.js

Line 119 in c2bc8aa

const samlStrategy = new Strategy(providerOptions, verify)

ends up being executed with providerOptions value which does not contain field cert, e.g.:

const samlStrategy = new Strategy({
  entryPoint: 'https://idp.example.com/idp/profile/SAML2/POST/SSO',
  identifierFormat: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient',
  authnRequestBinding: 'HTTP-POST',
  issuer: 'urn:test:example'
  // Notice absence of cert property
}, verify)

then there would be major security issue because end users could intercept SAML login response and modify SAML login response assertions (i.e. impersonate anyone) because passport-saml versions < 3.0.0 would just skip SAML login response signature checking (due lack of cert information) and accept anything (including modified login responses).

Disclaimer:
I have not tried Gluu server. I don't know what actually happens in gluu-passport if one would try to save passport-saml configuration so that cert attribute is completely removed. Gluu's UI screenshot just gave an idea that removal of cert might be possible.

[christian-hawk]

@srd90 thanks for providing such detailed information. Sorry I didn't checked it out yet, but we will soon, as we need to upgrade as passport-saml is about to release important patch related to xmldom

then there would be major security issue because end users could intercept SAML login response and modify SAML login response assertions (i.e. impersonate anyone) because passport-saml versions < 3.0.0 would just skip SAML login response signature checking (due lack of cert information) and accept anything (including modified login responses).

I may be wrong but as far as I know saml specs allow trust relations without signing responses, but I agree that I would never want to accept unsigned responses.

[srd90]

I may be wrong but as far as I know saml specs allow trust relations without signing responses, but I agree that I would never want to accept unsigned responses.

@christian-hawk maybe I wasn't clear enough because it is not just response level signature checking which is disabled under the conditions communicated above. Assertion level signature checking is also disabled. I.e. in the big picture ”response signature” checking is completely disabled.

Did you read node-saml/passport-saml#523 ?

Let me highlight few points.
Quote from e.g.: https://shibboleth.atlassian.net/wiki/spaces/IDP4/pages/1265631694/SAML2SSOConfiguration

If you encounter a relying party that accepts an unsigned response and assertion that is transmitted via POST (and not artifact), you have identified an insecure implementation and should report the issue immediately while following your local security incident response process.

If passport-saml version < 3.0.0 is configured without cert (without cert option key at all in options object as showed in issue node-saml/passport-saml#523 ) it (passport-saml version < 3.0.0) disables both (response AND assertion) signature checkings (normally - if cert is present and contains IdP's certificate - passport-saml is happy if at least one of those (resonse or assertion level signature) is present and valid). Result - in case of totally missing cert config parameter - is that SW which instantiates passport-saml version < 3.0.0 without cert configuration key is that it accepts any assertions end user chooses to send to it (e.g. by intercepting with normal web bewser developer tools the response from IdP and modifying authnresponse content). I.e. SW/service which has configured passport-saml version < 3.0.0 without cert config key would be ”insecure implementation”.

gluu-passport uses (at the moment) passport-saml version < 3.0.0 and furthermore gluu's admin UI screenshots implicates that one might be able to remove cert completely.

btw. I tried to install gluu stack to check how it behaves when one clicks X at cert line at admin UI but it - installation - turned out to be too timeconsuming stuff….maybe I should give it another try. I assumed back in June 6th that gluu developers have stack up and running and it would take just few minutes to try whether embedded passport-saml version < 3.0.0 ends up being configured without cert key if gluu's end user has clicked X at the end of cert option line at admin UI and saved that configuration.

[christian-hawk]

@srd90 thanks again for your rich collaboration on this issue.

I haven't read node-saml/passport-saml#523 , thanks for that. Also found node-saml/passport-saml#547 till now.

gluu-passport uses (at the moment) passport-saml version < 3.0.0 and furthermore gluu's admin UI screenshots implicates that one might be able to remove cert completely.

Yes right now passport depends on gluu's ´UMA´ server to fetch it's own configuration, including providers, and frontend is also not using cert (or any) as a required field. Team is focusing in 5.x , but with the information brought by you I understand we should increase this issue's priority, even to upgrade passport to latest, or to make cert validate cert required.

We can start patching passport independently of oxtrust .

btw. I tried to install gluu stack to check how it behaves when one clicks X at cert line at admin UI but it - installation - turned out to be too timeconsuming stuff….maybe I should give it another try

It's usually simple, you can sudo snap install gluu-server or follow one of the instructions provided here. It should be done in around 10-20 minutes depending on your configuration.

[christian-hawk]

@kdhttps I also asked to run blackbox tests on passport 3.x patch by dependably #332 so created artifacts may help

[christian-hawk]

closed by #332
thanks all for contrib