support for saml idp initiated flow

[scotthew1]

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:

1. 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.
2. 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.
3. 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.
4. 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]

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

[scotthew1]

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]

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 :+1:

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]

@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!

[srenatus]

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

[scotthew1]

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]

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

[mhaddon]

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

[Saqibm128]

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

[bonifaido]

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]

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]

any plans to merge this?

[srenatus]

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

[heidemn-faro]

@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. https://github.com/faro-oss/dex/pull/22 https://github.com/faro-oss/dex/tree/saml-idp-initiated

[karaimin]

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

[erhudy]

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.