SAML test scaffolding #1295
Comments
|
I'd be happy to help and willing to make any changes to lite-idp required to facilitate testing. |
|
|
|
@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? |
|
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 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.
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? |
|
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 ***@***.***> 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
<#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
<#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 <#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
<#1295 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AArhFMQ1gLaj1cUMZe4eOCPHKEXxIVGyks5uZlVAgaJpZM4Wg234>
.
|
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. |
|
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 ***@***.***> 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
<#1295 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AArhFG2TcGo9iAr_Lquy0kyGGnpG1RiWks5uaBUwgaJpZM4Wg234>
.
|
|
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.. |
|
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 ***@***.***> 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
<#1295 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AArhFCAcMK8sHeTcFkS93mewnDELzBLSks5uaO64gaJpZM4Wg234>
.
|
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
and of course the feature-full, not-meant-for-dev-only projects, like
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.
The text was updated successfully, but these errors were encountered: