support for saml idp initiated flow #1514

scotthew1 · 2019-08-08T21:11:19Z

hey @srenatus i know #1149 was closed due to security concerns, but this flow has become a hard requirement for our use case. that original issue discusses the ability to use the 'state' parameter to handle this, but i found that relying on 'RelayState' from the SAML provider was fine for our use case and don't think this approach ended up being too obtrusive. perhaps this is a bit naive and/or too specific to our use case, but i thought i'd put it out there for discussion.

a few notables:

this relies on the ssoIssuer config (which is otherwise optional) to match the unsolicited post from the SAML provider with the connector in the dex config.
to determine the client the authorization is destined for, I match the RelayState with any redirectURI a known client. i'm not sure if it's valid for two different clients to have the same redirectURI which would make it difficult to determine which client to create authorization for.
i need to fabricate an auth request, so i have to assume some set of claims. it's probably best to make this configureable, but i've just got them hardcode right now.
i have the 'code' response type hardcoded as well. i'm not sure whether or not it's possible to make the implicit flow work with this setup (client would expect to validate a nonce that we can't fabricate).

srenatus · 2019-08-09T05:47:01Z

Thanks! Haven't had a chance to look at this yet, but I'm excited about this. 😃

srenatus

Thanks for whipping this up. Besides my inline nitpicks and comments,

we'll need some tests
and some docs
Do you think this might better be something to opt into using a config value?
I could see this as a configurable of the SAML connector, where you say that for this one, you want to allow IdP-initiated logins. Maybe even combined with client IDs? Like, for this SAML connector, client a and b can use IdP-initiated logins?

Thanks again for contributing this! 🎉

srenatus · 2019-08-09T06:48:56Z

server/handlers.go

-			s.renderError(w, http.StatusBadRequest, "User session error.")
+		var code int
+		var err error
+		authID, samlInResponseTo, code, err = s.handleSamlCallback(r)


[style] Initialisms should be cased like this:

Suggested change

authID, samlInResponseTo, code, err = s.handleSamlCallback(r)

authID, samlInResponseTo, code, err = s.handleSAMLCallback(r)

at least if we want to follow this -- and I think it wouldn't be bad to that. 😉

srenatus · 2019-08-09T06:49:37Z

server/handlers.go

+// handleSamlCallback handles a saml callback response with support for the IdP initiated flow.
+// if the RelayState is not one of our auth requests, we check to see if it is a valid redirectURI
+// for one of our clients. then, if the SAMLResponse is from a registered connector, we create a
+// new auth request in the database so we can contuine with our regular callback flow.


typo:

Suggested change

// new auth request in the database so we can contuine with our regular callback flow.

// new auth request in the database so we can continue with our regular callback flow.

srenatus · 2019-08-09T06:50:33Z

server/handlers.go

+func (s *Server) handleSamlCallback(r *http.Request) (string, string, int, error) {
+	relayState := r.PostFormValue("RelayState")
+	if relayState == "" {
+		return "", "", http.StatusBadRequest, fmt.Errorf("user session error")


There's no need to format anything, let's do

Suggested change

return "", "", http.StatusBadRequest, fmt.Errorf("user session error")

return "", "", http.StatusBadRequest, errors.New("user session error")

Also, something more helpful might be good, if we're touching this part anyways? Like, "missing RelayState"? 🤔

whoops yeah can definitely use errors.New()

re: the various error message comments, i was trying to keep the errors returned to the user intentionally vague for security reasons. since this is already a flow prone to man-in-the-middle attacks i was trying to reduce information gleaned from specific error messages and provide the more detailed error in the logs.

if this isn't something we find important, i can certainly make the returned errors more specific.

You're correct, better safe than sorry. We'll log the details.

srenatus · 2019-08-09T06:53:45Z

server/handlers.go

+		return relayState, relayState, http.StatusOK, nil
+	} else if err != storage.ErrNotFound {
+		s.logger.Errorf("Failed to get auth request: %v", err)
+		return "", "", http.StatusInternalServerError, fmt.Errorf("database error")


Like above, let's either include the err here (fmt.Errorf("database error: %v", err)) or use errors.New(). (There's a few more of these below, not flagging them separately 😉)

srenatus · 2019-08-09T06:55:35Z

server/handlers.go

+		return "", "", http.StatusInternalServerError, fmt.Errorf("database error")
+	}
+	// find the client the RelayState is trying to send us to, stop when we match a redirectURI
+	// TODO: is it valid for differrent clients to have the same redirectURI? ¯\_(ツ)_/¯


I believe it is 🤔 But it might be something to call out in some docs or comments as a restriction when wanting to use IdP-initiated login.

another idea i had is to put the clientId in the RelayState and provide something like a samlRedirectURI config on the client to decide which URI we should send the code to once we've verified the SAML request. this would be a more substantial change but perhaps worth it?

srenatus · 2019-08-09T06:56:14Z

server/handlers.go

+	}
+	if client == nil {
+		s.logger.Errorf("Cannot find client with redirectURI: %s", relayState)
+		return "", "", http.StatusInternalServerError, fmt.Errorf("bad saml response")


No I believe a 400 might be OK here, too.

oh yeah, agreed. copy/paste error

srenatus · 2019-08-09T06:57:38Z

server/handlers.go

+		var cfg saml.Config
+		if err := json.Unmarshal(c.Config, &cfg); err != nil {
+			s.logger.Errorf("Cannot parse saml config for connector %s: %s", c.ID, err)
+			return "", "", http.StatusPreconditionFailed, fmt.Errorf("config error")


Hmm not 500? 💭 But this really cannot happen.... if the config was bad, the server wouldn't start, I think.

yeeeah agreed that this shouldn't ever really happened, but wanted to handle the error regardless. since it's config related i figured it made sense as a precondition failure

srenatus · 2019-08-09T07:06:31Z

server/handlers.go

+		State:               "",
+		Nonce:               "",
+		ForceApprovalPrompt: false,
+		// TODO: make IdP initiated scopes configurable as part of the saml connector


connector/saml/saml.go

scotthew1 · 2019-08-13T21:55:20Z

hey @srenatus here's a thought on supporting custom scopes rather than the hardcoded ones i've got now. update the Client struct in storage to something like...

type Client struct {
	...
	SAMLInitiated *SAMLInitiatedConfig
}

type SAMLInitiatedConfig struct {
	RedirectURI string 
	Scopes      []string
}

this way each client could have different scopes for dealing with an IdP initiated scope. this change also includes the idea i had for handling RelaySate differently were we don't have to try and match to some existing redirectURI on any client, we'd just check if the RelayState matches some existing clientID and then see if that client has this SAMLInitiatedConfig to figure out how this request is handled. finally, this means any client would need to opt-in to working with IdP initiated flows by providing this configuration. perhaps there's even additional configuration here to decide which connectors the client is willing to work with as you described in your original comment.

just wanted to get some feedback on this design before implementing since it impacts storage and thus a bit less trivial to implement.

srenatus · 2019-08-15T17:30:58Z

I haven't dug into how much of an effort this is going to be, but -- taken at face value, having the clients determine if they want to allow being selected for IdP-initiated Sign In seems like a good choice 👍

My only hesitation is that we're adding something to clients that is very specific to a single type of connector. (💭 I wonder if this could be abstracted?) I mean, on the other hand, the SAML connector(s) already necessitate special handling in the server's handlers, and that's a thing differentiating SAML from the rest. (What I mean is that we've already gone down that road.)

scotthew1 · 2019-08-15T20:02:35Z

@srenatus i just pushed some changes with an initial attempt at the approach i described. i've not updated the various storage connectors, but what i had was enough to work with the static config i've been testing with. further scope would involve updating the various storage connectors (some of which look to be easier than others to update).

i feel you on being adverse to having much connector-specific config on the client, but i'm not sure of a better way to handle this.. the whole IdP initiated flow is outside of anything oauth intends to do (with good reason), so it's hard to not have SAML things creep outside of the connector.

if you've got any good ideas for better abstraction let me know! and let me know what you think about the current approach with my latest commit!

scotthew1 · 2019-08-20T15:58:47Z

storage/sql/migrate.go

+				add column saml_init_redirect_uri text not null default '';`,
+			`
+			alter table client
+				add column saml_init_scopes bytea; -- JSON array of strings`,


@srenatus i took a stab at updating the storage connectors with my changes. tests are passing now, but one questionable bit is this migration. seems like the convention is to make all columns not null across every table, but i ran into a bit of a conflict here.

postgresql won't let you add a not null column to a table without specifying a default, however mysql won't let you specify a default on a BLOB column (what bytea translates to in the mysql flavor). so.... i just let this column be nullable. not sure if there's any potential issues with that.

We'd expect most clients not to have a SAML config, wouldn't we? So, null seems alright for that...? 🤔

srenatus · 2019-08-29T14:26:58Z

Sorry for radio silence -- I have not forgotten about this. (I still want this! 😄)
Will review soonish.

srenatus

This looks pretty good. As we've discussed already, this goes pretty deep into the service, but SAML has broken the connector abstraction right from the start. (Also, IdP-initiated logins seem to be SAML-only feature, given our set of connectors, so I suppose this is OK.)

What I'd like to get in with this is, if you don't mind,

docs
some tests

Now, this doesn't have to go as far as SAML test scaffolding #1295; but maybe there must be some things we could test here...?

Also: Thank you for working on this! It's great to see this happen 🎉

srenatus · 2019-09-06T12:31:04Z

server/handlers.go

+		return "", "", http.StatusBadRequest, errors.New("user session error")
+	}
+	// TODO: should we check that 'idtoken' is one of the scopes? or just put the
+	// burden on the user to confiugre these scopes correctly?


Suggested change

// burden on the user to confiugre these scopes correctly?

// burden on the user to configure these scopes correctly?

srenatus · 2019-09-06T12:34:24Z

storage/kubernetes/types.go

@@ -167,6 +167,14 @@ type Client struct {

 	Name    string `json:"name,omitempty"`
 	LogoURL string `json:"logoURL,omitempty"`
+
+	SAMLInitiated SAMLInitiatedConfig `json:"samlInitiated"`


❓ How about adding ,omitempty? I suspect most people won't use this right away... 🤔

srenatus · 2019-09-06T12:35:54Z

storage/sql/migrate.go

+				add column saml_init_redirect_uri text not null default '';`,
+			`
+			alter table client
+				add column saml_init_scopes bytea; -- JSON array of strings`,


We'd expect most clients not to have a SAML config, wouldn't we? So, null seems alright for that...? 🤔

srenatus · 2019-09-06T12:36:52Z

storage/sql/migrate.go

+			alter table client
+				add column saml_init_redirect_uri text not null default '';`,
+			`
+			alter table client


I think these could be combined into a single statement, ALTER TABLE client ADD COLUMN ..., ADD COLUMN. Not sure if MySQL prohibits this?

i'm not sure.. tbh i just followed the structure of the refresh_token migration above it which does three alters each in a separate statement.

scotthew1 · 2019-09-24T21:10:20Z

hey @srenatus i updated with docs and fixes to some minor issues.

i took a look at what might be needed to add some tests for handleSAMLCallback() and it seems kinda difficult to do much without addressing #1295. TestHandleInvalidSAMLCallbacks in handlers_test.go already covers some of the negative testing, but adding more would require a more elaborate saml connector setup with the test server.

pschaumburg · 2019-12-18T13:34:40Z

@scotthew1 @srenatus
Is there any update on this? :)

mhaddon · 2020-03-18T17:30:59Z

Is there anything we can do to assist in the delivery of this feature?

Saqibm128 · 2020-03-24T21:22:59Z

same, i would be interested in this feature as well for our organization

bonifaido · 2020-03-25T08:04:58Z

First of all this branch has to be rebased on current master since some things around storage and CI have changed. If someone could give a stab at trying to resurrect this branch that would be awesome!

scotthew1 · 2020-03-25T15:48:27Z

hey i just rebased the branch. in regards to what's need for a merge... i can throw some docs together in the near future, but i'm not sure about the testing question. never really got a response on that one and iirc there were some requirements on another issue.

ghost · 2020-08-14T15:26:03Z

any plans to merge this?

srenatus · 2020-10-12T18:19:20Z

Sorry for having dropped the ball on this. Anyone listening? I'll try to re-review when this is rebased! 😃

heidemn-faro · 2021-02-05T06:40:26Z

@srenatus @scotthew1 I didn't rebase, but merged it with recent master. Still worked for me (tested with Okta).
Feel free to continue. Our company will most likely not need this, so I'm currently not motivated to continue myself.
faro-oss#22
https://github.com/faro-oss/dex/tree/saml-idp-initiated

karaimin · 2023-02-23T12:13:49Z

What is the status for this PR ? Any plans for moving ahead ?

erhudy · 2023-11-27T23:42:58Z

What is the status for this PR ? Any plans for moving ahead ?

This PR is of interest to me (specifically doing provider-initiated flows from Okta to Argo CD), so I am working on resolving the merge conflicts and getting it to build and pass tests again. Since this PR is obviously from a fork that I do not own, I will open a separate PR once I have it in an appropriate state.

This feature rebases from the current master branch to include PR dexidp#1514.

srenatus reviewed Aug 9, 2019

View reviewed changes

scotthew1 force-pushed the saml-idp-initiated branch from 876b2d4 to f1f52c4 Compare August 15, 2019 19:56

scotthew1 force-pushed the saml-idp-initiated branch 3 times, most recently from 0f671ac to 36b413e Compare August 20, 2019 15:44

scotthew1 commented Aug 20, 2019

View reviewed changes

scotthew1 force-pushed the saml-idp-initiated branch 2 times, most recently from 621ae04 to 9ac5a96 Compare August 22, 2019 14:49

srenatus mentioned this pull request Sep 6, 2019

IdP-initiated sign in chef/automate#1445

Closed

srenatus reviewed Sep 6, 2019

View reviewed changes

susanev mentioned this pull request Jan 15, 2020

[automate-2530] document supported LDAP/SAML configurations chef/automate#2636

Merged

4 tasks

scotthew1 added 7 commits March 25, 2020 11:41

support for saml idp initiated flow

710d5ff

initial custom IdP SAML scope changes

823ada6

updated storage connectors for client changes (needs testing)

574b930

update conformance test, fix sql storage

372f03b

update api test

21ab763

small changes from review

104d398

add documentation for SAML IdP initiated flow

4a178b8

scotthew1 force-pushed the saml-idp-initiated branch from 9d63128 to 4a178b8 Compare March 25, 2020 15:43

fix renderError call

8738800

MostafaAmer2200 pushed a commit to MostafaAmer2200/dex that referenced this pull request Dec 28, 2023

feat: rebase saml idp flow feature

de17714

This feature rebases from the current master branch to include PR dexidp#1514.

MostafaAmer2200 pushed a commit to MostafaAmer2200/dex that referenced this pull request Dec 28, 2023

feat: rebase saml idp flow feature

3a05f4a

This feature rebases from the current master branch to include PR dexidp#1514.

MostafaAmer2200 mentioned this pull request Dec 28, 2023

SAML Initiated Flow #3250

Open

oleksandr-gubchenko mentioned this pull request Feb 9, 2024

dex + okta login disaplay "User session error. argoproj/argo-cd#16686

Open

	authID, samlInResponseTo, code, err = s.handleSamlCallback(r)
	authID, samlInResponseTo, code, err = s.handleSAMLCallback(r)

	// new auth request in the database so we can contuine with our regular callback flow.
	// new auth request in the database so we can continue with our regular callback flow.

	return "", "", http.StatusBadRequest, fmt.Errorf("user session error")
	return "", "", http.StatusBadRequest, errors.New("user session error")

	// burden on the user to confiugre these scopes correctly?
	// burden on the user to configure these scopes correctly?

support for saml idp initiated flow #1514

Are you sure you want to change the base?

support for saml idp initiated flow #1514

Conversation

scotthew1 commented Aug 8, 2019

srenatus commented Aug 9, 2019

srenatus 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

scotthew1 Aug 9, 2019 • edited Loading

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

scotthew1 commented Aug 13, 2019

srenatus commented Aug 15, 2019

scotthew1 commented Aug 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus commented Aug 29, 2019

srenatus left a comment • edited Loading

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

scotthew1 commented Sep 24, 2019

pschaumburg commented Dec 18, 2019

mhaddon commented Mar 18, 2020

Saqibm128 commented Mar 24, 2020

bonifaido commented Mar 25, 2020

scotthew1 commented Mar 25, 2020

ghost commented Aug 14, 2020

srenatus commented Oct 12, 2020

heidemn-faro commented Feb 5, 2021

karaimin commented Feb 23, 2023

erhudy commented Nov 27, 2023

scotthew1 Aug 9, 2019 •

edited

Loading

srenatus left a comment •

edited

Loading