Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possibly allow unspecified InResponseTo SAML attribute #1479

Open
oakad opened this issue Jul 3, 2019 · 6 comments
Open

Possibly allow unspecified InResponseTo SAML attribute #1479

oakad opened this issue Jul 3, 2019 · 6 comments

Comments

@oakad
Copy link

oakad commented Jul 3, 2019

InResponseTo attribute is defined as "optional" in the SAML spec. Indeed, other SAML libraries usually allow for "less strict" mode of operation, whereupon InResponseTo is only validated when it is actually present in the message, but is allowed not to be (because "optional").

I'm dealing with an Okta setup which appears not to include the InResponseTo attribute in its SAML response. This works with some SAML libraries, but not with Dex, because Dex always expects this attribute to be present.

Related to #869.

@srenatus
Copy link
Contributor

srenatus commented Jul 3, 2019

I'm dealing with an Okta setup which appears not to include the InResponseTo attribute in its SAML response. This works with some SAML libraries, but not with Dex, because Dex always expects this attribute to be present.

Note that there was some discussion re: using an existing SAML library instead of our handcrafted code in #1295.

That said, I've been using Dex with Okta successfully -- The only scenario where it's lacking the InResponseTo was if you use IdP-initiated login, i.e., click on the icon in the Okta overview. That, however, won't work, since Dex expects an auth request to have been created (what it does when you go to dex and choose to login using SAML)... That is not what you're doing, is it? 😃

@oakad
Copy link
Author

oakad commented Jul 4, 2019

No, I'm not using IdP. :-)

What I'm seeing here is somewhat strange. We have multiple unrelated services sharing a common Okta auth. If I login to some other service and then use the Dex enabled one, the InResponseTo attribute is always there and Dex does it job without issues.

However, if I try to login to the Dex enabled service first (without valid Okta session state in the browser), then everything appears to work the same, apart from InResponseTo attribute missing in the SAML message. Yet, it is totally not obvious to me why would Okta decide to omit this attribute when initializing a new session, but happily includes it for established ones.

@srenatus
Copy link
Contributor

srenatus commented Jul 4, 2019

Sounds like something you could ask okta's support about... :thinking_face:

Also, I think on the long run, we'd all be better off to use oidc with okta. Have you tried that? Okta supports that but I don't know if our connector has feature parity. It could be a much better solution in theory, since oidc could do refresh tokens... Our oidc connector doesn't yet, though, I think there's WIP for that.

@srenatus
Copy link
Contributor

So, the problem with a missing InResponseTo is that we use it to match auth requests. If you have two SAML connectors set up, and you receive a SAML assertion without InResponseTo, which connector was it for? I'm not saying that it is too difficult to figure that out, it's just that we'll have to deal with this to implement this.

I think figuring out the connector that way would also allow for IdP-initated login for SAML. Wouldn't be a bad feature, I guess 😄

@srenatus
Copy link
Contributor

❌ I was wrong. We use the RelayState for matching auth requests,

dex/server/handlers.go

Lines 402 to 406 in 421c26f

case http.MethodPost: // SAML POST binding
if authID = r.PostFormValue("RelayState"); authID == "" {
s.renderError(w, http.StatusBadRequest, "User session error.")
return
}

@oakad
Copy link
Author

oakad commented Jul 24, 2019

In our tests RelayState comes back just fine - the InResponseTo is supposed to be an optional safeguard mechanism.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants