SAML Artifact Binding Support

[amdonov]

This issue relates to #1045. The Artifact Binding is very much like the oauth 2 server-side flow and can be implemented as a redirect connector if RelayState is an acceptable state query parameter in addition to state. Unlike the redirect Binding the SAML assertion isn't limited by query size. Another nice feature is that it is retrieved directly from the IdP and is more secure. The go SAML library currently used in dex doesn't support Artifact Binding. This one includes an alternative that I wrote that has been used in production for some time. I didn't vendor it in this commit.

I'd like this to be included as an experimental alternative to the existing SAML connector for others to try out.

[ericchiang]

Same issue as #1045. Any good way to test this?

[amdonov]

I tested it by running a local IdP with ArtifactBinding support. It's a bit tricky to unit test given the need to make a HTTP call to resolve the artifact. If that's a deal-breaker, I can refactor a bit to get something going.

I also went ahead and submitted a pull request to the SAML 2 library you're using to include artifact binding support, russellhaering/gosaml2#46. If that's accepted, I'll update this to avoid needing another library.

[ericchiang]

I tested it by running a local IdP with ArtifactBinding support.

Which local IdP? If there's not a good option for future dex maintainers to test this it's hard for us to maintain the feature :)

[amdonov]

I tested this with https://github.com/amdonov/lite-idp. The SP code has also been tested with Shibboleth. I can write up setup steps if you'd like.

[ericchiang]

Before you add more code to this PR, can you take a second to explain why you want to do this? SAML is a tire fire, and I'm skeptical that adding more SAML features to dex is going to be maintainable long term.

Does this make dex work with more providers? Is there an extremely common provider that only implements this?

[amdonov]

Sure, reasonable question. However, I think most of the hassles associated with SAML are related to the POST binding which I try to avoid. Outside of that the flow is pretty much like OAuth with more complicated messages.

I have a client with a highly customized SAML IdP. They want to move towards OIDC/OAuth for obvious reasons, but they don't want to break existing applications. Adding dex do their environment and integrating via SAML minimizes their need for change. They prefer Artifact Binding to POST for a couple of reasons. 1) If someone links to an image on their page they can login them in with redirects alone. POSTs don't work for those types of resources 2) Scripting clients is easier for them because code just has to accept cookies and follow redirects.

My hope was to get this accepted by either including the SAML library that we use or updating the one you use.

Next step would be to add SAML Attribute Query to allow refresh.

[ericchiang]

Sure, reasonable question. However, I think most of the hassles associated with SAML are related to the POST binding which I try to avoid. Outside of that the flow is pretty much like OAuth with more complicated messages.

We've had vulnerabilities in the XML parsing and signature validation too. All the optional signing is just as insane :)

I have a client with a highly customized SAML IdP. They want to move towards OIDC/OAuth for obvious reasons, but they don't want to break existing applications. Adding dex do their environment and integrating via SAML minimizes their need for change. They prefer Artifact Binding to POST for a couple of reasons. 1) If someone links to an image on their page they can login them in with redirects alone. POSTs don't work for those types of resources 2) Scripting clients is easier for them because code just has to accept cookies and follow redirects.

I think this is a bad reason to add it to dex. We're trying to tradeoff the amount of pain to maintain a feature with how broad of an open source audience it reaches. A SAML feature that's limited to a customized IdP leans heavily towards a lot of pain and a small audience.

[amdonov]

I totally understand the desire to not take on additional connectors, but I think you should change dex to make it more embeddable and extensible if that's the case.

Right now adding a new connector to create a custom distribution of dex requires a bunch of code. I'd like to see two changes made. 1) Open up the Server struct to allow specification of connectors, storage, etc. 2) Move dex's cobra.Command to a package other than main and make it public, so it can be imported and added to other projects.

If you'd entertain a pull request like that, I'll submit something.

[ericchiang]

Changes like this that require modifying dex's server aren't trivial to support in an extensible way. We've got an auth proxy mode for most OAuth2 stuff, but I don't think making the CLI importable is going to solve this case.

https://github.com/dexidp/dex/blob/master/Documentation/connectors/authproxy.md

[amdonov]

Thanks for considering.