Add ssoIssuer to fix Response issuer checking #885
Conversation
|
Just realized we already have AudienceRestriction checking in https://github.com/coreos/dex/blob/master/connector/saml/saml.go#L462. Should we remove the checking altogether? Edit: Decided to add optional ssoIssuer to perform checking on. What do you guys think? |
Calpicow
force-pushed
the
saml_issuer_fix
branch
from
f77f7a7 to
29bad37
Compare
Calpicow
changed the title
connector/saml/saml.go
Outdated
| SSOURL string `json:"ssoURL"` | ||
| // Issuer refers to the SP issuer | ||
| Issuer string `json:"issuer"` | ||
| SSOIssuer string `json:"ssoIssuer"` |
There was a problem hiding this comment.
I'm a little confused by this fix. What's SSOIssuer? How is it different than issuer? What part of the spec is enforcing this?
There was a problem hiding this comment.
Refer to the issue I filed. In a nutshell, there appears to be some confusion between the SP issuer and IDP issuer.
There was a problem hiding this comment.
Okay thanks. Give me a few to read up on your linked article.
There was a problem hiding this comment.
Any updates? We don't really need to add this field, and just remove the Response issuer checking if you like.
There was a problem hiding this comment.
Sorry, we've been dealing with a security issue in this connector. Please see https://groups.google.com/forum/#!topic/coreos-user/s_BWh4a3E1Y
| } | ||
| if redirectURI == "" { | ||
| redirectURI = defaultRedirectURI | ||
| } | ||
| now, _ := time.Parse(time.RFC3339, "2017-01-24T20:48:41Z") | ||
| timeFunc := func() time.Time { return now } | ||
| return &provider{ | ||
| issuer: issuer, |
There was a problem hiding this comment.
Sorry, trying to re-review this. Issuer is no longer being used here then, correct? I was under the impression that issuer was supposed to be the value that you've changed to ssoIssuer to anyway. E.g. that issuer should just be http://www.okta.com/exk91cb99lKkKSYoy0h7.
Maybe this an issue with the documentation?
There was a problem hiding this comment.
Ah, nvm I see what you're saying.
Calpicow
force-pushed
the
saml_issuer_fix
branch
from
29bad37 to
2ee512e
Compare
|
Couple comments: So we might want to rename issuer in this case, since it has different meanings for OpenID Connect. Issuer means the remote provider, not the dex server. Maybe Also is this always going to be the same or different then redirectURI? |
Calpicow
force-pushed
the
saml_issuer_fix
branch
from
4b580ae to
bcb0cc4
Compare
|
It is possible for issuer it to be different than redirectURI. I've avoided renaming issuer since it would be a breaking change, but I'm open to that. |
We've marked it experimental explicitly so we can do these kind of breaking changes :) |
|
(to give a timeline, we'd like to do one final push over the next couple days to get it to stable) |
Calpicow
force-pushed
the
saml_issuer_fix
branch
from
bcb0cc4 to
b3d95c3
Compare
Rename issuer to entityIssuer
Calpicow
force-pushed
the
saml_issuer_fix
branch
from
b3d95c3 to
8c0eb67
Compare
|
Done |
Add ssoIssuer to fix Response issuer checking
Fixes #884