search

italia / spid-sp-test

SAML2 SPID/CIE Service Provider validation tool
European Union Public License 1.2
39 stars 17 forks source link

Regression in authentication level validation #124

Closed mauromol closed 2 years ago

mauromol commented 2 years ago

111 introduced a regression.

If my SP sends an AuthnRequest with this:

<samlp:RequestedAuthnContext Comparison="exact">
  <saml:AuthnContextClassRef>https://www.spid.gov.it/SpidL1</saml:AuthnContextClassRef>
</samlp:RequestedAuthnContext>

and the IdP response states that the authentication has been established with L2, the SP must consider the response valid, even if SAML 2.0 says otherwise. This is what SPID specification says in 1.2.2.1 section:

N.B. L’Identity Provider ha facoltà di utilizzare per l’autenticazione un livello SPID più alto rispetto a quelli risultanti dall’indicazione del richiedente mediante l’attributo Comparison. Tale scelta non deve comportare un esito negativo della richiesta.

So, currently tests 95 and 96 are failing for me with current spid-sp-test for this reason, in the above scenario.

ewedlund commented 2 years ago

Would it not be more correct tu use Comparison="minimum" in this case?

peppelinux commented 2 years ago

I am available to acquire your proposal for improvement and join a Pull Request that manages both minimum and exact. This project is community-driven, feel free to submit your proposal

mauromol commented 2 years ago

Would it not be more correct tu use Comparison="minimum" in this case?

From a SAML 2.0 yes, it should be more correct, but the requirement from SPID specification is quite explicit: the SP should not return an error if a higher authentication level is returned, whatever the Comparison attribute value is. Instead, current spid-sp-test is requiring a non-compliant behaviour.

I am available to acquire your proposal for improvement and join a Pull Request that manages both minimum and exact. This project is community-driven, feel free to submit your proposal

I've not looked at it thoroughly, but my suspect is that it's just a matter of reverting changes from #111, because the SP should behave just like if "minimum" were specified, whatever other Comparison value was specified in the AuthRequest.

ewedlund commented 2 years ago

I went back to the documentation and it is quite confusing, because it states:

L’elemento <RequestedAuthnContext> prevede un attributo Comparison con il quale indicare il metodo per stabilire il rispetto del vincolo sul contesto di abilitazione: i valori ammessi per questo attributo sono:

Nel caso dell’elemento <RequestedAuthnContext>, questa informazione si riflette sulle tipologie di meccanismi utilizzabili dall’Identity Provider ai fini dell’autenticazione dell’utente.

The text cited by you is placed underneath an example where comparison="exact", which makes it a bit unclear whether it applies to the specific example or in general. I think i would be absurd applied in general because then it would be more correct to say that the Comparison attribute should always be minimum. Is there any way to get a clarification on this?

peppelinux commented 2 years ago

@damikael ^

mauromol commented 2 years ago

Nel caso dell’elemento <RequestedAuthnContext>, questa informazione si riflette sulle tipologie di meccanismi utilizzabili dall’Identity Provider ai fini dell’autenticazione dell’utente.

From this I understand that the Comparison is just a hint for the IdP to make a choice about the authentification level, however it's free to choose a higher one in any case.

I remember someone told me Poste Italiane only supports L2 (and not L1): if so, any request with exact/L1 should fail with that IdP if strict SAML policy were applied. I expect it to just respond with an L2 authentication and the SP should accept it, IMHO.

damikael commented 2 years ago

What @mauromol said is right, IdP can upgrade the level and SP MUST accept the response when authentication is performed at higher level, even if the effective AuthenticationContextClassRef of that response is different from the AuthenticationContextClassRef requested within the AuthenticationRequest. For this reason, to avoid ambiguity, it's advisable to specify comparison value as "minimum".

mauromol commented 2 years ago

So, is current spid-saml-check using a version of spid-sp-test with this regression? Because, of course, it must be taken into account by whoever needs to start the onboarding process (like me ;-)). Thanks.

peppelinux commented 2 years ago

Ok guys, that's all so interesting and thank you for all this anyway, what to do with this issue?

Do we have developed some preferences, we leave it like this, we bring it back, tell me, come on

By me I prefer minimum because it's more flexible, exact could be somethign cktitical sp side. Anyway, do we have to close this or do we need to explore this issue one more?

mauromol commented 2 years ago

IMHO, spid-sp-test should test SP compliance to SPID requirements. For this reason:

ewedlund commented 2 years ago

IMHO, spid-sp-test should test SP compliance to SPID requirements. For this reason:

  • if the SP specified Comparison="exact", SAML says the returned level should be the same, but SPID says it may be higher, so spid-sp-test should test that the SP accepts either the same level or any higher one
  • if the SP specified Comparison="minimum", SAML and SPID agree, so spid-sp-test should test that the SP accepts either the same level or any higher one (that is: same behaviour as for "exact")
  • if the SP specified Comparison="better", SAML says the returned level should be higher and the SPID requirement doesn't add anything to that, so spid-sp-test should test that the SP accepts any higher level, but not the one in the request or any lower one (of course Comparison="better" with a requested level of L3 cannot be satisfied, so such a request should be considered invalid)
  • if the SP specified Comparison="maximum", SAML says the returned level should not be higher than the requested one, but SPID says it may still be higher, so IMHO spid-sp-test should test that the SP accepts ANY level in the response

At this point I agree, since, as @damikael has confirmed, this is indeed what the SPID documentations says.

peppelinux commented 2 years ago

Dear friends, first of all thank you all for the precious analisys :heart:

Here the path to get how we implemented in spid-sp-test the response mods and how the ACR comparison has been made, and how to improve it.

First, we have the response settings. These can load custom plugins (response mods) and this is how we get the ACRs, here: https://github.com/italia/spid-sp-test/blob/main/src/spid_sp_test/responses/settings.py#L867

The namespace that's to the python package points here: https://github.com/italia/spid-sp-test/blob/main/src/spid_sp_test/responses/response_mods.py

The desidered behaviour can be implemented following the approach we already have at line 15 or any other as your taste.

get_acr and get_acr_comparison are methods of the class SpidResponeCheck, here: https://github.com/italia/spid-sp-test/blob/main/src/spid_sp_test/response.py#L154

I'm not having time to follow this feature/fix but I can get in pair with anyone of you to get it done in a useful time. @nunzionapoli is working of v1.2.0 that has the fully support for Aggregatori Privati, if you like to join in this milestone let me know.

See you on slack