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

New commands: tctl sso test, tctl sso configure for GitHub #12783

Merged
merged 31 commits into from
May 26, 2022
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
5ac32cd
Implement `tctl sso` commands for GitHub auth.
Tener May 20, 2022
4f529c7
Small consistency fixes across connector types.
Tener May 20, 2022
de38004
Merge branch 'master' into tener/tctl-sso-gh
Tener May 20, 2022
8a61bf5
Remove unused leftover type declaration.
Tener May 20, 2022
8ec57c7
Add missing formatting directive.
Tener May 20, 2022
5ea3480
Merge branch 'master' into tener/tctl-sso-gh
Tener May 20, 2022
fc54be0
Rename variable.
Tener May 24, 2022
29c52c1
Consistency in pointer use.
Tener May 24, 2022
f776db7
Remove outdated todo.
Tener May 24, 2022
91e07cc
Extract `getSupportedKinds()` method.
Tener May 24, 2022
b5d1ce3
Improve comment.
Tener May 24, 2022
c6f9e2a
Check request for nil.
Tener May 24, 2022
04efb88
Remove superfluous comment.
Tener May 24, 2022
4db5f04
Rename function arg.
Tener May 24, 2022
16b17c0
Merge branch 'master' into tener/tctl-sso-gh
Tener May 24, 2022
292fb72
Make teamsToLoginsParser a struct.
Tener May 24, 2022
ddd7163
Merge branch 'master' into tener/tctl-sso-gh
Tener May 24, 2022
eb234b4
Drop DiagnosticInfoField type.
Tener May 25, 2022
ecffead
Document AuthRequestInfo.
Tener May 25, 2022
d34550d
Extract methods. Fix `--ignore-missing-roles`.
Tener May 25, 2022
0add745
Document `IsCumulative()` method.
Tener May 25, 2022
095595c
Improve readability: extract `printDiagnosticInfo`.
Tener May 25, 2022
2677ab1
Share logic via `ResolveCallbackURL()`.
Tener May 25, 2022
c43577e
Merge branch 'master' into tener/tctl-sso-gh
Tener May 25, 2022
abdf8e9
Call correct function in test.
Tener May 26, 2022
0fc805f
Refactor getGithubOAuth2Client.
Tener May 26, 2022
6d72ed2
Merge branch 'master' into tener/tctl-sso-gh
Tener May 26, 2022
68ffcba
Tweak commands.
Tener May 26, 2022
ff41328
Mark RFDs as implemented.
Tener May 26, 2022
9ed7272
Merge branch 'master' into tener/tctl-sso-gh
Tener May 26, 2022
69166b9
Merge branch 'master' into tener/tctl-sso-gh
Tener May 26, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions api/types/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,6 @@ func NewGithubConnector(name string, spec GithubConnectorSpecV3) (GithubConnecto
return c, nil
}

// GithubClaims represents Github user information obtained during OAuth2 flow
type GithubClaims struct {
// Username is the user's username
Username string
// OrganizationToTeams is the user's organization and team membership
OrganizationToTeams map[string][]string
// Teams is the users team membership
Teams []string
}

// GetVersion returns resource version
func (c *GithubConnectorV3) GetVersion() string {
return c.Version
Expand Down Expand Up @@ -161,6 +151,18 @@ func (c *GithubConnectorV3) CheckAndSetDefaults() error {
if err := c.Metadata.CheckAndSetDefaults(); err != nil {
return trace.Wrap(err)
}

// make sure claim mappings have either roles or a role template
for i, v := range c.Spec.TeamsToLogins {
if v.Team == "" {
return trace.BadParameter("team_to_logins mapping #%v is invalid, team is empty.", i+1)
}
}

if len(c.Spec.TeamsToLogins) == 0 {
return trace.BadParameter("team_to_logins mapping is invalid, no mappings defined.")
}

return nil
}

Expand Down
2,391 changes: 1,546 additions & 845 deletions api/types/types.pb.go

Large diffs are not rendered by default.

34 changes: 34 additions & 0 deletions api/types/types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -2812,6 +2812,40 @@ message SSODiagnosticInfo {
(gogoproto.nullable) = false,
(gogoproto.jsontag) = "oidc_connector_trait_mapping,omitempty"
];

// GithubClaims represents Github user information obtained during OAuth2 flow.
GithubClaims GithubClaims = 30 [ (gogoproto.jsontag) = "github_claims,omitempty" ];

// GithubTeamsToLogins is TeamsToLogins mapping from Github connector used in the SSO flow.
repeated TeamMapping GithubTeamsToLogins = 31
[ (gogoproto.nullable) = false, (gogoproto.jsontag) = "github_teams_to_logins,omitempty" ];

// GithubTokenInfo stores diagnostic info about Github OAuth2 token obtained during SSO flow.
GithubTokenInfo GithubTokenInfo = 32 [ (gogoproto.jsontag) = "github_token_info,omitempty" ];
}

// GithubTokenInfo stores diagnostic info about Github OAuth2 token obtained during SSO flow.
// The token itself is secret and therefore not included.
message GithubTokenInfo {
string TokenType = 1 [ (gogoproto.jsontag) = "token_type" ];
int64 Expires = 2 [ (gogoproto.jsontag) = "expires" ];
string Scope = 3 [ (gogoproto.jsontag) = "scope" ];
}

// GithubClaims represents Github user information obtained during OAuth2 flow
message GithubClaims {
// Username is the user's username
string Username = 1 [ (gogoproto.jsontag) = "username" ];

// OrganizationToTeams is the user's organization and team membership
wrappers.LabelValues OrganizationToTeams = 2 [
(gogoproto.nullable) = false,
(gogoproto.jsontag) = "organization_to_teams",
(gogoproto.customtype) = "github.com/gravitational/teleport/api/types/wrappers.Traits"
];

// Teams is the users team membership
repeated string Teams = 3 [ (gogoproto.jsontag) = "teams" ];
}

// TeamMapping represents a single team membership mapping.
Expand Down
11 changes: 10 additions & 1 deletion lib/auth/apiserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ func NewAPIServer(config *APIConfig) (http.Handler, error) {
srv.DELETE("/:version/github/connectors/:id", srv.withAuth(srv.deleteGithubConnector))
srv.POST("/:version/github/requests/create", srv.withAuth(srv.createGithubAuthRequest))
srv.POST("/:version/github/requests/validate", srv.withAuth(srv.validateGithubAuthCallback))
srv.GET("/:version/github/requests/get/:id", srv.withAuth(srv.getGithubAuthRequest))

// SSO diag info
srv.GET("/:version/sso/diag/:auth/:id", srv.withAuth(srv.getSSODiagnosticInfo))
Expand Down Expand Up @@ -1589,6 +1590,14 @@ func (s *APIServer) createGithubAuthRequest(auth ClientI, w http.ResponseWriter,
return response, nil
}

func (s *APIServer) getGithubAuthRequest(auth ClientI, w http.ResponseWriter, r *http.Request, p httprouter.Params, version string) (interface{}, error) {
request, err := auth.GetGithubAuthRequest(r.Context(), p.ByName("id"))
if err != nil {
return nil, trace.Wrap(err)
}
return request, nil
}

// validateGithubAuthCallbackReq is a request to validate Github OAuth2 callback
type validateGithubAuthCallbackReq struct {
// Query is the callback query string
Expand Down Expand Up @@ -1626,7 +1635,7 @@ func (s *APIServer) validateGithubAuthCallback(auth ClientI, w http.ResponseWrit
if err := httplib.ReadJSON(r, &req); err != nil {
return nil, trace.Wrap(err)
}
response, err := auth.ValidateGithubAuthCallback(req.Query)
response, err := auth.ValidateGithubAuthCallback(r.Context(), req.Query)
if err != nil {
return nil, trace.Wrap(err)
}
Expand Down
10 changes: 9 additions & 1 deletion lib/auth/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -920,7 +920,15 @@ func TestGithubConnectorCRUDEventsEmitted(t *testing.T) {

ctx := context.Background()
// test github create event
github, err := types.NewGithubConnector("test", types.GithubConnectorSpecV3{})
github, err := types.NewGithubConnector("test", types.GithubConnectorSpecV3{
TeamsToLogins: []types.TeamMapping{
{
Organization: "octocats",
Team: "dummy",
Logins: []string{"dummy"},
},
},
})
require.NoError(t, err)
err = s.a.upsertGithubConnector(ctx, github)
require.NoError(t, err)
Expand Down
23 changes: 19 additions & 4 deletions lib/auth/auth_with_roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -2712,18 +2712,33 @@ func (a *ServerWithRoles) CreateGithubAuthRequest(req services.GithubAuthRequest
return nil, trace.Wrap(err)
}

// require additional permissions for executing SSO test flow.
if req.SSOTestFlow {
if err := a.authConnectorAction(apidefaults.Namespace, types.KindGithub, types.VerbCreate); err != nil {
return nil, trace.Wrap(err)
}
}

githubReq, err := a.authServer.CreateGithubAuthRequest(req)
if err != nil {
// TODO(Tener): Update `testFlow` flag once GitHub SSO starts supporting test flows.
emitSSOLoginFailureEvent(a.authServer.closeCtx, a.authServer.emitter, events.LoginMethodGithub, err, false)
emitSSOLoginFailureEvent(a.authServer.closeCtx, a.authServer.emitter, events.LoginMethodGithub, err, req.SSOTestFlow)
return nil, trace.Wrap(err)
}

return githubReq, nil
}

func (a *ServerWithRoles) ValidateGithubAuthCallback(q url.Values) (*GithubAuthResponse, error) {
return a.authServer.ValidateGithubAuthCallback(q)
// GetGithubAuthRequest returns Github auth request if found.
func (a *ServerWithRoles) GetGithubAuthRequest(ctx context.Context, id string) (*services.GithubAuthRequest, error) {
if err := a.action(apidefaults.Namespace, types.KindGithubRequest, types.VerbRead); err != nil {
return nil, trace.Wrap(err)
}

return a.authServer.GetGithubAuthRequest(ctx, id)
}

func (a *ServerWithRoles) ValidateGithubAuthCallback(ctx context.Context, q url.Values) (*GithubAuthResponse, error) {
return a.authServer.ValidateGithubAuthCallback(ctx, q)
}

// EmitAuditEvent emits a single audit event
Expand Down
177 changes: 176 additions & 1 deletion lib/auth/auth_with_roles_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import (
"testing"
"time"

"golang.org/x/crypto/ssh"

"github.com/gravitational/teleport"
"github.com/gravitational/teleport/api/client/proto"
"github.com/gravitational/teleport/api/constants"
Expand All @@ -37,7 +39,6 @@ import (
"github.com/gravitational/teleport/lib/services"
"github.com/gravitational/teleport/lib/tlsca"
"github.com/gravitational/teleport/lib/utils"
"golang.org/x/crypto/ssh"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
Expand Down Expand Up @@ -629,6 +630,180 @@ func TestOIDCAuthRequest(t *testing.T) {
}
}

func TestGithubAuthRequest(t *testing.T) {
ctx := context.Background()
srv := newTestTLSServer(t)

emptyRole, err := CreateRole(ctx, srv.Auth(), "test-empty", types.RoleSpecV5{})
require.NoError(t, err)

access1Role, err := CreateRole(ctx, srv.Auth(), "test-access-1", types.RoleSpecV5{
Allow: types.RoleConditions{
Rules: []types.Rule{
{
Resources: []string{types.KindGithubRequest},
Verbs: []string{types.VerbCreate},
},
},
},
})
require.NoError(t, err)

access2Role, err := CreateRole(ctx, srv.Auth(), "test-access-2", types.RoleSpecV5{
Allow: types.RoleConditions{
Rules: []types.Rule{
{
Resources: []string{types.KindGithub},
Verbs: []string{types.VerbCreate},
},
},
},
})
require.NoError(t, err)

access3Role, err := CreateRole(ctx, srv.Auth(), "test-access-3", types.RoleSpecV5{
Allow: types.RoleConditions{
Rules: []types.Rule{
{
Resources: []string{types.KindGithub, types.KindGithubRequest},
Verbs: []string{types.VerbCreate},
},
},
},
})
require.NoError(t, err)

readerRole, err := CreateRole(ctx, srv.Auth(), "test-access-4", types.RoleSpecV5{
Allow: types.RoleConditions{
Rules: []types.Rule{
{
Resources: []string{types.KindGithubRequest},
Verbs: []string{types.VerbRead},
},
},
},
})
require.NoError(t, err)

conn, err := types.NewGithubConnector("example", types.GithubConnectorSpecV3{
ClientID: "example-client-id",
ClientSecret: "example-client-secret",
RedirectURL: "https://localhost:3080/v1/webapi/github/callback",
Display: "sign in with github",
TeamsToLogins: []types.TeamMapping{
{
Organization: "octocats",
Team: "idp-admin",
Logins: []string{"access"},
},
},
})
require.NoError(t, err)

err = srv.Auth().UpsertGithubConnector(context.Background(), conn)
require.NoError(t, err)

reqNormal := services.GithubAuthRequest{ConnectorID: conn.GetName(), Type: constants.Github}
reqTest := services.GithubAuthRequest{ConnectorID: conn.GetName(), Type: constants.Github, SSOTestFlow: true, ConnectorSpec: &types.GithubConnectorSpecV3{
ClientID: "example-client-id",
ClientSecret: "example-client-secret",
RedirectURL: "https://localhost:3080/v1/webapi/github/callback",
Display: "sign in with github",
TeamsToLogins: []types.TeamMapping{
{
Organization: "octocats",
Team: "idp-admin",
Logins: []string{"access"},
},
},
}}

tests := []struct {
desc string
roles []string
request services.GithubAuthRequest
expectAccessDenied bool
}{
{
desc: "empty role - no access",
roles: []string{emptyRole.GetName()},
request: reqNormal,
expectAccessDenied: true,
},
{
desc: "can create regular request with normal access",
roles: []string{access1Role.GetName()},
request: reqNormal,
expectAccessDenied: false,
},
{
desc: "cannot create sso test request with normal access",
roles: []string{access1Role.GetName()},
request: reqTest,
expectAccessDenied: true,
},
{
desc: "cannot create normal request with connector access",
roles: []string{access2Role.GetName()},
request: reqNormal,
expectAccessDenied: true,
},
{
desc: "cannot create sso test request with connector access",
roles: []string{access2Role.GetName()},
request: reqTest,
expectAccessDenied: true,
},
{
desc: "can create regular request with combined access",
roles: []string{access3Role.GetName()},
request: reqNormal,
expectAccessDenied: false,
},
{
desc: "can create sso test request with combined access",
roles: []string{access3Role.GetName()},
request: reqTest,
expectAccessDenied: false,
},
}

user, err := CreateUser(srv.Auth(), "dummy")
require.NoError(t, err)

userReader, err := CreateUser(srv.Auth(), "dummy-reader", readerRole)
require.NoError(t, err)

clientReader, err := srv.NewClient(TestUser(userReader.GetName()))
require.NoError(t, err)

for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
user.SetRoles(tt.roles)
err = srv.Auth().UpsertUser(user)
require.NoError(t, err)

client, err := srv.NewClient(TestUser(user.GetName()))
require.NoError(t, err)

request, err := client.CreateGithubAuthRequest(tt.request)
if tt.expectAccessDenied {
require.Error(t, err)
require.True(t, trace.IsAccessDenied(err), "expected access denied, got: %v", err)
return
}

require.NoError(t, err)
require.NotEmpty(t, request.StateToken)
require.Equal(t, tt.request.ConnectorID, request.ConnectorID)

requestCopy, err := clientReader.GetGithubAuthRequest(ctx, request.StateToken)
require.NoError(t, err)
require.Equal(t, request, requestCopy)
})
}
}

func TestSSODiagnosticInfo(t *testing.T) {
ctx := context.Background()
srv := newTestTLSServer(t)
Expand Down
21 changes: 18 additions & 3 deletions lib/auth/clt.go
Original file line number Diff line number Diff line change
Expand Up @@ -1254,9 +1254,22 @@ func (c *Client) CreateGithubAuthRequest(req services.GithubAuthRequest) (*servi
return &response, nil
}

// GetGithubAuthRequest gets Github AuthnRequest
func (c *Client) GetGithubAuthRequest(ctx context.Context, id string) (*services.GithubAuthRequest, error) {
out, err := c.Get(ctx, c.Endpoint("github", "requests", "get", id), url.Values{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that this needs to be an http request rather than gRPC? lib/auth.Client is deprecated in favor of api/client.Client, which is the purely gRPC client - #6394

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the gRPC endpoint would require adding the relevant types to .proto files and more refactoring besides. This is not necessarily the wrong thing to do, but I'm afraid it would balloon the size of this already large PR even more. Also, I think it would make sense to move all methods from a given group in one go, rather than have some implemented on HTTP side, and some on gRPC side.

Copy link
Contributor

@Joerger Joerger May 26, 2022

Choose a reason for hiding this comment

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

I think that making new http endpoints only makes things more confusing for developers. IMO it'd be better to take a hard lined approach of only adding new endpoints to gRPC, even if that fits less smoothly in the current code base. Otherwise we may have more instances of http endpoints being added needlessly, moving us further away from having a purely gRPC, public API, in one place.

Before this gets pushed to a release branch, we should move these new (and old) http endpoints to gRPC. I can create a follow up PR for this.

Copy link
Contributor Author

@Tener Tener May 26, 2022

Choose a reason for hiding this comment

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

Before this gets pushed to a release branch, we should move these new (and old) http endpoints to gRPC. I can create a follow up PR for this.

Sure, let's do it. There should be enough time before the v10 release to make this happen. This feature won't be merged to v9 either.

if err != nil {
return nil, trace.Wrap(err)
}
var response services.GithubAuthRequest
if err := json.Unmarshal(out.Bytes(), &response); err != nil {
return nil, trace.Wrap(err)
}
return &response, nil
}

// ValidateGithubAuthCallback validates Github auth callback returned from redirect
func (c *Client) ValidateGithubAuthCallback(q url.Values) (*GithubAuthResponse, error) {
out, err := c.PostJSON(context.TODO(), c.Endpoint("github", "requests", "validate"),
func (c *Client) ValidateGithubAuthCallback(ctx context.Context, q url.Values) (*GithubAuthResponse, error) {
out, err := c.PostJSON(ctx, c.Endpoint("github", "requests", "validate"),
validateGithubAuthCallbackReq{Query: q})
if err != nil {
return nil, trace.Wrap(err)
Expand Down Expand Up @@ -1741,8 +1754,10 @@ type IdentityService interface {
DeleteGithubConnector(ctx context.Context, id string) error
// CreateGithubAuthRequest creates a new request for Github OAuth2 flow
CreateGithubAuthRequest(services.GithubAuthRequest) (*services.GithubAuthRequest, error)
// GetGithubAuthRequest returns Github auth request if found
GetGithubAuthRequest(ctx context.Context, id string) (*services.GithubAuthRequest, error)
// ValidateGithubAuthCallback validates Github auth callback
ValidateGithubAuthCallback(q url.Values) (*GithubAuthResponse, error)
ValidateGithubAuthCallback(ctx context.Context, q url.Values) (*GithubAuthResponse, error)

// GetUser returns user by name
GetUser(name string, withSecrets bool) (types.User, error)
Expand Down
Loading