From 61864cd8421d66857f0581a49e8b582b24170b0f Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Thu, 29 Aug 2024 12:37:02 -0700 Subject: [PATCH 1/4] Commit working spike --- cmd/server/app/serve.go | 9 +- internal/auth/githubactions/githubactions.go | 76 +++++++++++ internal/auth/jwt/dynamic/dynamic_fetch.go | 129 +++++++++++++++++++ internal/auth/jwt/jwtauth.go | 7 + internal/auth/jwt/merged/merged_jwt.go | 43 +++++++ internal/controlplane/handlers_authz.go | 2 +- internal/flags/constants.go | 2 + internal/roles/service.go | 10 +- 8 files changed, 271 insertions(+), 7 deletions(-) create mode 100644 internal/auth/githubactions/githubactions.go create mode 100644 internal/auth/jwt/dynamic/dynamic_fetch.go create mode 100644 internal/auth/jwt/merged/merged_jwt.go diff --git a/cmd/server/app/serve.go b/cmd/server/app/serve.go index a364687b7a..1cdf4df7ca 100644 --- a/cmd/server/app/serve.go +++ b/cmd/server/app/serve.go @@ -28,7 +28,10 @@ import ( "github.com/spf13/viper" "github.com/stacklok/minder/internal/auth" + "github.com/stacklok/minder/internal/auth/githubactions" "github.com/stacklok/minder/internal/auth/jwt" + "github.com/stacklok/minder/internal/auth/jwt/dynamic" + "github.com/stacklok/minder/internal/auth/jwt/merged" "github.com/stacklok/minder/internal/auth/keycloak" "github.com/stacklok/minder/internal/authz" "github.com/stacklok/minder/internal/config" @@ -101,10 +104,12 @@ var serveCmd = &cobra.Command{ if err != nil { return fmt.Errorf("failed to create issuer URL: %w\n", err) } - jwt, err := jwt.NewJwtValidator(ctx, jwksUrl.String(), issUrl.String(), cfg.Identity.Server.Audience) + staticJwt, err := jwt.NewJwtValidator(ctx, jwksUrl.String(), issUrl.String(), cfg.Identity.Server.Audience) if err != nil { return fmt.Errorf("failed to fetch and cache identity provider JWKS: %w\n", err) } + dynamicJwt := dynamic.NewDynamicValidator(ctx, cfg.Identity.Server.Audience) + jwt := merged.Validator{Validators: []jwt.Validator{staticJwt, dynamicJwt}} authzc, err := authz.NewAuthzClient(&cfg.Authz, l) if err != nil { @@ -119,7 +124,7 @@ var serveCmd = &cobra.Command{ if err != nil { return fmt.Errorf("unable to create keycloak identity provider: %w", err) } - idClient, err := auth.NewIdentityClient(kc) + idClient, err := auth.NewIdentityClient(kc, &githubactions.GitHubActions{}) if err != nil { return fmt.Errorf("unable to create identity client: %w", err) } diff --git a/internal/auth/githubactions/githubactions.go b/internal/auth/githubactions/githubactions.go new file mode 100644 index 0000000000..6cccec3155 --- /dev/null +++ b/internal/auth/githubactions/githubactions.go @@ -0,0 +1,76 @@ +// +// Copyright 2024 Stacklok, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package githubactions provides an implementation of the GitHub IdentityProvider. +package githubactions + +import ( + "context" + "errors" + "net/url" + "strings" + + "github.com/lestrrat-go/jwx/v2/jwt" + + "github.com/stacklok/minder/internal/auth" +) + +// GitHubActions is an implementation of the auth.IdentityProvider interface. +type GitHubActions struct { +} + +var _ auth.IdentityProvider = (*GitHubActions)(nil) +var _ auth.Resolver = (*GitHubActions)(nil) + +var ghIssuerUrl = url.URL{ + Scheme: "https", + Host: "token.actions.githubusercontent.com", +} + +// String implements auth.IdentityProvider. +func (_ *GitHubActions) String() string { + return "githubactions" +} + +// URL implements auth.IdentityProvider. +func (_ *GitHubActions) URL() url.URL { + return ghIssuerUrl +} + +// Resolve implements auth.IdentityProvider. +func (gha *GitHubActions) Resolve(_ context.Context, id string) (*auth.Identity, error) { + // GitHub Actions subjects look like: + // repo:evankanderson/actions-id-token-testing:ref:refs/heads/main + // however, OpenFGA does not allow the "#" or ":" characters in the subject: + // https://github.com/openfga/openfga/blob/main/pkg/tuple/tuple.go#L34 + return &auth.Identity{ + UserID: strings.ReplaceAll(id, ":", "+"), + HumanName: strings.ReplaceAll(id, "+", ":"), + Provider: gha, + }, nil +} + +// Validate implements auth.IdentityProvider. +func (gha *GitHubActions) Validate(_ context.Context, token jwt.Token) (*auth.Identity, error) { + expectedUrl := gha.URL() + if token.Issuer() != expectedUrl.String() { + return nil, errors.New("token issuer is not the expected issuer") + } + return &auth.Identity{ + UserID: strings.ReplaceAll(token.Subject(), ":", "+"), + HumanName: token.Subject(), + Provider: gha, + }, nil +} diff --git a/internal/auth/jwt/dynamic/dynamic_fetch.go b/internal/auth/jwt/dynamic/dynamic_fetch.go new file mode 100644 index 0000000000..ed3cc9b885 --- /dev/null +++ b/internal/auth/jwt/dynamic/dynamic_fetch.go @@ -0,0 +1,129 @@ +// +// Copyright 2024 Stacklok, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package dynamic provides the logic for reading and validating JWT tokens +// using a JWKS URL from the token's +package dynamic + +import ( + "context" + "encoding/base64" + "encoding/json" + "fmt" + "io" + "net/http" + "time" + + "github.com/lestrrat-go/jwx/v2/jwk" + "github.com/lestrrat-go/jwx/v2/jws" + "github.com/lestrrat-go/jwx/v2/jwt" + "github.com/lestrrat-go/jwx/v2/jwt/openid" + + stacklok_jwt "github.com/stacklok/minder/internal/auth/jwt" +) + +// a subset of the openID well-known configuration for JSON parsing +type openIdConfig struct { + JwksURI string `json:"jwks_uri"` +} + +// Validator dynamically validates JWTs by fetching the key from the well-known OIDC issuer URL. +type Validator struct { + jwks *jwk.Cache + aud string +} + +var _ stacklok_jwt.Validator = (*Validator)(nil) + +// NewDynamicValidator creates a new instance of the dynamic JWT validator +func NewDynamicValidator(ctx context.Context, aud string) *Validator { + return &Validator{ + jwks: jwk.NewCache(ctx), + aud: aud, + } +} + +// ParseAndValidate implements jwt.Validator. +func (m Validator) ParseAndValidate(tokenString string) (openid.Token, error) { + // This is based on https://github.com/lestrrat-go/jwx/blob/v2/examples/jwt_parse_with_key_provider_example_test.go + + _, b64payload, _, err := jws.SplitCompact([]byte(tokenString)) + if err != nil { + return nil, fmt.Errorf("failed to split compact JWT: %w", err) + } + + jwtPayload := make([]byte, base64.RawStdEncoding.DecodedLen(len(b64payload))) + if _, err := base64.RawStdEncoding.Decode(jwtPayload, b64payload); err != nil { + return nil, fmt.Errorf("failed to decode JWT payload: %w", err) + } + + parsed, err := jwt.Parse(jwtPayload, jwt.WithVerify(false), jwt.WithToken(openid.New())) + if err != nil { + return nil, fmt.Errorf("failed to parse JWT payload: %w", err) + } + openIdToken, ok := parsed.(openid.Token) + if !ok { + return nil, fmt.Errorf("failed to cast JWT payload to openid.Token") + } + + // Now that we've got the issuer, we can validate the token + keySet, err := m.getKeySet(parsed.Issuer()) + if err != nil { + return nil, fmt.Errorf("failed to get JWK set: %w", err) + } + if _, err := jws.Verify([]byte(tokenString), jws.WithKeySet(keySet)); err != nil { + return nil, fmt.Errorf("failed to verify JWT: %w", err) + } + + return openIdToken, nil +} + +func (m Validator) getKeySet(issuer string) (jwk.Set, error) { + jwksUrl, err := getJWKSUrlForOpenId(issuer) + if err != nil { + return nil, fmt.Errorf("failed to fetch JWKS URL from openid: %w", err) + } + if err := m.jwks.Register(jwksUrl, jwk.WithMinRefreshInterval(15*time.Minute)); err != nil { + return nil, fmt.Errorf("failed to register JWKS URL: %w", err) + } + + return m.jwks.Get(context.Background(), jwksUrl) +} + +func getJWKSUrlForOpenId(issuer string) (string, error) { + wellKnownUrl := fmt.Sprintf("%s/.well-known/openid-configuration", issuer) + + resp, err := http.Get(wellKnownUrl) // #nosec: G107 + if err != nil { + return "", err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("unexpected status code: %d", resp.StatusCode) + } + + body, err := io.ReadAll(resp.Body) + if err != nil { + return "", fmt.Errorf("Failed to read respons body: %w", err) + } + + config := openIdConfig{} + if err := json.Unmarshal(body, &config); err != nil { + return "", fmt.Errorf("failed to unmarshal JSON: %w", err) + } + + return config.JwksURI, nil +} diff --git a/internal/auth/jwt/jwtauth.go b/internal/auth/jwt/jwtauth.go index 1825e2debb..c02ec2a858 100644 --- a/internal/auth/jwt/jwtauth.go +++ b/internal/auth/jwt/jwtauth.go @@ -19,6 +19,7 @@ package jwt import ( "context" "fmt" + "strings" "github.com/lestrrat-go/jwx/v2/jwk" "github.com/lestrrat-go/jwx/v2/jwt" @@ -124,6 +125,12 @@ func GetUserSubjectFromContext(ctx context.Context) string { if !ok { return "" } + // TODO: wire this in to IdentityProvider interface. Alternatively, have a different version + // for authzClient.Check that is IdentityProvider aware + + if token.Issuer() == "https://token.actions.githubusercontent.com" { + return fmt.Sprintf("githubactions/%s", strings.ReplaceAll(token.Subject(), ":", "+")) + } return token.Subject() } diff --git a/internal/auth/jwt/merged/merged_jwt.go b/internal/auth/jwt/merged/merged_jwt.go new file mode 100644 index 0000000000..6e60b5693c --- /dev/null +++ b/internal/auth/jwt/merged/merged_jwt.go @@ -0,0 +1,43 @@ +// +// Copyright 2024 Stacklok, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package merged provides the logic for reading and validating JWT tokens +package merged + +import ( + "fmt" + + "github.com/lestrrat-go/jwx/v2/jwt/openid" + + stacklok_jwt "github.com/stacklok/minder/internal/auth/jwt" +) + +// Validator is a struct that combines multiple JWT validators. +type Validator struct { + Validators []stacklok_jwt.Validator +} + +var _ stacklok_jwt.Validator = (*Validator)(nil) + +// ParseAndValidate implements jwt.Validator. +func (m Validator) ParseAndValidate(tokenString string) (openid.Token, error) { + for _, v := range m.Validators { + t, err := v.ParseAndValidate(tokenString) + if err == nil { + return t, nil + } + } + return nil, fmt.Errorf("no validator could parse and validate the token") +} diff --git a/internal/controlplane/handlers_authz.go b/internal/controlplane/handlers_authz.go index fc17d4a3fd..c2c25ebb03 100644 --- a/internal/controlplane/handlers_authz.go +++ b/internal/controlplane/handlers_authz.go @@ -347,7 +347,7 @@ func (s *Server) AssignRole(ctx context.Context, req *minder.AssignRoleRequest) } else if sub != "" && inviteeEmail == "" { // Enable one or the other. // This is temporary until we deprecate it completely in favor of email-based role assignments - if !flags.Bool(ctx, s.featureFlags, flags.UserManagement) { + if flags.Bool(ctx, s.featureFlags, flags.MachineAccounts) || !flags.Bool(ctx, s.featureFlags, flags.UserManagement) { assignment, err := db.WithTransaction(s.store, func(qtx db.ExtendQuerier) (*minder.RoleAssignment, error) { return s.roles.CreateRoleAssignment(ctx, qtx, s.authzClient, s.idClient, targetProject, sub, authzRole) }) diff --git a/internal/flags/constants.go b/internal/flags/constants.go index 31ee4e9e0f..8ed28d1ead 100644 --- a/internal/flags/constants.go +++ b/internal/flags/constants.go @@ -24,4 +24,6 @@ const ( DockerHubProvider Experiment = "dockerhub_provider" // GitLabProvider enables the GitLab provider. GitLabProvider Experiment = "gitlab_provider" + // MachineAccounts enables machine accounts (in particular, GitHub Actions) for authorization + MachineAccounts Experiment = "machine_accounts" ) diff --git a/internal/roles/service.go b/internal/roles/service.go index 1d0ee75764..a190b33f35 100644 --- a/internal/roles/service.go +++ b/internal/roles/service.go @@ -71,11 +71,13 @@ func (_ *roleService) CreateRoleAssignment(ctx context.Context, qtx db.Querier, // TODO: this assumes that we store all users in the database, and that we don't // need to namespace identify providers. We should revisit these assumptions. // - if _, err := qtx.GetUserBySubject(ctx, identity.String()); err != nil { - if errors.Is(err, sql.ErrNoRows) { - return nil, util.UserVisibleError(codes.NotFound, "User not found") + if identity.Provider.String() == "" { + if _, err := qtx.GetUserBySubject(ctx, identity.String()); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, util.UserVisibleError(codes.NotFound, "User not found") + } + return nil, status.Errorf(codes.Internal, "error getting user: %v", err) } - return nil, status.Errorf(codes.Internal, "error getting user: %v", err) } // Check in case there's an existing role assignment for the user From 3e08526c22a451d1ddc72b4d1b5d1c1da6e17421 Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Thu, 29 Aug 2024 15:28:11 -0700 Subject: [PATCH 2/4] Relax requirement for users to be in Minder DB in two other places in authz code --- internal/roles/service.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/internal/roles/service.go b/internal/roles/service.go index a190b33f35..70ba088b35 100644 --- a/internal/roles/service.go +++ b/internal/roles/service.go @@ -115,11 +115,13 @@ func (_ *roleService) UpdateRoleAssignment(ctx context.Context, qtx db.Querier, } // Verify if user exists - if _, err := qtx.GetUserBySubject(ctx, identity.String()); err != nil { - if errors.Is(err, sql.ErrNoRows) { - return nil, util.UserVisibleError(codes.NotFound, "User not found") + if identity.Provider.String() == "" { + if _, err := qtx.GetUserBySubject(ctx, identity.String()); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, util.UserVisibleError(codes.NotFound, "User not found") + } + return nil, status.Errorf(codes.Internal, "error getting user: %v", err) } - return nil, status.Errorf(codes.Internal, "error getting user: %v", err) } // Remove the existing role assignment for the user @@ -164,11 +166,13 @@ func (_ *roleService) RemoveRoleAssignment(ctx context.Context, qtx db.Querier, } // Verify if user exists - if _, err := qtx.GetUserBySubject(ctx, identity.String()); err != nil { - if errors.Is(err, sql.ErrNoRows) { - return nil, util.UserVisibleError(codes.NotFound, "User not found") + if identity.Provider.String() == "" { + if _, err := qtx.GetUserBySubject(ctx, identity.String()); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, util.UserVisibleError(codes.NotFound, "User not found") + } + return nil, status.Errorf(codes.Internal, "error getting user: %v", err) } - return nil, status.Errorf(codes.Internal, "error getting user: %v", err) } // Get all role assignments for the project From 1c955b95b2d4c44017121d5a57bf8db54cddd2a0 Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Thu, 5 Dec 2024 12:23:04 -0800 Subject: [PATCH 3/4] Fix stacklok/minder --> mindersec/minder --- cmd/server/app/serve.go | 6 +++--- internal/auth/githubactions/githubactions.go | 2 +- internal/auth/jwt/dynamic/dynamic_fetch.go | 2 +- internal/auth/jwt/merged/merged_jwt.go | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/server/app/serve.go b/cmd/server/app/serve.go index 17cfb7b16b..1bc2daf938 100644 --- a/cmd/server/app/serve.go +++ b/cmd/server/app/serve.go @@ -16,7 +16,10 @@ import ( "github.com/spf13/viper" "github.com/mindersec/minder/internal/auth" + "github.com/mindersec/minder/internal/auth/githubactions" "github.com/mindersec/minder/internal/auth/jwt" + "github.com/mindersec/minder/internal/auth/jwt/dynamic" + "github.com/mindersec/minder/internal/auth/jwt/merged" "github.com/mindersec/minder/internal/auth/keycloak" "github.com/mindersec/minder/internal/authz" cpmetrics "github.com/mindersec/minder/internal/controlplane/metrics" @@ -28,9 +31,6 @@ import ( "github.com/mindersec/minder/internal/service" "github.com/mindersec/minder/pkg/config" serverconfig "github.com/mindersec/minder/pkg/config/server" - "github.com/stacklok/minder/internal/auth/githubactions" - "github.com/stacklok/minder/internal/auth/jwt/dynamic" - "github.com/stacklok/minder/internal/auth/jwt/merged" ) var serveCmd = &cobra.Command{ diff --git a/internal/auth/githubactions/githubactions.go b/internal/auth/githubactions/githubactions.go index 6cccec3155..6d280349e0 100644 --- a/internal/auth/githubactions/githubactions.go +++ b/internal/auth/githubactions/githubactions.go @@ -24,7 +24,7 @@ import ( "github.com/lestrrat-go/jwx/v2/jwt" - "github.com/stacklok/minder/internal/auth" + "github.com/mindersec/minder/internal/auth" ) // GitHubActions is an implementation of the auth.IdentityProvider interface. diff --git a/internal/auth/jwt/dynamic/dynamic_fetch.go b/internal/auth/jwt/dynamic/dynamic_fetch.go index ed3cc9b885..befcfdde4b 100644 --- a/internal/auth/jwt/dynamic/dynamic_fetch.go +++ b/internal/auth/jwt/dynamic/dynamic_fetch.go @@ -31,7 +31,7 @@ import ( "github.com/lestrrat-go/jwx/v2/jwt" "github.com/lestrrat-go/jwx/v2/jwt/openid" - stacklok_jwt "github.com/stacklok/minder/internal/auth/jwt" + stacklok_jwt "github.com/mindersec/minder/internal/auth/jwt" ) // a subset of the openID well-known configuration for JSON parsing diff --git a/internal/auth/jwt/merged/merged_jwt.go b/internal/auth/jwt/merged/merged_jwt.go index 6e60b5693c..5066e23828 100644 --- a/internal/auth/jwt/merged/merged_jwt.go +++ b/internal/auth/jwt/merged/merged_jwt.go @@ -21,7 +21,7 @@ import ( "github.com/lestrrat-go/jwx/v2/jwt/openid" - stacklok_jwt "github.com/stacklok/minder/internal/auth/jwt" + stacklok_jwt "github.com/mindersec/minder/internal/auth/jwt" ) // Validator is a struct that combines multiple JWT validators. From d41e578cc1ba7b96fd2cddebb83c7262bddd35b8 Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Thu, 5 Dec 2024 12:37:48 -0800 Subject: [PATCH 4/4] Fix mocks in tests added after this PR started --- internal/controlplane/handlers_authz_test.go | 3 +++ internal/roles/service_test.go | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/internal/controlplane/handlers_authz_test.go b/internal/controlplane/handlers_authz_test.go index fd578f1967..bc91687f81 100644 --- a/internal/controlplane/handlers_authz_test.go +++ b/internal/controlplane/handlers_authz_test.go @@ -31,6 +31,7 @@ import ( "github.com/mindersec/minder/internal/auth" authjwt "github.com/mindersec/minder/internal/auth/jwt" "github.com/mindersec/minder/internal/auth/jwt/noop" + "github.com/mindersec/minder/internal/auth/keycloak" "github.com/mindersec/minder/internal/authz" "github.com/mindersec/minder/internal/authz/mock" "github.com/mindersec/minder/internal/db" @@ -485,9 +486,11 @@ func TestRoleManagement(t *testing.T) { data: []auth.Identity{{ UserID: user1.String(), HumanName: "user1", + Provider: &keycloak.KeyCloak{}, }, { UserID: user2.String(), HumanName: "user2", + Provider: &keycloak.KeyCloak{}, }}, }, jwt: noop.NewJwtValidator("test"), diff --git a/internal/roles/service_test.go b/internal/roles/service_test.go index f789d7ed18..0800e0fb0b 100644 --- a/internal/roles/service_test.go +++ b/internal/roles/service_test.go @@ -13,6 +13,7 @@ import ( "go.uber.org/mock/gomock" "github.com/mindersec/minder/internal/auth" + "github.com/mindersec/minder/internal/auth/keycloak" mockauth "github.com/mindersec/minder/internal/auth/mock" "github.com/mindersec/minder/internal/authz" "github.com/mindersec/minder/internal/authz/mock" @@ -82,6 +83,7 @@ func TestCreateRoleAssignment(t *testing.T) { idClient := mockauth.NewMockResolver(ctrl) idClient.EXPECT().Resolve(ctx, subject).Return(&auth.Identity{ UserID: subject, + Provider: &keycloak.KeyCloak{}, }, nil) service := NewRoleService() @@ -152,6 +154,7 @@ func TestUpdateRoleAssignment(t *testing.T) { idClient := mockauth.NewMockResolver(ctrl) idClient.EXPECT().Resolve(ctx, subject).Return(&auth.Identity{ UserID: subject, + Provider: &keycloak.KeyCloak{}, }, nil) service := NewRoleService() @@ -230,6 +233,7 @@ func TestRemoveRole(t *testing.T) { idClient := mockauth.NewMockResolver(ctrl) idClient.EXPECT().Resolve(ctx, subject).Return(&auth.Identity{ UserID: subject, + Provider: &keycloak.KeyCloak{}, }, nil) authzClient := &mock.SimpleClient{