SAML test scaffolding

[srenatus]

In various SAML-related PRs, it became apparent that out lack of automated tests for the SAML connector a hurdle to developing and maintaining that code (see for example #1045, #1175, #1282).

👉 The LDAP connector spins up an LDAP server (openldap) in the travis tests.

It would be great if we had a similar thing going for SAML. There seem to be a few applicable projects to plug into our tests, like

• https://github.com/OpenConext/Mujina

and of course the feature-full, not-meant-for-dev-only projects, like

• https://github.com/amdonov/lite-idp (@amdonov 👋)
• https://simplesamlphp.org/

While the artifact binding seems to be controversial, I don't think the redirect binding is -- so any project used should perhaps support that, so we don't restrict ourselves for no good reason.

[amdonov]

I'd be happy to help and willing to make any changes to lite-idp required to facilitate testing.

[srenatus]

~@amdonov would you mind if I assigned this to you then? 😉~ issues can only be assigned if part of the repo or something. Sorry for the noise.

[amdonov]

@srenatus I looked at this briefly this morning. My initial inclination would be to beef up an external SAML service provider implementation like https://github.com/russellhaering/gosaml2 and integrate that with dex. It would probably get more use and testing in other scenarios and potentially benefit other users/projects.

Not committing to using that specific library, but would you be open to that type of approach?

[srenatus]

So, rephrasing to ensure I've grokked it, your proposal would be to replace the current SAML connector with one based on a purpose-built SAML library (over the current code which uses libraries for making sense of XML and the signature verification).

(Assuming that's it, ) I think that's a good idea. The only concern I'd have is backwards-compatibility: it seems like either, we aim for backwards-compat (for example by replacing the current saml connector), or we introduce a new connector (I've seen the idea of having a saml2 floating around in some PR already 😉 (⏩ #1282)).

If we went with the first option, I'd suppose we might first get tests in place to latter know that we've kept our backwards-compat goals when swapping the connector.

Glancing at the gosaml2 lib, it seems like this would unlock further improvements: we might be able to let our users provide merely the SAML metadata (and certs probably) and they'd be good to go. 😍 (I haven't looked close enough to tell if it was able to do metadata refresh, too, like OIDC does for key rotation. That would be awesome.)

I think I'd lean towards the first option, though -- if only because the SAML information exchange handling currently already is a special path in the code. I would think that adding another "saml2" path there would complicate things further.

Not committing to using that specific library, but would you be open to that type of approach?

I think we should perhaps look into the discussion of #1282 again; and see if we're avoiding the concerns here. From my perspective -- I certainly don't love SAML either, but we, too, have customers that want it... (and the Redirect Binding (like #1175) is more interesting for us than the Artifact Binding). In the interest of a secure product, I think using a libraries that other people use, too, is preferable.

If we get this done in a way that's consumable in smaller pieces, and if it included a similar testing regime as used for LDAP, I suppose this could be OK -- what do you think, @ericchiang?

[amdonov]

My preference would be to maintain compatibility in the existing connector rather than introduce a new one. I took that approach initially for expediency and my specific requirement for artifact binding. The lite-idp service provider that I used on #1282 doesn't currently meet all of your requirements.

On Mon, Sep 10, 2018 at 8:04 AM Stephan Renatus notifications@github.com wrote:

So, rephrasing to ensure I've grokked it, your proposal would be to replace the current SAML connector with one based on a purpose-built SAML library (over the current code which uses libraries https://github.com/dexidp/dex/blob/666356d22d9c83f764045340d956e4f2213540ae/connector/saml/saml.go#L15-L17 for making sense of XML and the signature verification).

(Assuming that's it, ) I think that's a good idea. The only concern I'd have is backwards-compatibility: it seems like either, we aim for backwards-compat (for example by replacing the current saml connector), or we introduce a new connector (I've seen the idea of having a saml2 floating around in some PR already 😉 (⏩ #1282 https://github.com/dexidp/dex/pull/1282)).

If we went with the first option, I'd suppose we might first get tests in place to latter know that we've kept our backwards-compat goals when swapping the connector.

Glancing at the gosaml2 lib, it seems like this would unlock further improvements: we might be able to let our users provide merely the SAML metadata (and certs probably) and they'd be good to go. 😍 (I haven't looked close enough to tell if it was able to do metadata refresh, too, like OIDC does for key rotation. That would be awesome.)

I think I'd lean towards the first option, though -- if only because the SAML information exchange handling currently already is a special path in the code. I would think that adding another "saml2" path there would complicate things further.

Not committing to using that specific library, but would you be open to that type of approach?

I think we should perhaps look into the discussion of #1282 https://github.com/dexidp/dex/pull/1282 again; and see if we're avoiding the concerns here. From my perspective -- I certainly don't love SAML either, but we, too, have customers that want it... (and the Redirect Binding (like #1175 https://github.com/dexidp/dex/pull/1175) is more interesting for us than the Artifact Binding). In the interest of a secure product, I think using a libraries that other people use, too, is preferable.

If we get this done in a way that's consumable in smaller pieces, and if it included a similar testing regime as used for LDAP, I suppose this could be OK -- what do you think, @ericchiang https://github.com/ericchiang?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dexidp/dex/issues/1295#issuecomment-419888956, or mute the thread https://github.com/notifications/unsubscribe-auth/AArhFMQ1gLaj1cUMZe4eOCPHKEXxIVGyks5uZlVAgaJpZM4Wg234 .

[ericchiang]

If we get this done in a way that's consumable in smaller pieces, and if it included a similar testing regime as used for LDAP, I suppose this could be OK -- what do you think, @ericchiang?

I'm always for more testing.

We'd probably need to expand https://github.com/dexidp/dex/blob/master/server/server_test.go to get a full server up and running for these kind of tests.

A good start might be to move server_test.go to it's own integration test directory (and maybe clean it up). I think shibboleth would be a good framework to look into since it's open source and popular.

[amdonov]

I have a ton of experience with Shibboleth. I agree that it is a very standards compliant implementation, but it's a bear to run. That's the whole reason I wrote lite-idp. I'd recommend capturing requests/responses with Shibboleth and replicating them with lite-idp. Then, use gosaml2 for your connector. They use totally different XML encryption libraries while both being very lightweight.

On Tue, Sep 11, 2018 at 3:55 PM Eric Chiang notifications@github.com wrote:

If we get this done in a way that's consumable in smaller pieces, and if it included a similar testing regime as used for LDAP, I suppose this could be OK -- what do you think, @ericchiang https://github.com/ericchiang?

I'm always for more testing.

We'd probably need to expand https://github.com/dexidp/dex/blob/master/server/server_test.go to get a full server up and running for these kind of tests.

A good start might be to move server_test.go to it's own integration test directory (and maybe clean it up). I think shibboleth would be a good framework to look into since it's open source and popular.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dexidp/dex/issues/1295#issuecomment-420403152, or mute the thread https://github.com/notifications/unsubscribe-auth/AArhFG2TcGo9iAr_Lquy0kyGGnpG1RiWks5uaBUwgaJpZM4Wg234 .

[srenatus]

If we capture shib responses, we'd need to do that in a reproducible way - which might be roughly the same work as spinning up shib in the first place. I suppose if we used any saml2-idp that is standards compliant, we might be fine? (I'm not sure that it must be shib...)

OTOH interesting tests might include the possible differences within the standard. What's signed, for example. Maybe expanding canned responses would be enough for that. The question again would be how to reproducibly generate them..

[amdonov]

Sorry, I wasn't clear. I was just suggesting using the output from Shibboleth responses as the representative example to reproduce because as you said their are differences in the implementations. I don't think it's terribly difficult to reproduce the messages from any one of the providers if there are edge cases that cause people problems.

On Wed, Sep 12, 2018 at 7:23 AM Stephan Renatus notifications@github.com wrote:

If we capture shib responses, we'd need to do that in a reproducible way - which might be roughly the same work as spinning up shib in the first place. I suppose if we used any saml2-idp that is standards compliant, we might be fine? (I'm not sure that it must be shib...)

OTOH interesting tests might include the possible differences within the standard. What's signed, for example. Maybe expanding canned responses would be enough for that. The question again would be how to reproducibly generate them..

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dexidp/dex/issues/1295#issuecomment-420613229, or mute the thread https://github.com/notifications/unsubscribe-auth/AArhFCAcMK8sHeTcFkS93mewnDELzBLSks5uaO64gaJpZM4Wg234 .