add use IDP LOA (authnContextClassReff) in Stepup Decision#1893
add use IDP LOA (authnContextClassReff) in Stepup Decision#1893ArnoutvdKnaap wants to merge 7 commits intoOpenConext:mainfrom
Conversation
|
fixes #1883 |
|
the commit history is not very nice any more but all tests are passing now |
| } | ||
|
|
||
| #[\PHPUnit\Framework\Attributes\Test] | ||
| #[\PHPUnit\Framework\Attributes\Group('Stepup')] |
There was a problem hiding this comment.
I would personally add a few use-statements on the top of this file for these attributes and the exception below to keep the code better readable.
There was a problem hiding this comment.
I only added 1 for the exception. That is more in line with the other tests.
There was a problem hiding this comment.
Fair enough! I'm a nitpicker anyway :D
MKodde
left a comment
MKodde
left a comment
There was a problem hiding this comment.
Sorry to be so late to the party, but I wanted to give some additional feedback and ask a fundamental question.
Other than that your change is looking nice and neat, I see we merged some other features that mix with your changes, so a rebase is required before merging. I would also love to see a Behat test covering this feature. Should be doable..
| $idpResponseLoa = null; | ||
| // Determine the IdP response LoA if feature is enabled | ||
| if($application->getDiContainer()->getFeatureConfiguration()->isEnabled('eb.stepup.use_idp_response_loa')) { | ||
| $idpResponseLoa = $originalAssertions->getAuthnContextClassRef(); |
There was a problem hiding this comment.
The:
- AuthnRequest can specify multiple authnContextClassRefs. As in: one of these must be met.
- SAML Assertion can only have one authnContextClassRef. As in: this authn method was used for this authentication.
Thinking about this that makes little sense. I would expect that in the case of an IdP handled 1st and 2nd factor authentication both the first and second factor class ref be present in the Assertion. But the SAML2 lib certainly exposes only one. (\SAML2\Assertion::getAuthnContextClassRef)
The XSD for the Assertion does imply that multiple authnContextClassRef can be stated. See: https://docs.oasis-open.org/security/saml/v2.0/saml-schema-assertion-2.0.xsd
I started thinking about this after reading this: https://security.stackexchange.com/a/227882
@pmeulen @baszoetekouw Am I making things up here?
| $authnRequestLoas = $receivedRequest->getStepupObligations($this->_loaRepository->getStepUpLoas()); | ||
|
|
||
| $stepupDecision = new StepupDecision($idp, $sp, $authnRequestLoas, $pdpLoas, $this->_loaRepository, $log); | ||
| $stepupDecision = new StepupDecision($idp, $sp, $authnRequestLoas, $pdpLoas, null, $this->_loaRepository, $log); |
There was a problem hiding this comment.
Consider moving the new StepupDecision::idpResponseLoa parameter to be the last (optional) param, that way you do not need to pass a null value here. Also I expect this should be an array of AuthnContextClassRef as per my previous comment.
I also get that you might want to bundle all LoA associated params together. So feel free to leave as is.
| /** | ||
| * Check if the IDP response LoA is sufficient for the requested LoA. | ||
| */ | ||
| private function checkIDPLoaIsSufficient($isLoaAsked): bool |
There was a problem hiding this comment.
| private function checkIDPLoaIsSufficient($isLoaAsked): bool | |
| private function checkIdpLoaIsSufficient($isLoaAsked): bool |
|
I find it hard to evaluate this PR. I'm missing the design and thinking that went into it. Did I miss something? When implementing the Stepup functionality in engineblock as it is now, we deliberately chose to NOT accept the AuthnContextClassRef used by Stepup from IdP's. This PR breaks that assumption. |
The issue I was trying to solve here was a use case we have where some IDPs use stepup as their MFA token functionality and other IDPs already have their own tokens they would prefer to use. We have an SP that would sometimes like to request MFA. Using RequestedAuthnContext with a stepup defined loa will in this case always trigger stepup. Even if I pass the requested context to the IDP and it handles MFA. This means all IDPs have to have a token or I need to define a different LOA and not use stepup. Or am I missing something? |
4 participants
adds a feature flag to enable engineblock to use the AuthnContextClassReff in the response from the IDP in the stepupDecision.
This allows for IDPs to use their own MFA in combination with stepup.