Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implementing SAML in Kratos #2148

Merged
merged 0 commits into from
Aug 9, 2022
Merged

feat: implementing SAML in Kratos #2148

merged 0 commits into from
Aug 9, 2022

Conversation

ThibHrrd
Copy link

@ThibHrrd ThibHrrd commented Jan 14, 2022

Completion Progress

  • First Part
  • Second Part
  • Third Part
  • Fourth Part
  • Fifth Part

We thought about 5 main tasks :

First Part

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

Second Part

The second part will 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.

Third Part

The goal here is to allow the SDK to call our SAML function. 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.

Fourth Part

Now that the endpoints are created, the SAML requests must be processed by Kratos. This means that the endpoint /acs must receive the SAML requests, understand 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.

Fifth part

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

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • 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
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • 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.

@CLAassistant
Copy link

CLAassistant commented Jan 14, 2022

CLA assistant check
All committers have signed the CLA.

@ThibHrrd ThibHrrd changed the title Implementing SAML in Kratos feat: implementing SAML in Kratos Jan 14, 2022
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Took a quick peak :)

From what I can say, this looks pretty good in terms of what files and directories are being touched :) Keep going! :)

contrib/quickstart/kratos/email-password/kratos.yml Outdated Show resolved Hide resolved
driver/config/config.go Outdated Show resolved Hide resolved
embedx/config.schema.json Outdated Show resolved Hide resolved
embedx/config.schema.json Outdated Show resolved Hide resolved
selfservice/strategy/saml/types.go Outdated Show resolved Hide resolved
selfservice/strategy/saml/types.go Outdated Show resolved Hide resolved
@ThibHrrd
Copy link
Author

Hello @aeneasr! Thank you very much for the feedback on my code, it helps me a lot. I will take into consideration all the remarks and improve my code.

@ThibHrrd
Copy link
Author

ThibHrrd commented Feb 1, 2022

I'm making another comment to describe my progress a little bit. The session creation is working well. I still have a problem with the passing of attributes from the IDP for the session creation. I also have a problem with the database migration scripts. I need to add values at database startup but my scripts are not used, I don't know why. And finally, a last problem with the redirection after authentication where the session cookies are not transmitted and therefore the session is not detected by the /whoami.

@ThibHrrd
Copy link
Author

@sebferrer sebferrer force-pushed the saml branch 3 times, most recently from 2ec70d0 to aac5547 Compare April 6, 2022 09:31
@ThibHrrd ThibHrrd force-pushed the saml branch 6 times, most recently from cac09e2 to 92015ba Compare April 19, 2022 15:49
@ThibHrrd ThibHrrd force-pushed the saml branch 4 times, most recently from 6b5b243 to 4272676 Compare May 6, 2022 16:13
@ThibHrrd ThibHrrd marked this pull request as ready for review May 6, 2022 16:15
@ThibHrrd ThibHrrd requested a review from zepatrik as a code owner May 6, 2022 16:15
@ThibHrrd
Copy link
Author

ThibHrrd commented May 6, 2022

@aeneasr Hello, I'm proud to announce that our implementation of SAML in Kratos is ready for review. The PR for the documentation will come soon.

Do not hesitate to give us feedback about our work.

I will not be very available next month so take your time to review it (I will still take a look from time to time 😃)

@ThibHrrd ThibHrrd requested a review from aeneasr May 6, 2022 16:31
@alexGNX alexGNX force-pushed the saml branch 4 times, most recently from 685b455 to 5a13d4b Compare May 10, 2022 16:29
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #2148 (d05acdf) into master (4fe2d25) will decrease coverage by 0.87%.
The diff coverage is 47.77%.

❗ Current head d05acdf differs from pull request most recent head e70a0f3. Consider uploading reports for the commit e70a0f3 to get more accurate results

@@            Coverage Diff             @@
##           master    #2148      +/-   ##
==========================================
- Coverage   76.30%   75.42%   -0.88%     
==========================================
  Files         316      327      +11     
  Lines       17463    18026     +563     
==========================================
+ Hits        13326    13597     +271     
- Misses       3195     3445     +250     
- Partials      942      984      +42     
Impacted Files Coverage Δ
cmd/remote/status.go 0.00% <0.00%> (ø)
cmd/remote/version.go 0.00% <0.00%> (ø)
identity/credentials.go 69.23% <ø> (ø)
identity/credentials_saml.go 0.00% <0.00%> (ø)
...elfservice/strategy/saml/strategy/strategy_auth.go 0.00% <0.00%> (ø)
ui/node/node.go 90.65% <ø> (ø)
...lfservice/strategy/saml/strategy/strategy_login.go 21.05% <21.05%> (ø)
...ce/strategy/saml/strategy/strategy_registration.go 24.61% <24.61%> (ø)
selfservice/strategy/saml/strategy/strategy.go 35.00% <35.00%> (ø)
selfservice/flow/saml/handler.go 53.43% <53.43%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b682689...e70a0f3. Read the comment docs.

@alexGNX alexGNX force-pushed the saml branch 2 times, most recently from c7d132c to e70a0f3 Compare May 12, 2022 08:44
@sebferrer
Copy link

sebferrer commented Jun 3, 2022

Hey,
We have a little more work than expected to finalize the implementation and felt that the current feature wasn't yet complete enough to review.
So we are setting it back to Draft for now.
We will get back to you as soon as it's okay for us, thanks for your patience! 🙂

@ThibHrrd ThibHrrd marked this pull request as draft June 3, 2022 11:07
@kmherrmann
Copy link
Contributor

Really great feature, thanks so much for working on this!
How are things progressing on your end? Any blockers, anything where you'd need our support? Thanks!

@sebferrer
Copy link

Hey,

Following a mistake that accidentally caused this PR to automatically change to "Merged" status, we've reopened it here: #2653

To answer to your question @kmherrmann, we've made good progress since our last update, we're about to ask for a review :)

@kmherrmann
Copy link
Contributor

Fantastic, thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants