connector/saml: implement SP-Initiated Single Logout via LogoutCallbackConnector#4742
connector/saml: implement SP-Initiated Single Logout via LogoutCallbackConnector#4742Jabejixo wants to merge 4 commits into
Conversation
Signed-off-by: Ivan Zvyagintsev <ivan.zvyagintsev@flant.com>
Signed-off-by: Ivan Zvyagintsev <ivan.zvyagintsev@flant.com>
nabokihms
left a comment
nabokihms
left a comment
There was a problem hiding this comment.
Before we merge, two things I'd like us to sort out:
- The outgoing
LogoutRequestisn't signed, and most IdPs reject unsigned ones by default. HandleLogoutCallbackis missing a few checks that login-side already does —InResponseTo,Issuer,Destination. WithoutInResponseTomatching, a captured response could be replayed.
Left some other minor suggestions.
| return fmt.Errorf("saml slo: %w", xrvErr) | ||
| } | ||
|
|
||
| if !p.insecureSkipSLOSignatureValidation { |
There was a problem hiding this comment.
A few checks we do on the login side that I think we want here too:
InResponseTo- otherwise, a captured valid response could be replayedDestinationandIssuer— to prevent recipient/issuer confusion- Freshness on
IssueInstant
InResponseTo is the important one — would need to stash the outgoing request ID in LogoutState. WDYT?
| func newRequestID() string { | ||
| buf := make([]byte, 16) | ||
| if _, err := io.ReadFull(rand.Reader, buf); err != nil { | ||
| panic("crypto/rand failed: " + err.Error()) |
There was a problem hiding this comment.
NIT: most of the connector returns errors rather than panicking. Mind making this (string, error) to match?
| return "", nil | ||
| } | ||
|
|
||
| req := &logoutRequest{ |
There was a problem hiding this comment.
Most IdPs reject unsigned LogoutRequests in production. You've already got the redirect-binding sign logic for the verify path, so reusing it here should be straightforward. Could be a follow-up if we want to keep this PR focused, but probably worth flagging in the docs either way.
Did you get a chance to test against a real IdP? Curious if it accepted the unsigned request. Internally, we discussed that you tested with Keycloak, but...
| nameIDPolicyFormat: c.NameIDPolicyFormat, | ||
|
|
||
| sloURL: c.SLOURL, | ||
| insecureSkipSLOSignatureValidation: c.InsecureSkipSLOSignatureValidation, |
There was a problem hiding this comment.
Is there a case where someone trusts login signatures but not logout (or vice versa)? If not, maybe just reuse the existing InsecureSkipSignatureValidation? Two near-identically-named flags tend to cause support pain.
Signed-off-by: Ivan Zvyagintsev <ivan.zvyagintsev@flant.com>
Signed-off-by: Ivan Zvyagintsev <ivan.zvyagintsev@flant.com>
3 participants
Overview
Add SAML Single Logout (SLO) support to the SAML connector by implementing
the
LogoutCallbackConnectorinterface from #4674.What this PR does / why we need it
The SAML connector does not support Single Logout - when a user logs out
through Dex, the upstream IdP session stays active.
This PR adds two methods to the SAML connector:
LogoutURL- builds a<LogoutRequest>with HTTP-Redirect binding(DEFLATE + base64) using
NameIDandSessionIndexcaptured during login.HandleLogoutCallback- validates the IdP's<LogoutResponse>for bothHTTP-Redirect (GET) and HTTP-POST bindings with binding-aware signature
validation.
New config fields:
sloURL(IdP's SLO endpoint) andinsecureSkipSLOSignatureValidation. Existingca/caDatacertificatesare reused - no additional configuration required.
Special notes for your reviewer
LogoutCallbackConnectorinterface called by
logout.go.NameID,NameIDFormat, andSessionIndexare persisted inConnectorDataduring
HandlePOSTalongside the existing refresh token fields.nameID.FormatXML tag was missingattr— it is anattribute, not a child element.
Signature/SigAlgquery params) isimplemented separately from embedded XML signatures.