Core changes for "tctl sso test" command (SAML only).#10393
Conversation
|
|
||
| // try to recover diagnostic info from proxy error | ||
| var di *SsoDiagnosticInfo | ||
| di = nil |
There was a problem hiding this comment.
Di is already nil you don't need to reset it again.
| di = nil | ||
|
|
||
| var ssoDi SsoDiagnosticInfo | ||
| if trace.UnwrapProxyField(err, "sso-diag-info", &ssoDi) { |
There was a problem hiding this comment.
UnwrapProxyField returns boolean, not an error?
| di.SetProblem("Failed to parse SAML response", err) | ||
| return nil, di, trace.Wrap(err) |
There was a problem hiding this comment.
Does the di needs to be returned with err != nil
What about decoupling error message and successful fileds from SsoDiagnosticInfo
and proagating error to the http caller by return nil, trace.Wrap(err).WithField(SSODiagnosticErrors, di)
There was a problem hiding this comment.
Does the di needs to be returned with err != nil
Yeah, it does, that is part of the difficulty here. For example, even though the user has logged in successfully with SSO they may still be interested in the list of roles they were assigned along with the claims returned from IdP. The point of having SsoDiagnosticInfo is so we don't have to go around hunting for the data that we need. The UnwrapProxyField is really an unfortunate corner case rather than something I'd like to use elsewhere.
| response, di, err := auth.ValidateSAMLResponse(req.Response) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err) | ||
| return nil, trace.Wrap(err).AddField("sso-diag-info", di) |
There was a problem hiding this comment.
Can di field can be propagated by the auth.ValidateSAMLResponse handler. So the auth.ValidateSAMLResponse will return only response and err where err will have preset diagnostic ErrorField
|
If this involved interacting with any SAML response data from a third party IdP like Azure AD (which it seems to do), please make sure that this also works when Azure AD Encrypted SAML is enabled. That part of the standard sometimes behaves wierdly and usually needs to be manually tested when making SAML changes. |
Thanks for the tip, I'll make sure to go through that flow. |
|
Switching to another PR: #11508 |
3 participants
These are the changes required for "tctl sso test" command. For now only implemented for SAML.
See: #9270 for original issue.
Actual command: https://github.com/gravitational/teleport.e/pull/398
Changes to trace library: gravitational/trace#74