Skip to content

Changes for tctl sso test, tctl sso configure commands [SAML]#11508

Merged
Tener merged 61 commits into
masterfrom
tener/tctl-sso
May 4, 2022
Merged

Changes for tctl sso test, tctl sso configure commands [SAML]#11508
Tener merged 61 commits into
masterfrom
tener/tctl-sso

Conversation

@Tener
Copy link
Copy Markdown
Contributor

@Tener Tener commented Mar 28, 2022

These are necessary changes to support tctl sso test and (to a much smaller degree) tctl sso configure command.

RFD: tctl sso configure command: #9845
RFD: tctl sso test command: #9775

See: #9270 for original issue, which covers larger scope: SAML, OIDC and GitHub auth connectors. This PR is only touching on SAML, but the implementations for OIDC and GitHub should be parallel to this one.

Actual commands for SAML are implemented in: https://github.com/gravitational/teleport.e/pull/425

Webapps PR: gravitational/webapps#717

@Tener Tener marked this pull request as ready for review March 29, 2022 14:47
@github-actions github-actions Bot added the audit-log Issues related to Teleports Audit Log label Mar 29, 2022
@github-actions github-actions Bot requested review from atburke and rosstimothy March 29, 2022 14:48
@Tener Tener requested review from r0mant and smallinsky March 29, 2022 14:48
@Tener
Copy link
Copy Markdown
Contributor Author

Tener commented Mar 29, 2022

Dear reviewers, but especially @r0mant @smallinsky : please take a close look at the security angle, including the topics mentioned in the respective RFDs. Thanks!

@Tener Tener changed the title Changes for tctl sso test, tctl sso configure commands Changes for tctl sso test, tctl sso configure commands [SAML] Mar 31, 2022
@Tener Tener added the sso Used for single sign on related tasks. label Mar 31, 2022
Copy link
Copy Markdown
Contributor

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass, Left some comments.

Comment thread api/types/sso.go Outdated
Comment thread lib/auth/apiserver.go Outdated
Comment thread lib/auth/apiserver.go Outdated
Comment thread lib/web/apiserver.go Outdated
Comment thread lib/services/identity.go Outdated
Comment thread lib/auth/saml.go Outdated
Comment thread lib/auth/saml.go Outdated
Comment thread lib/web/saml.go
Comment thread lib/client/redirect.go Outdated
Comment thread lib/auth/github.go Outdated
@Tener Tener requested a review from smallinsky April 5, 2022 13:21
@Tener
Copy link
Copy Markdown
Contributor Author

Tener commented Apr 22, 2022

I left a few more minor suggestions, the implementation is looking good to me now! But I think this is still missing test coverage?

@r0mant thanks for the review, I'll add test coverage next.

Tener and others added 8 commits April 22, 2022 09:18
Co-authored-by: Roman Tkachenko <roman@goteleport.com>
- Rename auth.AssertionInfoWrapper to shorter auth.AssertionInfo.
- Add bool TestFlow to SSODiagnosticInfo
- Make SSODiagnosticInfo.Success a bool instead of string.
- Rename SAMLAttributesToRolesWarnings to more generic SSOWarnings
- Add godocs in several places.
- Avoid explicit call to trace.AddUserMessage() where possible.
@Tener
Copy link
Copy Markdown
Contributor Author

Tener commented Apr 25, 2022

@r0mant I've added test coverage for the critical pieces. Let me know if you'd like me to add more coverage anywhere.

@Tener Tener requested a review from r0mant April 26, 2022 14:57
Copy link
Copy Markdown
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tener Ship it! But after 2nd approval :)

@Tener
Copy link
Copy Markdown
Contributor Author

Tener commented Apr 28, 2022

@Tener Ship it! But after 2nd approval :)

Awesome, thanks for approval.

@smallinsky PTAL?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit-log Issues related to Teleports Audit Log sso Used for single sign on related tasks.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants