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

[Spike] Changes needed to support GitHub Actions direct authentication to Minder #4317

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 7 additions & 2 deletions cmd/server/app/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -89,10 +92,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 {
Expand All @@ -107,7 +112,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)
}
Expand Down
76 changes: 76 additions & 0 deletions internal/auth/githubactions/githubactions.go
Original file line number Diff line number Diff line change
@@ -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/mindersec/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
}
129 changes: 129 additions & 0 deletions internal/auth/jwt/dynamic/dynamic_fetch.go
Original file line number Diff line number Diff line change
@@ -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/mindersec/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()))
Copy link
Member Author

Choose a reason for hiding this comment

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

Oop, this doesn't check audience, and should.

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())
Copy link
Member Author

Choose a reason for hiding this comment

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

There's a possible resource exhaustion attack here if you got a lot of OAuth issuers. We can probably narrow this down with an allow-list, e.g. from IdentityProvider API (get all the allowed issuers, drop the others early).

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) {
Copy link
Member Author

Choose a reason for hiding this comment

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

You would think this function existed in github.com/lestrrat-go/jwx, but you'd be mistaken, except for in one test.

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
}
7 changes: 7 additions & 0 deletions internal/auth/jwt/jwtauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package jwt
import (
"context"
"fmt"
"strings"

"github.com/lestrrat-go/jwx/v2/jwk"
"github.com/lestrrat-go/jwx/v2/jwt"
Expand Down Expand Up @@ -112,6 +113,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(), ":", "+"))
}
Comment on lines +116 to +121
Copy link
Member Author

Choose a reason for hiding this comment

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

Hahaha, this is gross. We should probably put all of this mangling / un-mangling in authzClient, rather than putting some inside and some outside (which is the current state of play).

return token.Subject()
}

Expand Down
43 changes: 43 additions & 0 deletions internal/auth/jwt/merged/merged_jwt.go
Original file line number Diff line number Diff line change
@@ -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/mindersec/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")
}
2 changes: 1 addition & 1 deletion internal/controlplane/handlers_authz.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,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) {
Comment on lines 337 to +339
Copy link
Member Author

Choose a reason for hiding this comment

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

Since machine accounts can't accept ToS or invitations, I'm extending the lifetime of this branch. Sorry-not-sorry!

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)
})
Expand Down
3 changes: 3 additions & 0 deletions internal/controlplane/handlers_authz_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"),
Expand Down
2 changes: 2 additions & 0 deletions internal/flags/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ 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"
// VulnCheckErrorTemplate enables improved evaluation details
// messages in the vulncheck rule.
VulnCheckErrorTemplate Experiment = "vulncheck_error_template"
Expand Down
30 changes: 18 additions & 12 deletions internal/roles/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,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)
}
Comment on lines 59 to 69
Copy link
Member Author

Choose a reason for hiding this comment

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

Oh look:

TODO: ... We should revisit these assumptions.

Visited!


// Check in case there's an existing role assignment for the user
Expand Down Expand Up @@ -101,11 +103,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
Expand Down Expand Up @@ -150,11 +154,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
Expand Down
Loading
Loading