SAML Implementation in Kratos #3

ThibHrrd · 2022-05-03T09:00:08Z

SAML Implementation

Related issue(s)

ory#1928

Stoakes

Nice work !

Some questions and a few comments. Mostly on English, Golang & code style.

Stoakes · 2022-05-03T13:00:54Z

.schema/api.openapi.json

@@ -1932,6 +1932,81 @@
        ]
      }
    },
+    "/self-service/saml/metadata":{
+      "get":{
+        "description": "This endpoint is for the IDP to obtain kratos metadata",


The style for the other descriptions is slightly different.

"This endpoints returns kratos metadata as part of a SAMl login" would be closer to what is done on other endpoints.

Stoakes · 2022-05-03T13:01:51Z

.schema/api.openapi.json

+    },
+    "/self-service/saml/idp":{
+      "get":{
+        "description": "This endpoint is to redirect the user to the idp auth flow",


Same as above: style for description text is different from other already existing endpoints.

Stoakes · 2022-05-03T13:04:07Z

.schema/api.openapi.json

+      "get":{
+        "description": "This endpoint is for the IDP to obtain kratos metadata",
+        "operationId": "getSamlMetadata",
+        "response": {


I'm missing a few bits on SAML flow, so this might be an irrelevant comment; is it expected there's no 200 response ?

Stoakes · 2022-05-03T13:05:57Z

.schema/openapi.json

@@ -2006,6 +2006,81 @@
        ]
      }
    },
+    "/self-service/saml/idp":{
+      "get":{
+        "description": "This endpoint is to redirect the user to the idp auth flow",


All other descriptions are something like "This endpoint + verb at the active form".

Stoakes · 2022-05-03T13:09:29Z

embedx/config.schema.json

+        },
+        "mapper_url": {
+          "title": "Jsonnet Mapper URL",
+          "description": "The URL where the jsonnet source is located for mapping the provider's data to Ory Kratos data.",


misc english wording

Suggested change

"description": "The URL where the jsonnet source is located for mapping the provider's data to Ory Kratos data.",

"description": "Location for the jsonnet mapping between provider's data and Ory Kratos data",

Stoakes · 2022-05-03T13:33:17Z

selfservice/strategy/saml/strategy/strategy.go

+
+	// does not need sorting because there is only one field
+	c.SetCSRF(s.d.GenerateCSRFToken(r))
+	// AddSamlProviders(c, conf.Providers, message)


Commented code can be removed

Stoakes · 2022-05-03T13:35:17Z

selfservice/strategy/saml/strategy/strategy.go

+	return err
+}
+
+func (s *Strategy) CountActiveCredentials(cc map[identity.CredentialsType]identity.Credentials) (count int, err error) {


Personal preference: returning explicitly values instead of using named return argument. I do so, because I find it less error prone.

Stoakes · 2022-05-03T13:36:05Z

selfservice/strategy/saml/strategy/strategy.go

+	return
+}
+
+func (s *Strategy) CountActiveFirstFactorCredentials(cc map[identity.CredentialsType]identity.Credentials) (count int, err error) {


This function requires a bit of reading to fully understand what it does; it might be interesting to add a comment describing what it does.

Stoakes · 2022-05-03T13:37:57Z

selfservice/strategy/saml/strategy/strategy_auth.go

+// This file is called when the /ACS receives an assertion, the method below allows you to define whether to register or login the user indicated in the assertion
+//#################
+
+// Handle SAML Assertion and process to either login or register


golang standard comment format starts with variable of function name.

Kratos uses a different one:

kratos/selfservice/flow/recovery/handler.go

Line 203 in af1df63

// HTTP Cookies

To be adapted based on your knowledge of ory coding style.

Stoakes · 2022-05-03T13:38:35Z

selfservice/strategy/saml/strategy/strategy_auth.go

+			if err = s.processRegistration(w, r, registerFlow, provider, claims); err != nil {
+				if i == nil {
+					return nil, s.handleError(w, r, registerFlow, provider.Config().ID, nil, err)
+				} else {


Unused else: No need to add a else when last statement of an if is a return.

Here and below

psauvage0

Great work ! :D

The main problem I see is with the attribute mapping that only works with a fixed list of attributes names.

Otherwise it's about small code quality improvements. There are some pointed out by the linter that I didn't bother to comment as they are already shown in the PR builds.

I honestly think that we could already take reviews from the Ory team & community, as there are maybe some things about how they want the code to be organized.

psauvage0 · 2022-05-03T09:34:42Z

embedx/config.schema.json

+            "Microsoft Active Directory"
+          ]
+        },
+        "public_cert_path": {


certificate_path ? Or public_key_certificate_path ?

psauvage0 · 2022-05-03T09:47:31Z

embedx/config.schema.json

+            }
+          ]
+        },
+        "attributes_map": {


The attributes mapping is given in the config ? What if I have an attribute that is not provided here ? For example group ?

For the moment, this part is not configurable, that is to say that the identity cannot have more fields than what is present here.

psauvage0 · 2022-05-03T10:00:40Z

internal/httpclient/api/openapi.yaml

@@ -1185,6 +1185,51 @@ paths:
      summary: Create a Logout URL for Browsers
      tags:
      - v0alpha2
+  /self-service/methods/saml/auth:


Not related to the doc but to the route of this call (I don't know where the route is defined).

In the Ory Documentation for the API, we can see that when there are different calls to make for browser vs API, they usually end the route with /browser or /api. Maybe the same can be applied here ?

We did it on purpose based on a misunderstanding, indeed we should change it back

psauvage0 · 2022-05-03T10:20:08Z

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))


No Out Of Bound possible here ?

psauvage0 · 2022-05-03T10:36:45Z

selfservice/flow/saml/handler.go

+	var idpMetadata *samlidp.EntityDescriptor
+
+	// We check if the metadata file is provided
+	if c.SAMLProviders[len(c.SAMLProviders)-1].IDPInformation["idp_metadata_url"] != "" {


OOB ?
Also, you can check if a map entry is set with something like if value, ok := mymap["key"]; ok { ...

Suggested change

if c.SAMLProviders[len(c.SAMLProviders)-1].IDPInformation["idp_metadata_url"] != "" {

if metadataURL, ok := c.SAMLProviders[len(c.SAMLProviders)-1].IDPInformation["idp_metadata_url"]; ok {

psauvage0 · 2022-05-03T13:14:53Z

selfservice/strategy/saml/strategy/strategy.go

+	return
+}
+
+func (s *Strategy) CountActiveMultiFactorCredentials(cc map[identity.CredentialsType]identity.Credentials) (count int, err error) {


Not implemented ?

Not yet. This implementation isn't required since a long time, so we had to do it like this on our last rebase.

psauvage0 · 2022-05-03T13:28:36Z

selfservice/strategy/saml/strategy/strategy_auth.go

+			return nil, err
+		}
+	} else {
+		// The user already exist in database so we log him


This makes me think : what happens when the IDP deletes a user ? I guess the user remains in the SP forever... What are the consequences of that ? It should not be a problem at all, but we could investigate this case a bit

This is indeed what will happen but it is complicated for the SP to know that the IDP has deleted a user

psauvage0 · 2022-05-03T13:39:15Z

selfservice/strategy/saml/strategy/strategy_login.go

+	return nil, nil
+}
+
+// Method not used but necessary to implement the interface


I find this weird but if you say so 🤨

Here's the kind of error we get otherwise:

# github.com/ory/kratos/selfservice/strategy/saml/strategy selfservice/strategy/saml/strategy/strategy_login.go:25:24: cannot use new(Strategy) (value of type *Strategy) as type login.Strategy in variable declaration: *Strategy does not implement login.Strategy (missing Login method)

What I found weird was that the method is called "Login", so I would expect that it is a pretty important method to implement in an IAM.

Edit: As we discussed, it's because of the rebase against the origin. It's a new method added to the interface, probably worth checking where this method is called, to ensure that it is safe to not implement it

psauvage0 · 2022-05-03T13:55:59Z

selfservice/strategy/saml/strategy/test/strategy_test.go

+	is "gotest.tools/assert/cmp"
+)
+
+func TestGetAndDecryptAssertion(t *testing.T) {


Unrelated to the code : it would be interesting to discuss test strategy with the Ory community. It seems there is not a lot of info on what is considered a good level of testing on CONTRIBUING.md file (should we cover all branches of a functions ? Should we test for edge cases ? Or simply testing the main intent of the function is OK ?)

psauvage0 · 2022-05-03T14:00:07Z

spec/api.json

@@ -3636,6 +3653,48 @@
        ]
      }
    },
+    "/self-service/methods/saml/auth": {


Same thing as before about the route : /browser vs /api routes

… account using oidc. (ory#2496) Close ory#2444 Co-authored-by: aeneasr <[email protected]>

Closes ory#2380 Co-authored-by: aeneasr <[email protected]>

BREAKING CHANGE: SDK Method `getJsonSchema` was renamed to `getIdentitySchema`.

[skip ci]

This PR resolves an issue with the redirect after a successful verification, if not specified.

[skip ci]

…y#2908)

[skip ci]

Previously the context was not propagated to the http client. As a result the (instrumented) client did not find the existing span and the sapns for outgoing http request have been orphains. With this simple Fix they are now children of the corresponding webhook spans.

…2917) Request cookies were already available in raw form in the ctx.request_headers top-level argument to the Jsonnet snippet. Parsing cookies in Jsonnet is tedious and error-prone, though, so we parse them internally for convenience.

[skip ci]

BREAKING CHANGE: The `/admin/courier/messages` endpoint now uses `keysetpagination` instead.

[skip ci]

Signed-off-by: ThibaultHerard <[email protected]> Co-authored-by: sebferrer <[email protected]> Co-authored-by: psauvage <[email protected]> Co-authored-by: alexGNX <[email protected]> Co-authored-by: Stoakes <[email protected]>

Signed-off-by: ThibaultHerard <[email protected]> Co-authored-by: sebferrer <[email protected]>

Signed-off-by: sebferrer <[email protected]> Co-authored-by: ThibaultHerard <[email protected]>

ThibHrrd requested review from alexGNX, Stoakes and sebferrer May 3, 2022 12:38

Stoakes reviewed May 3, 2022

View reviewed changes

psauvage0 suggested changes May 3, 2022

View reviewed changes

ThibHrrd force-pushed the saml branch 4 times, most recently from 6b5b243 to 4272676 Compare May 6, 2022 16:13

alexGNX force-pushed the saml branch 6 times, most recently from c7d132c to e70a0f3 Compare May 12, 2022 08:44

kopancek mentioned this pull request May 12, 2022

Design Document for SAML2 Implementation as a Service Provider ory/kratos#1928

Closed

6 tasks

aeneasr and others added 14 commits July 21, 2022 09:21

chore: format

de77771

test(e2e): use correct dir

907dbe3

test(e2e): improve webauthn test reliability

4d323d0

fix: make hydra consistently localhost

70211a1

fix: panic

1182278

fix: metadata should not be required

05afd68

ci: bump hydra

28ccc01

test: fix oidc test regression

6c14b68

fix: duplicate messages in recovery flow (ory#2592)

43fcc51

fix: return browser to 'return_to' when logging in without registered…

a4194f5

… account using oidc. (ory#2496) Close ory#2444 Co-authored-by: aeneasr <[email protected]>

feat: add PATCH to adminUpdateIdentity (ory#2380) (ory#2471)

94a3741

Closes ory#2380 Co-authored-by: aeneasr <[email protected]>

refactor(sdk): rename getJsonSchema to getIdentitySchema (ory#2606)

8dc2ecf

BREAKING CHANGE: SDK Method `getJsonSchema` was renamed to `getIdentitySchema`.

fix: format

0934def

ci: add issues and PRs to board

1d7381a

jonas-jonas and others added 12 commits November 18, 2022 09:49

feat: change code length to 6 numbers (ory#2894)

56feb07

autogen(docs): regenerate and update changelog

2d46209

[skip ci]

fix: active attribute based off IsActive checks (ory#2901)

bcbf68e

autogen(docs): regenerate and update changelog

9d64033

[skip ci]

feat: add "success" UITextType (ory#2900)

2ff34b6

autogen(docs): regenerate and update changelog

b6c212c

[skip ci]

fix: verification redirect & continue label (ory#2905)

e1119e8

This PR resolves an issue with the redirect after a successful verification, if not specified.

autogen(docs): regenerate and update changelog

57e569e

[skip ci]

fix: set cache default to false (ory#2906)

e407f92

fix: send out correct verification invalid email in code strategy (or…

d2bb67a

…y#2908)

autogen(docs): regenerate and update changelog

14c79b4

[skip ci]

feat: support md5 hash import (ory#2725)

d1b4e17

sebferrer force-pushed the saml branch from 141a9a3 to c0f06b3 Compare November 25, 2022 14:59

martinei and others added 15 commits November 28, 2022 18:50

autogen: pin v0.11.0-alpha.0.pre.2 release commit

624e1f0

autogen: pin v0.11.0 release commit

59c30b6

autogen(docs): generate and bump docs

727439c

[skip ci]

autogen: add v0.11.0 to version.schema.json

bef1a6c

[skip ci]

feat: require verification on login (ory#2927)

efb8ae8

feat: store errors of courier message (ory#2914)

fc7aa86

BREAKING CHANGE: The `/admin/courier/messages` endpoint now uses `keysetpagination` instead.

autogen(openapi): regenerate swagger spec and internal client

ce72501

[skip ci]

autogen(docs): regenerate and update changelog

b3b6f88

[skip ci]

fix: cors headers not added to the response ory#2922 (ory#2934)

1ed6839

feat(saml): saml 2.0 implementation

3f5480f

Signed-off-by: ThibaultHerard <[email protected]> Co-authored-by: sebferrer <[email protected]> Co-authored-by: psauvage <[email protected]> Co-authored-by: alexGNX <[email protected]> Co-authored-by: Stoakes <[email protected]>

feat(saml): use ory/x fetcher

b26f11d

Signed-off-by: ThibaultHerard <[email protected]> Co-authored-by: sebferrer <[email protected]>

feat(saml): improved error handling

37f7bbe

Signed-off-by: ThibaultHerard <[email protected]> Co-authored-by: sebferrer <[email protected]>

feat(saml): relaystate continuity fix + unit tests

aeb172c

Signed-off-by: sebferrer <[email protected]> Co-authored-by: ThibaultHerard <[email protected]>

sebferrer force-pushed the saml branch from fc408c6 to aeb172c Compare December 14, 2022 16:13

sebferrer closed this Jan 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SAML Implementation in Kratos #3

SAML Implementation in Kratos #3

ThibHrrd commented May 3, 2022

Stoakes left a comment

Stoakes May 3, 2022

Stoakes May 3, 2022

Stoakes May 3, 2022

Stoakes May 3, 2022

Stoakes May 3, 2022

Stoakes May 3, 2022

Stoakes May 3, 2022

Stoakes May 3, 2022

Stoakes May 3, 2022

Stoakes May 3, 2022

psauvage0 left a comment

psauvage0 May 3, 2022

psauvage0 May 3, 2022

ThibHrrd May 4, 2022

psauvage0 May 3, 2022

sebferrer May 3, 2022

psauvage0 May 3, 2022

psauvage0 May 3, 2022

psauvage0 May 3, 2022

sebferrer May 3, 2022

psauvage0 May 3, 2022

ThibHrrd May 4, 2022

psauvage0 May 3, 2022

sebferrer May 3, 2022

psauvage0 May 4, 2022 •

edited

Loading

psauvage0 May 3, 2022

psauvage0 May 3, 2022

	"description": "The URL where the jsonnet source is located for mapping the provider's data to Ory Kratos data.",
	"description": "Location for the jsonnet mapping between provider's data and Ory Kratos data",

	if c.SAMLProviders[len(c.SAMLProviders)-1].IDPInformation["idp_metadata_url"] != "" {
	if metadataURL, ok := c.SAMLProviders[len(c.SAMLProviders)-1].IDPInformation["idp_metadata_url"]; ok {

SAML Implementation in Kratos #3

SAML Implementation in Kratos #3

Conversation

ThibHrrd commented May 3, 2022

SAML Implementation

Related issue(s)

Stoakes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psauvage0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psauvage0 May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psauvage0 May 4, 2022 •

edited

Loading