Skip to content

SAML: Support for authnContextClassRef in proxy mode#833

Closed
tauceti2 wants to merge 5 commits intosimplesamlphp:masterfrom
tauceti2:authnContextClassRefForProxy
Closed

SAML: Support for authnContextClassRef in proxy mode#833
tauceti2 wants to merge 5 commits intosimplesamlphp:masterfrom
tauceti2:authnContextClassRefForProxy

Conversation

@tauceti2
Copy link
Copy Markdown
Contributor

If SSP works as a proxy, this code retain requested authnContextClassRef from
SP behind the proxy and pass it to the upper IdP. It also pass the
authnContextClassRef sent by the upper IdP back to the SP behind the proxy. By this change SPs behind the proxy can request different authn methods, especially useful for support multi-factor authn.

Please review the code, if it is acceptable also for mode when SSP doesn't work as a proxy.

@pradtke
Copy link
Copy Markdown
Contributor

pradtke commented May 30, 2018

We have some proxy use cases where this is undesirable so it would be nice if there was a way to opt in or opt out of the behavior change.

Use case is that SPs request MFA from the proxy. The proxy performs it own MFA for users and we don't want to request MFA from upstream IdPs.

@tauceti2 tauceti2 force-pushed the authnContextClassRefForProxy branch from f62fec0 to d7776db Compare June 20, 2018 13:13
@tauceti2
Copy link
Copy Markdown
Contributor Author

Hi @pradtke, added configuration option, which can enable that behaviour. By default it is off.

@tauceti2 tauceti2 force-pushed the authnContextClassRefForProxy branch 3 times, most recently from c236386 to 65049f3 Compare October 9, 2018 21:54
@tauceti2 tauceti2 force-pushed the authnContextClassRefForProxy branch 3 times, most recently from 24759d4 to b556ecf Compare November 30, 2018 13:25
If SSP works as a proxy, this code retain requested authnContextClassRef from
SP behind proxy and pass it to the upper IdP. It also pass the
authnContextClassRef sent by the upper IdP back to the SP behind the proxy.
@tauceti2 tauceti2 force-pushed the authnContextClassRefForProxy branch from b556ecf to d32de21 Compare November 30, 2018 13:26
@tvdijen tvdijen added the Proxy label Dec 12, 2018
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from 1c686ab to eb20457 Compare August 17, 2020 20:43
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 08ebb9c to 64fca25 Compare July 2, 2021 14:12
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 8, 2021

Codecov Report

Merging #833 (8ff4c21) into master (03bdf01) will increase coverage by 1.17%.
The diff coverage is 20.00%.

@@             Coverage Diff              @@
##             master     #833      +/-   ##
============================================
+ Coverage     40.39%   41.56%   +1.17%     
+ Complexity     3454     3438      -16     
============================================
  Files           142      142              
  Lines         10401    10343      -58     
============================================
+ Hits           4201     4299      +98     
+ Misses         6200     6044     -156     

Copy link
Copy Markdown

@dmotelica dmotelica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original review comment Hello, For those who use SimpleSAMLphp as a proxy to delegate auhentication to Azure AD and have to be NIH ready (Refeds MFA profile):

File SP.php - add this before line 487 :

if ($state['saml:RequestedAuthnContext']['AuthnContextClassRef'][0]=="https://refeds.org/profile/mfa"){ $state['saml:RequestedAuthnContext']['AuthnContextClassRef'][0] = 'http://schemas.microsoft.com/claims/multipleauthn'; }


file SAML2.php - add this before line 1137:

if(isset($state['saml:RequestedAuthnContext']) && $state['saml:sp:AuthnContext']=="http://schemas.microsoft.com/claims/multipleauthn") { $state['saml:sp:AuthnContext']="https://refeds.org/profile/mfa"; }


in config.php and authsources.php

'proxymode.passAuthnContextClassRef' => true,

This is way too specific for R&E federations to add to this PR.

@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 7a53fc8 to d73ae47 Compare September 26, 2021 13:03
@kikkervis
Copy link
Copy Markdown

Any idea when this will be merged? It's something we could really use in our setup.

@tvdijen
Copy link
Copy Markdown
Member

tvdijen commented Oct 14, 2021

I think we need to have a fundamental discussion on how to deal with "proxy-mode"... There are literally a ton of elements in a SAML request/response that you may or may not want to relay through the proxy and I don't like the idea of adding configuration settings for all of them.

@ghalse
Copy link
Copy Markdown
Contributor

ghalse commented Dec 3, 2021

This pull request is now referenced from the REFEDS MFA Profile implementation FAQ.

For that reason, I wanted to note that since the patch was first written there are changes to SSP that means you need to make sure you include the later merge commits (and particularly 2c6cf5a) if you're trying to use this as a patch against recent SSP.

@tvdijen tvdijen force-pushed the master branch 2 times, most recently from e5c0e21 to d5616df Compare January 9, 2022 11:00
ghalse added a commit to ghalse/simplesamlphp that referenced this pull request Oct 26, 2022
Forward port of @tauceti2 patch in simplesamlphp#833
to apply cleanly against simplesamlphp-2.0.0-rc2
ghalse added a commit to ghalse/simplesamlphp that referenced this pull request Oct 26, 2022
Forward port of @tauceti2 patch in simplesamlphp#833
to apply cleanly against simplesamlphp-2.0.0-rc2
@ghalse
Copy link
Copy Markdown
Contributor

ghalse commented Oct 26, 2022

I've forward ported this for simplesamlphp 2.0.0-rc2, and the patch is in 5544663

@tvdijen
Copy link
Copy Markdown
Member

tvdijen commented Oct 27, 2022

I can't seem to cherry-pick that commit.. It's not part of any branch.. What exactly did you do @ghalse ?

@ghalse
Copy link
Copy Markdown
Contributor

ghalse commented Oct 27, 2022

I can't seem to cherry-pick that commit.. It's not part of any branch.. What exactly did you do @ghalse ?

It's part of the authnContextClassRef branch in ghalse/simplesamlphp, which was based off the simplesamlphp-2.0 branch

simplesamlphp-2.0...ghalse:simplesamlphp:authnContextClassRef

tvdijen pushed a commit that referenced this pull request Oct 27, 2022
Forward port of @tauceti2 patch in #833
to apply cleanly against simplesamlphp-2.0.0-rc2
tvdijen pushed a commit that referenced this pull request Oct 27, 2022
Forward port of @tauceti2 patch in #833
to apply cleanly against simplesamlphp-2.0.0-rc2
@tvdijen
Copy link
Copy Markdown
Member

tvdijen commented Oct 27, 2022

Ah, thanks! Cherry-pick it!

@tvdijen tvdijen closed this Oct 27, 2022
@github-actions
Copy link
Copy Markdown
Contributor

\n This pull request has been automatically locked since there has \n not been any recent activity after it was closed.\n Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants