feat: saml federation #2653

sebferrer · 2022-08-09T15:25:27Z

The original PR #2148 has been accidentally closed, so here's a new one.

Completion Progress

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: #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: ory/elements#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

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.

selfservice/flow/saml/helpertest/helpertest.go

selfservice/strategy/saml/error.go

embedx/config.schema.json

internal/httpclient/api/openapi.yaml

internal/httpclient/docs/V0alpha2Api.md

persistence/sql/migrations/templates/20201201161451_credential_types_values.down.fizz

postgres.yaml

kmherrmann · 2022-08-25T08:34:15Z

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

psauvage0 · 2022-08-25T11:58:51Z

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.

continuity/manager_relaystate.go

codecov · 2022-09-01T15:53:41Z

Codecov Report

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

❗ 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%)`	⬇️
...elfservice/strategy/saml/strategy/strategy_auth.go	`0.00% <0.00%> (ø)`
ui/node/node.go	`91.20% <ø> (-1.10%)`	⬇️
x/provider.go	`75.00% <ø> (+25.00%)`	⬆️
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.

aeneasr

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

aeneasr · 2022-09-05T13:34:14Z

selfservice/flow/saml/handler.go

@@ -0,0 +1,336 @@
+package saml


To follow the rest of the design pattern, this handler should move into the strategy. I don't think that it belongs into the flow as it doesn't have its own flow object nor any other properties of a flow (hooks, ui, ...) as far as I can tell.

aeneasr · 2022-09-05T13:36:18Z

selfservice/flow/saml/handler.go

+	}
+
+	// Key pair to encrypt and sign SAML requests
+	keyPair, err := tls.LoadX509KeyPair(strings.Replace(c.SAMLProviders[len(c.SAMLProviders)-1].PublicCertPath, "file://", "", 1), strings.Replace(c.SAMLProviders[len(c.SAMLProviders)-1].PrivateKeyPath, "file://", "", 1))


This "use the last entry" functionality is copy & pasted across several lines. Do we need it everywhere? ANd why do we care only about the last entry?

Good point. This is a leftover from the very beginning of our work on SAML implementation. We implemented the Provider() method incorrectly (provider_config.go), I'll fix that quickly :)

aeneasr · 2022-09-05T13:37:58Z

selfservice/flow/saml/handler.go

+	}
+	keyPair.Leaf, err = x509.ParseCertificate(keyPair.Certificate[0])
+	if err != nil {
+		return err


Please wrap errors in descriptive error messages using the herodot package (e.g. herodot.ErrBadRequest) and use errors.WithStack() to include stack traces

aeneasr · 2022-09-05T13:38:21Z

selfservice/flow/saml/handler.go

+
+		metadataURL := c.SAMLProviders[len(c.SAMLProviders)-1].IDPInformation["idp_metadata_url"]
+		// The metadata file is provided
+		if strings.HasPrefix(metadataURL, "file://") {


We have a library for that in ory/x called fetcher, please use that one!

Hey, we've done the file reorganization, it should fit your design pattern better now. We are now about to take into account the rest of your review :)

splaunov · 2022-09-08T07:59:35Z

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 · 2022-09-08T08:02:54Z

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 · 2022-09-08T08:40:44Z

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 · 2022-09-08T09:15:01Z

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 · 2022-09-08T13:09:35Z

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 · 2022-09-08T14:35:17Z

selfservice/flow/saml/handler.go

+	// We have to get the SessionID from the cookie to inject it into the context to ensure continuity
+	cookie, err := r.Cookie(continuity.CookieName)
+	if err != nil {
+		h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err)


If cookie is not set, nil pointer dereference error is thrown down the code.

splaunov · 2022-09-16T06:58:50Z

Have added registration test: ovh#4

sebferrer · 2022-11-25T17:57:15Z

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 · 2022-12-14T16:13:27Z

All committers have signed the CLA.

sebferrer · 2022-12-14T16:31:53Z

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

aeneasr · 2023-01-05T12:58:46Z

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! ☺️ 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 · 2023-01-05T12:59:14Z

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

sebferrer · 2023-01-05T13:41:48Z

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

aeneasr · 2023-01-05T16:21:17Z

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

aeneasr

Thank you! This is already in a great state. I've reviewed everything outside the SAML module for now, the saml module itself is still required to review on my end.

aeneasr · 2023-01-12T09:47:11Z

continuity/manager_relaystate.go

@@ -0,0 +1,151 @@
+package continuity


This file is almost a 1:1 copy of manger_cookie. Can you please remove it and use manager_cookie instead? If you need to parametrize things, feel ree to adjust the manager_cookie file :)

Done! We now use the manager_cookie and make the use of the RelayState parametrizable :)

aeneasr · 2023-01-12T09:47:23Z

contrib/quickstart/kratos/email-password/identity.schema.json

@@ -1,3 +1,6 @@
+// 20221125150145


Remove please - invalid json

aeneasr · 2023-01-12T09:49:39Z

driver/registry_default.go

+	// If m.continuityManager is nil or not a continuity.ManagerCookie
+	switch m.continuityManager.(type) {
+	case *continuity.ManagerCookie:
+	default:
 		m.continuityManager = continuity.NewManagerCookie(m)
 	}
 	return m.continuityManager
 }

+func (m *RegistryDefault) RelayStateContinuityManager() continuity.Manager {
+	// If m.continuityManager is nil or not a continuity.ManagerRelayState
+	switch m.continuityManager.(type) {
+	case *continuity.ManagerRelayState:
+	default:
+		m.continuityManager = continuity.NewManagerRelayState(m, m)
+	}
+	return m.continuityManager
+}


This looks incorrect and racy. If you need two separate managers, save them in separate variables

We don't need separate managers anymore, so problem solved :)

aeneasr · 2023-01-12T09:50:35Z

identity/credentials_saml.go

+// swagger:model identityCredentialsSamlProvider
+type CredentialsSAMLProvider struct {
+	Subject  string `json:"subject"`
+	Provider string `json:"samlProvider"`


We use underscore :)

saml_provider

aeneasr · 2023-01-12T09:51:03Z

identity/credentials_saml.go

+}
+
+// Create an uniq identifier for user in database. Its look like "id + the id of the saml provider"
+func NewCredentialsSAML(subject string, provider string) (*Credentials, error) {


Please add tests for this

aeneasr · 2023-01-12T09:51:49Z

persistence/sql/migrations/sql/2022080818000000000_identity_credentials_types_saml.up.sql

@@ -0,0 +1 @@
+INSERT INTO identity_credential_types (id, name) SELECT 'ff5a1823-8b47-4255-860f-4b70ed122740', 'saml' WHERE NOT EXISTS ( SELECT * FROM identity_credential_types WHERE name = 'saml');


How did you generate this UUID? :)

I generated it by hand because I still didn't understand how it all worked, how am I supposed to do that?

aeneasr · 2023-01-12T09:52:22Z

x/provider.go

@@ -29,6 +29,11 @@ type CookieProvider interface {
 	ContinuityCookieManager(ctx context.Context) sessions.StoreExact
 }

+type RelayStateProvider interface {


We potentially don't need this

Indeed, we removed it

aeneasr · 2023-01-12T09:52:57Z

x/relaystate.go

+)
+
+// SessionGetRelayState returns a string of the content of the relaystate for the current session.
+func SessionGetStringRelayState(r *http.Request, s sessions.StoreExact, id string, key interface{}) (string, error) {


This looks quite complex - which means that it needs a test :)

It's covered by the test in continuity/manager_relaystate_test.go :)

aeneasr · 2023-01-12T09:53:19Z

x/relaystate.go

+// SessionGetRelayState returns a string of the content of the relaystate for the current session.
+func SessionGetStringRelayState(r *http.Request, s sessions.StoreExact, id string, key interface{}) (string, error) {
+
+	cipherRelayState := r.PostForm.Get("RelayState")


This should be underscored and lowercase:

cipherRelayState := r.PostForm.Get("relay_state")

We have to use this "RelayState" notation because it is a SAML standard

ThibHrrd · 2023-02-15T11:00:13Z

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 · 2023-02-18T07:36:44Z

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 🚀

Signed-off-by: ThibaultHerard <thibaultherard10@gmail.com> Co-authored-by: sebferrer <sebferrer@users.noreply.github.com> Co-authored-by: psauvage <psauvage0@users.noreply.github.com> Co-authored-by: alexGNX <alexGNX@users.noreply.github.com> Co-authored-by: Stoakes <Stoakes@users.noreply.github.com>

Signed-off-by: ThibaultHerard <thibaultherard10@gmail.com> Co-authored-by: sebferrer <sebferrer@users.noreply.github.com>

Signed-off-by: sebferrer <sebferrer@users.noreply.github.com> Co-authored-by: ThibaultHerard <thibaultherard10@gmail.com>

+ update saml tests Signed-off-by: ThibaultHerard <thibaultherard10@gmail.com> Co-authored-by: sebferrer <sebferrer@users.noreply.github.com>

Signed-off-by: sebferrer <sebferrer@users.noreply.github.com> Co-authored-by: ThibaultHerard <thibaultherard10@gmail.com>

Signed-off-by: ThibaultHerard <thibaultherard10@gmail.com> Co-authored-by: sebferrer <sebferrer@users.noreply.github.com>

+ test new credentials saml + resolving conflicts with master Signed-off-by: sebferrer <sebferrer@users.noreply.github.com> Co-authored-by: ThibaultHerard <thibaultherard10@gmail.com>

sebferrer · 2023-02-22T17:33:48Z

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 · 2023-03-03T18:14:31Z

Conflicts @sebferrer

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

aeneasr

I finally had some time to look more into the code base - thank you very much. It's a lot of comments and I think there are several areas that have to be addressed before this rolls out in any production system.

As it stands currently, there are several areas of improvements. This ranges from code path issues (forgot to add return) to incorrect function use that would end up with responses that clients can't understand (e.g. sending a HTTP redirect instead of XML).

The traits pull-model on login is not working at the moment, as the identity is not updated as part of login currently.

Further, the traits mapping is naive in a sense that it just copies anything the SAML client sends into the traits, and it does not run the vital validation pipeline. Identities will not be able to perform e.g. account recovery or other basic flows (adding 2FA for example) as far as I can read this.

There seems to be the concept of JsonNet in the test data, but it is - as far as I can tell - not being used by the code base.

There are a couple more topics such as missing thread safety which will eventually end up with panics and invalid configuration. The problem is that we're using a global map-based cache for the configs. This will not work with hot-reloading.

Lastly, there are no functional / integration / e2e tests that perform the complete flow from a-z and prove that it works / test for regression.

In summary, it is great how far this has come! But there are stil miles to do before this can run in a production system.

aeneasr · 2023-04-06T11:53:17Z