feat: saml federation

[sebferrer]

The original PR https://github.com/ory/kratos/pull/2148 has been accidentally closed, so here's a new one.

Completion Progress

• [x] Main endpoints
• [x] SDK adaptation
• [x] Responses consumption
• [x] RelayState continuity
• [x] UI adaptation
• [x] YAML Configuration

Main endpoints

Concerning the first part, the goal is to develop the two main endpoints :

• /metadata (GET) : Generate the metadata of the SP (Kratos)
• /acs (POST) : Handle SAML request

Then we have to deal with the way endpoints work. The library already implements what we want to make these endpoints work. The library allows you to create a metadata file very easily so we will need to incorporate it into Kratos to allow the endpoint /metadata to create them easily. Concerning the endpoint /acs, the Crewjam library allows to receive the SAML requests, to understand them and to treat them accordingly.

SDK adaptation

The goal here is to allow the SDK to call our SAML methods. Currently, the SDK allows to protect a route via a redirection to the login page. We should copy this system a little and allow to protect a route via SAML by a redirection to the IDP. After authentication, the IDP will redirect the user to the desired page. There is also the very important problem of converting the session created by the Crewjam/saml library into a Kratos session to remain homogeneous.

Responses consumption

Now that the endpoints are created, the SAML responses must be processed by Kratos. This means that the endpoint /acs must receive the SAML responses, consume them and translate them into a language that Kratos can understand. More clearly, this endpoint must allow Kratos to support SAML requests and to perform the actions associated with these requests.

It is also in this part that you must check if the session has not expired (according to the duration indicated in option). If it is the case, you have to send a SAML Request to the IDP.

RelayState continuity

Kratos has a continuity management system that allows it to validate the continuity of a login flow (in particular to prevent the possibility of forging login requests). This system is based on the transit of a continuity cookie along the login flow. In the case of SAML, the cookie is lost at the end of the chain because the response is sent from the IdP to the SP in POST, and the SameSite cookies is set to "Lax". The idea is to keep the already present pattern which consists in starting from a continuity cookie to end with the same cookie, rebuilding the lost cookie that would have been kept via the RelayState. It is therefore necessary to add a continuity manager which uses the same principles as the current one, but which would also be based on the transit of the continuity value via the RelayState. More information here: https://github.com/ory/kratos/discussions/2486

UI adaptation

Now we need to make the buttons corresponding to our SAML configuration appear in the UI. This requires an adaptation of the Nodes in Kratos, but also in Elements. Indeed, Ory moved all of the UI rendering and handling of the flow object to Ory Elements so that it is reusable across many examples as well as the kratos-selfservice-ui-node repository. Right now they opted to expect certain flow nodes in the structure they want. Here is the related PR in ory/elements: https://github.com/ory/elements/pull/54

YAML Configuration

Finally, the last part will concern the configuration. Not everyone wants to use SAML so we will have to use the YAML and Kratos configuration system to adapt it to SAML by adding new options to indicate if we want to use SAML and fill in the endpoints. The objective here is to make the final link between Kratos and SAML and thus be able to create instances of Kratos implementing SAML.

Concerning the options, here are the variables we can modify :

• Bindings
• Session duration
• Level of security (Call the RequireAccount method every time that a route protected by Kratos is accessed or not)
• Traits update

Related issue(s)

Design Document

Checklist

• [x] I have read the contributing guidelines.
• [x] I have referenced an issue containing the design document if my change introduces a new feature.
• [x] I am following the contributing code guidelines.
• [x] I have read the security policy.
• [x] I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security. vulnerability, I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.
• [x] I have added tests that prove my fix is effective or that my feature works.
• [x] I have added or changed the documentation.

Disclaimer

At the moment, this is only a first version which is not intended to be merge. All the documentation and tests are still to be done.

[kmherrmann]

Hello again - just wanted to check in on this: When would you like us to take a look at this PR?

[psauvage0]

Hi @kmherrmann, I think there is still some minor tidying up to do on the branch, but the people in our team working on it are currently on vacation, and should come back next week. However, as the PR is pretty large, I'd say you can start reviewing now.

[codecov[bot]]

Codecov Report

Merging #2653 (6cf0778) into master (2d489e7) will decrease coverage by 4.05%. The diff coverage is 37.91%.

:exclamation: Current head 6cf0778 differs from pull request most recent head 7a827c6. Consider uploading reports for the commit 7a827c6 to get more accurate results

@@            Coverage Diff             @@
##           master    #2653      +/-   ##
==========================================
- Coverage   77.50%   73.46%   -4.05%     
==========================================
  Files         314      305       -9     
  Lines       19897    17517    -2380     
==========================================
- Hits        15421    12868    -2553     
- Misses       3285     3638     +353     
+ Partials     1191     1011     -180     

Impacted Files	Coverage Δ
continuity/manager.go	72.41% <ø> (ø)
continuity/manager_relaystate.go	0.00% <0.00%> (ø)
identity/credentials.go	75.00% <ø> (-5.00%)	:arrow_down:
...elfservice/strategy/saml/strategy/strategy_auth.go	0.00% <0.00%> (ø)
ui/node/node.go	91.20% <ø> (-1.10%)	:arrow_down:
x/provider.go	75.00% <ø> (+25.00%)	:arrow_up:
x/relaystate.go	0.00% <0.00%> (ø)
...lfservice/strategy/saml/strategy/strategy_login.go	18.00% <18.00%> (ø)
selfservice/strategy/saml/strategy/strategy.go	24.87% <24.87%> (ø)
...ce/strategy/saml/strategy/strategy_registration.go	32.14% <32.14%> (ø)
... and 327 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[splaunov]

How could we design end-to-end tests? Would it be a good choice as IdP for tests? https://gluu.org/docs/gluu-server/4.1/admin-guide/saml/

Or are there any simpler solutions? Some SAML IdP reference implementation?

[aeneasr]

I think there is an online SAML test server, I saw it somewhere on slack. I'd like to avoid setting up Gloo, I've tried before but it was really difficult to get it working (it was ~1 year ago).

[splaunov]

Will look into these: https://samltest.id https://www.samltool.com https://mocksaml.com https://stackoverflow.com/questions/1125915/can-you-recommend-a-saml-2-0-identity-provider-for-test

[sebferrer]

Will look into these: https://samltest.id https://www.samltool.com https://mocksaml.com https://stackoverflow.com/questions/1125915/can-you-recommend-a-saml-2-0-identity-provider-for-test

We used a lot samltest.id and samltool, I recommend these :)

[sebferrer]

Good news, this PR is ready for review!

We will soon take into account the first feedbacks and then take care of the missing tests.

Thank you!

[splaunov]

Have added registration test: https://github.com/ovh/kratos/pull/4

[splaunov]

Have added WebView support and test https://github.com/ovh/kratos/pull/4

[sebferrer]

Hey @splaunov Some changes are necessary in your PR following the reorganization of our part of the code, please take a look at it! :) https://github.com/ovh/kratos/pull/4#pullrequestreview-1145379698

[batmilkyway]

Will there be a follow up PR to support SAML in oathkeeper?

[sebferrer]

Will there be a follow up PR to support SAML in oathkeeper?

@batmilkyway This is unfortunately not planned on our side at the moment.

[sebferrer]

Awesome, thank you for working on this feature! There is a couple of topics I'd like to address, please see my comments. Thanks! :)

Hey, I think we've taken in account all points of your review, don't hesitate to take a look!

[CLAassistant]

All committers have signed the CLA.

[sebferrer]

Hey @aeneasr, small update: we added unit tests to cover our continuity manager based on RelayState (essential to the SAML flow, more information here https://github.com/ory/kratos/discussions/2486). Feel free to have a look at it, as well as at the previous modifications which take into account your last review :)

[aeneasr]

I tried pushing some changes required for merging the PR to your fork & branch, but it appears that I am not allowed to do so 😕

% git push ...
ERROR: Permission to push denied to aeneasr.
fatal: could not read from the remote repository.

Please make sure that you have the correct access rights
and the repository exists.

But the good news is, giving access is easy! :relaxed: All you need to do is enable write access for maintainers. Thank you! 😄

If the repository belongs to an organization, please add me for the project as a collaborator!

[aeneasr]

I wanted to fix the formatter error as well as the SDK generation error :)

[sebferrer]

@aeneasr thanks a lot for your help! You should have access to push on ovh/kratos now :)

[aeneasr]

Great, thanks! I have allocated some time on monday to look into the PR :)

[ThibHrrd]

Hey @aeneasr, thank you very much for your review. We wanted to finish our current task before taking it on, so here is some information about it: You had some concerns about the security of our SAML implementation. So we decided to set up some security tests. There are 27 new tests to check the main known SAML vulnerabilities. There is also a README summarizing what we did: https://github.com/ovh/kratos/blob/saml/selfservice/strategy/saml/saml_vulnerabilities_check.md

Don't hesitate if you have any questions or feedback!

[lol768]

I don't have much technical to contribute here, but just wanted to say this is really exciting to see being developed and it'll be great to see Kratos get SAML SP support when this is merged :rocket:

[sebferrer]

Hey @aeneasr! I think all your last reviews have been taken into account, we did a refactoring of the continuity manager in the case of using RelayState. Feel free to give us feedback on it, we can't wait to move forward with the Merge! :)

[mbessette-cleo]

Conflicts @sebferrer

And thank you for all the work on this! Definitely excited to see this get in!

[sebferrer]

Hey @aeneasr, thanks a lot for your review!

The points of improvement are very clear to us and will keep us moving in the right direction.

We had to move on to other priority tasks so we won't be making the changes right away. We can't make any commitment on a specific date to consider your review, but this PR is definitely still on the table for us.

If it takes too long for you, feel free to take over the code yourself, from our side we will keep you informed when we resume our work on this project.

Thank you again! 🙂

[Satish-Karunanithi]

Hi @aeneasr, we are looking for this SAML support for quite some time. Would like to know when this feature will be merged and available? This PR merge is blocked for some code issue? Thanks.