From 8c91ab04f5094bd351d7dc53644861ddd9ea8cf2 Mon Sep 17 00:00:00 2001 From: Jack Warren Date: Sat, 11 Nov 2023 23:13:11 -0500 Subject: [PATCH 1/6] handle gin 404/405 better --- sherlock/internal/boot/router.go | 10 ++++++++++ sherlock/internal/errors/errors.go | 8 ++++++++ 2 files changed, 18 insertions(+) diff --git a/sherlock/internal/boot/router.go b/sherlock/internal/boot/router.go index f2fc8961a..5f237a158 100644 --- a/sherlock/internal/boot/router.go +++ b/sherlock/internal/boot/router.go @@ -2,6 +2,7 @@ package boot import ( "context" + "fmt" "github.com/broadinstitute/sherlock/go-shared/pkg/version" "github.com/broadinstitute/sherlock/sherlock/docs" "github.com/broadinstitute/sherlock/sherlock/html" @@ -11,6 +12,7 @@ import ( "github.com/broadinstitute/sherlock/sherlock/internal/boot/middleware" "github.com/broadinstitute/sherlock/sherlock/internal/config" "github.com/broadinstitute/sherlock/sherlock/internal/deprecated_handlers/v2handlers" + "github.com/broadinstitute/sherlock/sherlock/internal/errors" "github.com/broadinstitute/sherlock/sherlock/internal/metrics" "github.com/broadinstitute/sherlock/sherlock/internal/slack" "github.com/gin-gonic/gin" @@ -54,6 +56,14 @@ func BuildRouter(ctx context.Context, db *gorm.DB) *gin.Engine { slack.ErrorReportingMiddleware(ctx), middleware.Headers()) + // Replace Gin's standard fallback responses with our standard error format for friendlier client behavior + router.NoRoute(func(ctx *gin.Context) { + errors.AbortRequest(ctx, fmt.Errorf("(%s) no handler for %s found", errors.NotFound, ctx.Request.URL.Path)) + }) + router.NoMethod(func(ctx *gin.Context) { + errors.AbortRequest(ctx, fmt.Errorf("(%s) method %s not allowed for %s", errors.MethodNotAllowed, ctx.Request.Method, ctx.Request.URL.Path)) + }) + // /status, /version misc.ConfigureRoutes(&router.RouterGroup) diff --git a/sherlock/internal/errors/errors.go b/sherlock/internal/errors/errors.go index f069105f4..d7d5dde74 100644 --- a/sherlock/internal/errors/errors.go +++ b/sherlock/internal/errors/errors.go @@ -19,6 +19,7 @@ const ( BadRequest = "HTTP Bad Request" // 400 Forbidden = "HTTP Forbidden" // 403 NotFound = "HTTP Not Found" // 404 + MethodNotAllowed = "HTTP Method Not Allowed" // 405 ProxyAuthenticationRequired = "HTTP Proxy Authentication Required" // 407 Conflict = "HTTP Conflict" // 409 InternalServerError = "HTTP Internal Server Error" // 500 @@ -72,6 +73,13 @@ func convert(err error) (int, ErrorResponse) { Message: errorString, } } + if strings.Contains(errorString, MethodNotAllowed) { + return http.StatusMethodNotAllowed, ErrorResponse{ + ToBlame: "client", + Type: MethodNotAllowed, + Message: errorString, + } + } if strings.Contains(errorString, ProxyAuthenticationRequired) { return http.StatusProxyAuthRequired, ErrorResponse{ ToBlame: "client", From 140f01e3e4a974744b5311298736ffbe89e97e0e Mon Sep 17 00:00:00 2001 From: Jack Warren Date: Sat, 11 Nov 2023 23:13:48 -0500 Subject: [PATCH 2/6] improve flow of authentication layer --- .../internal/api/sherlock/handler_test.go | 2 +- ...{db_middleware.go => database_provider.go} | 12 +- .../authentication/gha_oidc/parse_header.go | 62 ++++++--- .../authentication/local_user/parse_header.go | 23 ++-- .../internal/authentication/middleware.go | 38 ++++++ .../authentication/test_users/parse_header.go | 21 ++- .../authentication/user_middleware.go | 115 ---------------- .../internal/authentication/user_provider.go | 127 ++++++++++++++++++ sherlock/internal/boot/router.go | 2 +- 9 files changed, 239 insertions(+), 163 deletions(-) rename sherlock/internal/authentication/{db_middleware.go => database_provider.go} (59%) create mode 100644 sherlock/internal/authentication/middleware.go delete mode 100644 sherlock/internal/authentication/user_middleware.go create mode 100644 sherlock/internal/authentication/user_provider.go diff --git a/sherlock/internal/api/sherlock/handler_test.go b/sherlock/internal/api/sherlock/handler_test.go index 028ff50cb..7a9d591a3 100644 --- a/sherlock/internal/api/sherlock/handler_test.go +++ b/sherlock/internal/api/sherlock/handler_test.go @@ -36,7 +36,7 @@ func (s *handlerSuite) SetupSuite() { func (s *handlerSuite) SetupTest() { s.TestSuiteHelper.SetupTest() s.internalRouter = gin.New() - apiRouter := s.internalRouter.Group("api", authentication.UserMiddleware(s.DB), authentication.DbMiddleware(s.DB)) + apiRouter := s.internalRouter.Group("api", authentication.Middleware(s.DB)...) ConfigureRoutes(apiRouter) } diff --git a/sherlock/internal/authentication/db_middleware.go b/sherlock/internal/authentication/database_provider.go similarity index 59% rename from sherlock/internal/authentication/db_middleware.go rename to sherlock/internal/authentication/database_provider.go index 6cf17e858..b32d9d195 100644 --- a/sherlock/internal/authentication/db_middleware.go +++ b/sherlock/internal/authentication/database_provider.go @@ -10,8 +10,7 @@ import ( const ctxDbFieldName = "SherlockDB" -// DbMiddleware must strictly come after UserMiddleware, its call to MustUseUser will fail otherwise. -func DbMiddleware(db *gorm.DB) gin.HandlerFunc { +func setDatabaseWithUser(db *gorm.DB) gin.HandlerFunc { return func(ctx *gin.Context) { user, err := MustUseUser(ctx) if err != nil { @@ -22,21 +21,24 @@ func DbMiddleware(db *gorm.DB) gin.HandlerFunc { } } +// ShouldUseDB returns a non-nil *gorm.DB with the calling user accessible via models.GetCurrentUserForDB, or an error +// if that isn't possible. func ShouldUseDB(ctx *gin.Context) (*gorm.DB, error) { dbValue, exists := ctx.Get(ctxDbFieldName) if !exists { - return nil, fmt.Errorf("(%s) database authentication middleware not present", errors.InternalServerError) + return nil, fmt.Errorf("(%s) database reference not present; database authentication middleware likely not present", errors.InternalServerError) } db, ok := dbValue.(*gorm.DB) if !ok { - return nil, fmt.Errorf("(%s) database authentication middleware misconfigured: db represented as %T", errors.InternalServerError, dbValue) + return nil, fmt.Errorf("(%s) database authentication middleware likely misconfigured: represented as %T", errors.InternalServerError, dbValue) } if db == nil { - return nil, fmt.Errorf("(%s) database reference was nil", errors.InternalServerError) + return nil, fmt.Errorf("(%s) database authentication middleware likely misconfigured: database reference was nil", errors.InternalServerError) } return db, nil } +// MustUseDB is like ShouldUseDB except it calls errors.AbortRequest if there was an error so the caller doesn't have to. func MustUseDB(ctx *gin.Context) (*gorm.DB, error) { db, err := ShouldUseDB(ctx) if err != nil { diff --git a/sherlock/internal/authentication/gha_oidc/parse_header.go b/sherlock/internal/authentication/gha_oidc/parse_header.go index 265eaec75..a2d3e1a6e 100644 --- a/sherlock/internal/authentication/gha_oidc/parse_header.go +++ b/sherlock/internal/authentication/gha_oidc/parse_header.go @@ -4,37 +4,65 @@ import ( "fmt" "github.com/broadinstitute/sherlock/sherlock/internal/config" "github.com/broadinstitute/sherlock/sherlock/internal/errors" + "github.com/broadinstitute/sherlock/sherlock/internal/slack" "github.com/gin-gonic/gin" - "github.com/rs/zerolog/log" + "strings" ) const ( ghaOidcHeader = "X-GHA-OIDC-JWT" ) -type extraClaims struct { - Actor string `json:"actor"` - ActorID string `json:"actor_id"` - Repository string `json:"repository"` +type Claims struct { + // GitHub username that initiated the workflow + Actor string `json:"actor"` + // ID for the GitHub user that initiated the workflow + ActorID string `json:"actor_id"` + // Owner of the repo that the workflow ran in RepositoryOwner string `json:"repository_owner"` - JobWorkflowRef string `json:"job_workflow_ref"` + // Repo that the workflow ran in, given like / + Repository string `json:"repository"` + // The ref path to the workflow, given like //@ + WorkflowRef string `json:"workflow_ref"` + // The ID of the workflow + RunID string `json:"run_id"` + // The number of times the workflow has been attempted + RunAttempt string `json:"run_attempt"` } -func ParseHeader(ctx *gin.Context) (present bool, githubUsername string, githubID string, err error) { +// TrimmedRepositoryName provides the repository name without the leading owner and slash +func (c *Claims) TrimmedRepositoryName() string { + return strings.TrimPrefix(c.Repository, c.RepositoryOwner+"/") +} + +// TrimmedWorkflowPath provides the workflow path without the leading repository owner, name, and slash or the +// trailing git ref +func (c *Claims) TrimmedWorkflowPath() string { + return strings.Split(strings.TrimPrefix(c.WorkflowRef, c.Repository+"/"), "@")[0] +} + +func (c *Claims) WorkflowURL() string { + return fmt.Sprintf("https://github.com/%s/actions/runs/%s/attempts/%s", c.Repository, c.RunID, c.RunAttempt) +} + +func ParseHeader(ctx *gin.Context) (*Claims, error) { + if verifier == nil { + return nil, fmt.Errorf("(%s) gha_oidc.ParseHeader was called before gha_oidc.InitVerifier", errors.InternalServerError) + } ghaOidcJwt := ctx.GetHeader(ghaOidcHeader) if ghaOidcJwt == "" { - return false, "", "", nil + return nil, nil } payload, err := verifier.Verify(ctx, ghaOidcJwt) if err != nil { - return true, "", "", fmt.Errorf("(%s) failed to validate GHA OIDC JWT in '%s' header: %w", errors.BadRequest, ghaOidcHeader, err) + return nil, fmt.Errorf("(%s) failed to validate GHA OIDC JWT in '%s' header: %w", errors.BadRequest, ghaOidcHeader, err) } else if payload == nil { - return true, "", "", fmt.Errorf("(%s) GHA OIDC JWT seemed to pass validation but payload was nil", errors.BadRequest) + return nil, fmt.Errorf("(%s) GHA OIDC JWT seemed to pass validation but payload was nil", errors.BadRequest) } - var claims extraClaims + var claims Claims if err = payload.Claims(&claims); err != nil { - return true, "", "", fmt.Errorf("(%s) GHA OIDC JWT seemed to pass validation but couldn't be unmarshalled to %T: %w", errors.BadRequest, claims, err) + return nil, fmt.Errorf("(%s) GHA OIDC JWT seemed to pass validation but couldn't be unmarshalled to %T: %w", errors.BadRequest, claims, err) } var repositoryOwnerAccepted bool @@ -44,11 +72,9 @@ func ParseHeader(ctx *gin.Context) (present bool, githubUsername string, githubI break } } - if repositoryOwnerAccepted { - log.Info().Msgf("GHA | parsed GHA OIDC JWT from %s (ID %s) in %s via %s", claims.Actor, claims.ActorID, claims.Repository, claims.JobWorkflowRef) - return true, claims.Actor, claims.ActorID, nil - } else { - log.Warn().Msgf("GHA | rejected un-allowed organization GHA OIDC JWT from %s (ID %s) in %s via %s", claims.Actor, claims.ActorID, claims.Repository, claims.JobWorkflowRef) - return true, "", "", fmt.Errorf("(%s) GHA OIDC JWT was from an organization '%s' not allowed in Sherlock's config", errors.Forbidden, claims.RepositoryOwner) + if !repositoryOwnerAccepted { + slack.ReportError(ctx, fmt.Errorf("observed a GHA OIDC JWT from %s and ignored it because %s was not an allowed organization in Sherlock's config", claims.Repository, claims.RepositoryOwner)) + return nil, nil } + return &claims, nil } diff --git a/sherlock/internal/authentication/local_user/parse_header.go b/sherlock/internal/authentication/local_user/parse_header.go index 558fb66ad..7c5a161c7 100644 --- a/sherlock/internal/authentication/local_user/parse_header.go +++ b/sherlock/internal/authentication/local_user/parse_header.go @@ -1,8 +1,9 @@ package local_user import ( + "fmt" "github.com/gin-gonic/gin" - "strings" + "strconv" ) // TODO (Jack): Make this package actually grab and cache the ADC credentials, so we use the user's real email/ID @@ -21,20 +22,20 @@ var ( LocalUserSuitable = true ) -func ParseHeader(ctx *gin.Context) (email string, googleID string) { - emailHeader := ctx.GetHeader(EmailControlHeader) - if emailHeader != "" { +func ParseHeader(ctx *gin.Context) (email string, googleID string, err error) { + if emailHeader := ctx.GetHeader(EmailControlHeader); emailHeader != "" { LocalUserEmail = emailHeader } - googleIDHeader := ctx.GetHeader(GoogleIDControlHeader) - if googleIDHeader != "" { + + if googleIDHeader := ctx.GetHeader(GoogleIDControlHeader); googleIDHeader != "" { LocalUserGoogleID = googleIDHeader } - suitabilityHeader := ctx.GetHeader(SuitabilityControlHeader) - if strings.ToLower(suitabilityHeader) == "false" { - LocalUserSuitable = false - } else { + + if suitabilityHeader := ctx.GetHeader(SuitabilityControlHeader); suitabilityHeader == "" { LocalUserSuitable = true + } else if LocalUserSuitable, err = strconv.ParseBool(suitabilityHeader); err != nil { + return "", "", fmt.Errorf("failed to parse boolean from %s header: %w", SuitabilityControlHeader, err) } - return LocalUserEmail, LocalUserGoogleID + + return LocalUserEmail, LocalUserGoogleID, nil } diff --git a/sherlock/internal/authentication/middleware.go b/sherlock/internal/authentication/middleware.go new file mode 100644 index 000000000..9b9717dbe --- /dev/null +++ b/sherlock/internal/authentication/middleware.go @@ -0,0 +1,38 @@ +package authentication + +import ( + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/authentication_method" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/iap" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/local_user" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/test_users" + "github.com/broadinstitute/sherlock/sherlock/internal/config" + "github.com/gin-gonic/gin" + "github.com/rs/zerolog/log" + "gorm.io/gorm" +) + +// Middleware returns an ordered list of middleware to authenticate requests, enabling request handlers to use functions +// like ShouldUseDB, ShouldUseUser, and ShouldUseGithubClaims. +func Middleware(db *gorm.DB) gin.HandlersChain { + if config.Config.String("mode") == "debug" { + if config.Config.Bool("auth.createTestUsersInMiddleware") { + log.Info().Msgf("AUTH | using test authentication; will create test users from middleware") + return gin.HandlersChain{ + setUserWhoConnected(db, test_users.ParseHeader, authentication_method.TEST), + setDatabaseWithUser(db), + } + } else { + log.Info().Msgf("AUTH | using local authentication; will create a local user from middleware") + return gin.HandlersChain{ + setUserWhoConnected(db, local_user.ParseHeader, authentication_method.LOCAL), + setDatabaseWithUser(db), + } + } + } else { + return gin.HandlersChain{ + setUserWhoConnected(db, iap.ParseHeader, authentication_method.IAP), + setGithubClaimsAndEscalateUser(db), + setDatabaseWithUser(db), + } + } +} diff --git a/sherlock/internal/authentication/test_users/parse_header.go b/sherlock/internal/authentication/test_users/parse_header.go index 7c6e4bf24..a5c221136 100644 --- a/sherlock/internal/authentication/test_users/parse_header.go +++ b/sherlock/internal/authentication/test_users/parse_header.go @@ -1,6 +1,7 @@ package test_users import ( + "fmt" "github.com/gin-gonic/gin" "strconv" ) @@ -13,18 +14,14 @@ const ( NonSuitableTestUserGoogleID = "67896789" ) -func ParseHeader(ctx *gin.Context) (email string, googleID string) { - header := ctx.GetHeader(SuitabilityControlHeader) - if header == "" { - return SuitableTestUserEmail, SuitableTestUserGoogleID - } - suitable, err := strconv.ParseBool(header) - if err != nil { - panic(err) - } - if suitable { - return SuitableTestUserEmail, SuitableTestUserGoogleID +func ParseHeader(ctx *gin.Context) (email string, googleID string, err error) { + if header := ctx.GetHeader(SuitabilityControlHeader); header == "" { + return SuitableTestUserEmail, SuitableTestUserGoogleID, nil + } else if suitable, err := strconv.ParseBool(header); err != nil { + return "", "", fmt.Errorf("failed to parse boolean from %s header: %w", SuitabilityControlHeader, err) + } else if suitable { + return SuitableTestUserEmail, SuitableTestUserGoogleID, nil } else { - return NonSuitableTestUserEmail, NonSuitableTestUserGoogleID + return NonSuitableTestUserEmail, NonSuitableTestUserGoogleID, nil } } diff --git a/sherlock/internal/authentication/user_middleware.go b/sherlock/internal/authentication/user_middleware.go deleted file mode 100644 index 03a73ca2a..000000000 --- a/sherlock/internal/authentication/user_middleware.go +++ /dev/null @@ -1,115 +0,0 @@ -package authentication - -import ( - stdErrors "errors" - "fmt" - "github.com/broadinstitute/sherlock/sherlock/internal/authentication/authentication_method" - "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc" - "github.com/broadinstitute/sherlock/sherlock/internal/authentication/iap" - "github.com/broadinstitute/sherlock/sherlock/internal/authentication/local_user" - "github.com/broadinstitute/sherlock/sherlock/internal/authentication/test_users" - "github.com/broadinstitute/sherlock/sherlock/internal/config" - "github.com/broadinstitute/sherlock/sherlock/internal/errors" - "github.com/broadinstitute/sherlock/sherlock/internal/models" - "github.com/gin-gonic/gin" - "github.com/rs/zerolog/log" - "gorm.io/gorm" -) - -const ctxUserFieldName = "SherlockUser" - -func UserMiddleware(db *gorm.DB) gin.HandlerFunc { - if config.Config.String("mode") == "debug" { - if config.Config.Bool("auth.createTestUsersInMiddleware") { - log.Info().Msgf("AUTH | using test authentication; will create test users from middleware") - return fakeUserMiddleware(db, test_users.ParseHeader, authentication_method.TEST) - } else { - log.Info().Msgf("AUTH | using local authentication; will create a local user from middleware") - return fakeUserMiddleware(db, local_user.ParseHeader, authentication_method.LOCAL) - } - } else { - return realUserMiddleware(db) - } -} - -func realUserMiddleware(db *gorm.DB) gin.HandlerFunc { - return func(ctx *gin.Context) { - email, googleID, err := iap.ParseHeader(ctx) - if err != nil { - errors.AbortRequest(ctx, err) - return - } - - var iapUser models.User - if err = db.Where(&models.User{Email: email, GoogleID: googleID}).FirstOrCreate(&iapUser).Error; err != nil { - errors.AbortRequest(ctx, err) - return - } - iapUser.AuthenticationMethod = authentication_method.IAP - - headerPresent, githubUsername, githubID, err := gha_oidc.ParseHeader(ctx) - if headerPresent { - if err != nil { - errors.AbortRequest(ctx, err) - return - } - - var ghaUser models.User - if err = db.Where(&models.User{GithubID: &githubID}).First(&ghaUser).Error; err != nil { - if stdErrors.Is(err, gorm.ErrRecordNotFound) { - log.Info().Msgf("AUTH | ignored GHA OIDC JWT for unknown github user %s, still using IAP JWT user %s", githubUsername, iapUser.Email) - ctx.Set(ctxUserFieldName, &iapUser) - } else { - errors.AbortRequest(ctx, err) - return - } - } else { - ghaUser.AuthenticationMethod = authentication_method.GHA - ghaUser.Via = &iapUser - log.Info().Msgf("AUTH | substituted GHA OIDC JWT user %s (github %s) over IAP JWT user %s", ghaUser.Email, githubUsername, iapUser.Email) - ctx.Set(ctxUserFieldName, &ghaUser) - } - } else { - ctx.Set(ctxUserFieldName, &iapUser) - } - } -} - -// fakeUserMiddleware has the same out-facing functionality as realUserMiddleware but it basically blindly uses the given header parser method. -// This means that "nonsense" (insecure) header parsers can be passed, to effectively create Sherlock users out of thin air. -// This is useful for local/test operation. -func fakeUserMiddleware(db *gorm.DB, headerParser func(ctx *gin.Context) (email string, googleID string), method authentication_method.Method) gin.HandlerFunc { - return func(ctx *gin.Context) { - email, googleID := headerParser(ctx) - var fakeUser models.User - if err := db.Where(&models.User{Email: email, GoogleID: googleID}).FirstOrCreate(&fakeUser).Error; err != nil { - errors.AbortRequest(ctx, err) - return - } - fakeUser.AuthenticationMethod = method - ctx.Set(ctxUserFieldName, &fakeUser) - } -} - -func ShouldUseUser(ctx *gin.Context) (*models.User, error) { - userValue, exists := ctx.Get(ctxUserFieldName) - if !exists { - return nil, fmt.Errorf("(%s) user authentication middleware not present", errors.InternalServerError) - } - user, ok := userValue.(*models.User) - if !ok { - return nil, fmt.Errorf("(%s) user authentication middleware misconfigured: represented as %T", errors.InternalServerError, userValue) - } - if user == nil { - return nil, fmt.Errorf("(%s) user was nil", errors.InternalServerError) - } - return user, nil -} - -func MustUseUser(ctx *gin.Context) (*models.User, error) { - user, err := ShouldUseUser(ctx) - if err != nil { - errors.AbortRequest(ctx, err) - } - return user, err -} diff --git a/sherlock/internal/authentication/user_provider.go b/sherlock/internal/authentication/user_provider.go new file mode 100644 index 000000000..28e445a27 --- /dev/null +++ b/sherlock/internal/authentication/user_provider.go @@ -0,0 +1,127 @@ +package authentication + +import ( + "fmt" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/authentication_method" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc" + "github.com/broadinstitute/sherlock/sherlock/internal/errors" + "github.com/broadinstitute/sherlock/sherlock/internal/models" + "github.com/gin-gonic/gin" + "github.com/rs/zerolog/log" + "gorm.io/gorm" +) + +const ( + ctxUserFieldName = "SherlockUser" + ctxGithubClaimsFieldName = "GithubActionsClaims" +) + +// setUserWhoConnected returns a gin.HandlerFunc that finds or inserts the models.User of the connecting client. +// It sets ctxUserFieldName. +// +// When configured with a parser like test_users.ParseHeader, it essentially creates the test users out of thin air +// as requests are made. +// When configured with a parser like iap.ParseHeader, it enforces that Sherlock be running behind Google's +// Identity-Aware Proxy. +func setUserWhoConnected(db *gorm.DB, headerParser func(ctx *gin.Context) (email string, googleID string, err error), method authentication_method.Method) gin.HandlerFunc { + return func(ctx *gin.Context) { + var user models.User + if email, googleID, err := headerParser(ctx); err != nil { + errors.AbortRequest(ctx, fmt.Errorf("failed to parse the connecting user from headers: %w", err)) + return + } else if err = db.Where(&models.User{Email: email, GoogleID: googleID}).FirstOrCreate(&user).Error; err != nil { + errors.AbortRequest(ctx, fmt.Errorf("failed to find or insert the connecting user in database: %w", err)) + return + } else { + user.AuthenticationMethod = method + ctx.Set(ctxUserFieldName, &user) + } + } +} + +// setGithubClaimsAndEscalateUser returns a gin.HandlerFunc that attempts to parse gha_oidc.Claims from the request if +// it came from GitHub Actions. If those claims can be tied to a models.User it will use that as the primary user, too. +// It possibly sets ctxGithubClaimsFieldName, and if so, possibly ctxUserFieldName. +// +// gha_oidc.ParseHeader validates that the OIDC JWT from GitHub is authentic, so any information in gha_oidc.Claims will +// be true. +// Meanwhile, each models.User will only have a GitHub identity set based on that user sending Sherlock an access token +// for that GitHub identity. +// This means we can know for certain the models.User responsible for the call to Sherlock, so we can safely substitute +// that information over top of the (presumably service account) models.User used to access IAP and thus parsed by +// setUserWhoConnected. +func setGithubClaimsAndEscalateUser(db *gorm.DB) gin.HandlerFunc { + return func(ctx *gin.Context) { + if claims, err := gha_oidc.ParseHeader(ctx); err != nil { + errors.AbortRequest(ctx, fmt.Errorf("failed to parse GitHub Actions claims: %w", err)) + return + } else if claims != nil { + ctx.Set(ctxGithubClaimsFieldName, claims) + + var matchingGithubUsers []models.User + if err = db.Where(&models.User{GithubID: &claims.ActorID}).Limit(1).Find(&matchingGithubUsers).Error; err != nil { + errors.AbortRequest(ctx, fmt.Errorf("failed to query for users matching GitHub Actions claims: %w", err)) + return + } else if len(matchingGithubUsers) > 0 { + user := matchingGithubUsers[0] + if oldUser, err := ShouldUseUser(ctx); err != nil { + log.Warn().Err(err).Msg("AUTH | was unable to read old user to escalate user based on GitHub claims") + } else { + user.AuthenticationMethod = authentication_method.GHA + user.Via = oldUser + ctx.Set(ctxUserFieldName, &user) + log.Debug().Str("workflow", claims.WorkflowURL()).Msgf("AUTH | recognized %s connecting through GitHub Actions via %s", user.Email, oldUser.Email) + } + } else { + log.Debug().Str("workflow", claims.WorkflowURL()).Msgf("AUTH | ignoring unknown user %s referenced by GitHub Actions claims", claims.Actor) + } + } + } +} + +// ShouldUseUser returns a non-nil *models.User who made the request, or an error if that isn't possible. +func ShouldUseUser(ctx *gin.Context) (*models.User, error) { + userValue, exists := ctx.Get(ctxUserFieldName) + if !exists { + return nil, fmt.Errorf("(%s) user not present; authentication middleware likely not present", errors.InternalServerError) + } + user, ok := userValue.(*models.User) + if !ok { + return nil, fmt.Errorf("(%s) user authentication middleware likely misconfigured: represented as %T", errors.InternalServerError, userValue) + } + if user == nil { + return nil, fmt.Errorf("(%s) user authentication middleware likely misconfigured: user was nil", errors.InternalServerError) + } + return user, nil +} + +// MustUseUser is like ShouldUseUser except it calls errors.AbortRequest if there was an error so the +// caller doesn't have to. +func MustUseUser(ctx *gin.Context) (*models.User, error) { + user, err := ShouldUseUser(ctx) + if err != nil { + errors.AbortRequest(ctx, err) + } + return user, err +} + +// ShouldUseGithubClaims returns non-nil *gha_oidc.Claims associated with the request, or an error if that isn't +// possible. +// +// Note that the Actor/ActorID fields of the gha_oidc.Claims must not be correlated to a models.User. That correlation +// is the responsibility of the authentication package; use ShouldUseUser or MustUseUser to access the models.User +// associated with the request. +func ShouldUseGithubClaims(ctx *gin.Context) (*gha_oidc.Claims, error) { + claimsValue, exists := ctx.Get(ctxGithubClaimsFieldName) + if !exists { + return nil, fmt.Errorf("(%s) GitHub OIDC claims were not present", errors.InternalServerError) + } + claims, ok := claimsValue.(*gha_oidc.Claims) + if !ok { + return nil, fmt.Errorf("(%s) GitHub OIDC claims middleware may be misconfigured: represented as %T", errors.InternalServerError, claimsValue) + } + if claims == nil { + return nil, fmt.Errorf("(%s) GitHub OIDC claims middleware may be misconfigured: claims were nil", errors.InternalServerError) + } + return claims, nil +} diff --git a/sherlock/internal/boot/router.go b/sherlock/internal/boot/router.go index 5f237a158..b6c1259c2 100644 --- a/sherlock/internal/boot/router.go +++ b/sherlock/internal/boot/router.go @@ -78,7 +78,7 @@ func BuildRouter(ctx context.Context, db *gorm.DB) *gin.Engine { router.GET("", func(ctx *gin.Context) { ctx.Redirect(http.StatusMovedPermanently, "/swagger/index.html") }) // routes under /api require authentication and may use the database - apiRouter := router.Group("api", authentication.UserMiddleware(db), authentication.DbMiddleware(db)) + apiRouter := router.Group("api", authentication.Middleware(db)...) // refactored sherlock API, under /api/{type}/v3 sherlock.ConfigureRoutes(apiRouter) From 6ba7d3aaa412420cd17688d82a6ce5cd99b1a697 Mon Sep 17 00:00:00 2001 From: Jack Warren Date: Sat, 11 Nov 2023 23:15:49 -0500 Subject: [PATCH 3/6] grab claims in ci run upsert --- .../api/sherlock/ci_runs_v3_upsert.go | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/sherlock/internal/api/sherlock/ci_runs_v3_upsert.go b/sherlock/internal/api/sherlock/ci_runs_v3_upsert.go index 738d43c59..199cf9d9b 100644 --- a/sherlock/internal/api/sherlock/ci_runs_v3_upsert.go +++ b/sherlock/internal/api/sherlock/ci_runs_v3_upsert.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/broadinstitute/sherlock/go-shared/pkg/utils" "github.com/broadinstitute/sherlock/sherlock/internal/authentication" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc" "github.com/broadinstitute/sherlock/sherlock/internal/config" "github.com/broadinstitute/sherlock/sherlock/internal/deployhooks" "github.com/broadinstitute/sherlock/sherlock/internal/deprecated_models/v2models" @@ -67,6 +68,29 @@ func ciRunsV3Upsert(ctx *gin.Context) { return } + // Opportunistically fill empty fields with information passed in the GHA OIDC JWT + if body.Platform == "" || body.Platform == "github-actions" { + var claims *gha_oidc.Claims + if claims, err = authentication.ShouldUseGithubClaims(ctx); err == nil { + body.Platform = "github-actions" + if body.GithubActionsOwner == "" { + body.GithubActionsOwner = claims.RepositoryOwner + } + if body.GithubActionsRepo == "" { + body.GithubActionsRepo = claims.TrimmedRepositoryName() + } + if body.GithubActionsRunID == 0 { + body.GithubActionsRunID, _ = utils.ParseUint(claims.RunID) + } + if body.GithubActionsAttemptNumber == 0 { + body.GithubActionsAttemptNumber, _ = utils.ParseUint(claims.RunAttempt) + } + if body.GithubActionsWorkflowPath == "" { + body.GithubActionsWorkflowPath = claims.TrimmedWorkflowPath() + } + } + } + // We want to handle the "spreading" mechanic that some of the fields have. To do that, we'll literally just re-assemble // the body we got into one post-spread. Then we'll handle that body and de-dupe the resulting CiIdentifiers before // adding to the database (the SQL gets messed up if there's duplicates in what we give to Gorm). From 6add9a95f80998adcb03bc8c4e09b50a693e90c7 Mon Sep 17 00:00:00 2001 From: Jack Warren Date: Mon, 13 Nov 2023 15:27:24 -0500 Subject: [PATCH 4/6] make it testable :/ --- sherlock/.mockery.yaml | 3 + .../api/sherlock/ci_runs_v3_upsert.go | 4 +- .../internal/authentication/gha_oidc/boot.go | 58 +++++++++ .../gha_oidc/gha_oidc_claims/claims.go | 38 ++++++ .../gha_oidc_mocks/mock_mockable_verifier.go | 91 +++++++++++++++ .../authentication/gha_oidc/init_verifier.go | 36 ------ .../authentication/gha_oidc/parse_header.go | 60 ++-------- .../gha_oidc/parse_header_test.go | 110 ++++++++++++++++++ .../authentication/gha_oidc/verifier.go | 36 ++++++ .../authentication/iap/parse_header.go | 8 +- .../authentication/local_user/parse_header.go | 22 ++-- .../internal/authentication/middleware.go | 22 ++-- .../authentication/test_users/parse_header.go | 6 +- .../test_users/parse_header_test.go | 22 ++-- .../test_users/test_suite_helper.go | 6 +- .../test_users/test_suite_helper_test.go | 4 +- .../internal/authentication/user_provider.go | 35 +++--- sherlock/internal/github/client.go | 4 + sherlock/internal/slack/client.go | 4 + 19 files changed, 421 insertions(+), 148 deletions(-) create mode 100644 sherlock/internal/authentication/gha_oidc/boot.go create mode 100644 sherlock/internal/authentication/gha_oidc/gha_oidc_claims/claims.go create mode 100644 sherlock/internal/authentication/gha_oidc/gha_oidc_mocks/mock_mockable_verifier.go delete mode 100644 sherlock/internal/authentication/gha_oidc/init_verifier.go create mode 100644 sherlock/internal/authentication/gha_oidc/parse_header_test.go create mode 100644 sherlock/internal/authentication/gha_oidc/verifier.go diff --git a/sherlock/.mockery.yaml b/sherlock/.mockery.yaml index 41a5c4485..9db10ad75 100644 --- a/sherlock/.mockery.yaml +++ b/sherlock/.mockery.yaml @@ -10,6 +10,9 @@ packages: IRoutes: config: dir: ./internal/api/gin_mocks + github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc: + interfaces: + mockableVerifier: github.com/broadinstitute/sherlock/sherlock/internal/github: interfaces: mockableActionsClient: diff --git a/sherlock/internal/api/sherlock/ci_runs_v3_upsert.go b/sherlock/internal/api/sherlock/ci_runs_v3_upsert.go index 199cf9d9b..5623aa52a 100644 --- a/sherlock/internal/api/sherlock/ci_runs_v3_upsert.go +++ b/sherlock/internal/api/sherlock/ci_runs_v3_upsert.go @@ -4,7 +4,7 @@ import ( "fmt" "github.com/broadinstitute/sherlock/go-shared/pkg/utils" "github.com/broadinstitute/sherlock/sherlock/internal/authentication" - "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc/gha_oidc_claims" "github.com/broadinstitute/sherlock/sherlock/internal/config" "github.com/broadinstitute/sherlock/sherlock/internal/deployhooks" "github.com/broadinstitute/sherlock/sherlock/internal/deprecated_models/v2models" @@ -70,7 +70,7 @@ func ciRunsV3Upsert(ctx *gin.Context) { // Opportunistically fill empty fields with information passed in the GHA OIDC JWT if body.Platform == "" || body.Platform == "github-actions" { - var claims *gha_oidc.Claims + var claims *gha_oidc_claims.Claims if claims, err = authentication.ShouldUseGithubClaims(ctx); err == nil { body.Platform = "github-actions" if body.GithubActionsOwner == "" { diff --git a/sherlock/internal/authentication/gha_oidc/boot.go b/sherlock/internal/authentication/gha_oidc/boot.go new file mode 100644 index 000000000..b1082123e --- /dev/null +++ b/sherlock/internal/authentication/gha_oidc/boot.go @@ -0,0 +1,58 @@ +package gha_oidc + +import ( + "context" + "fmt" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc/gha_oidc_claims" + "github.com/broadinstitute/sherlock/sherlock/internal/config" + "github.com/broadinstitute/sherlock/sherlock/internal/errors" + "github.com/coreos/go-oidc" +) + +// realVerifierImplementation wraps *oidc.IDTokenVerifier so we can make an actually +// mockable *oidc.IDTokenVerifier.Verify method (that method returns a non-mockable +// struct). +type realVerifierImplementation struct { + *oidc.IDTokenVerifier +} + +func (r *realVerifierImplementation) VerifyAndParseClaims(ctx context.Context, rawIDToken string) (gha_oidc_claims.Claims, error) { + var claims gha_oidc_claims.Claims + if payload, err := r.Verify(ctx, rawIDToken); err != nil { + return claims, fmt.Errorf("(%s) failed to validate GHA OIDC JWT in '%s' header: %w", errors.BadRequest, Header, err) + } else if payload == nil { + return claims, fmt.Errorf("(%s) GHA OIDC JWT seemed to pass validation but payload was nil", errors.BadRequest) + } else if err = payload.Claims(&claims); err != nil { + return claims, fmt.Errorf("(%s) GHA OIDC JWT seemed to pass validation but couldn't be unmarshalled to %T: %w", errors.BadRequest, claims, err) + } else { + return claims, nil + } +} + +func InitVerifier(ctx context.Context) error { + type extraConfigurationClaims struct { + IdTokenSigningAlgValuesSupported []string `json:"id_token_signing_alg_values_supported"` + } + provider, err := oidc.NewProvider(ctx, config.Config.MustString("auth.githubActionsOIDC.issuer")) + if err != nil { + return err + } + var claims extraConfigurationClaims + if err = provider.Claims(&claims); err != nil { + return err + } + rawVerifier = &realVerifierImplementation{ + provider.Verifier(&oidc.Config{ + // The ClientID gets compared to the "aud" claim of the returned OIDC token. + // GitHub Actions actually allows customization of the "aud" claim when the ID token is created, so + // we can't rely on it as an actual security measure. What we're trying to do is match based the + // nonstandard "actor_id" claim to a stored GitHub user ID, so we can safely ignore the "aud" claim. + SkipClientIDCheck: true, + // The library says it defaults to RS256, but GitHub includes this information at its configuration + // endpoint, so we'll grab it to be safe. + SupportedSigningAlgs: claims.IdTokenSigningAlgValuesSupported, + }), + } + verifier = rawVerifier + return nil +} diff --git a/sherlock/internal/authentication/gha_oidc/gha_oidc_claims/claims.go b/sherlock/internal/authentication/gha_oidc/gha_oidc_claims/claims.go new file mode 100644 index 000000000..4cf77ae0c --- /dev/null +++ b/sherlock/internal/authentication/gha_oidc/gha_oidc_claims/claims.go @@ -0,0 +1,38 @@ +package gha_oidc_claims + +import ( + "fmt" + "strings" +) + +type Claims struct { + // GitHub username that initiated the workflow + Actor string `json:"actor"` + // ID for the GitHub user that initiated the workflow + ActorID string `json:"actor_id"` + // Owner of the repo that the workflow ran in + RepositoryOwner string `json:"repository_owner"` + // Repo that the workflow ran in, given like / + Repository string `json:"repository"` + // The ref path to the workflow, given like //@ + WorkflowRef string `json:"workflow_ref"` + // The ID of the workflow + RunID string `json:"run_id"` + // The number of times the workflow has been attempted + RunAttempt string `json:"run_attempt"` +} + +// TrimmedRepositoryName provides the repository name without the leading owner and slash +func (c *Claims) TrimmedRepositoryName() string { + return strings.TrimPrefix(c.Repository, c.RepositoryOwner+"/") +} + +// TrimmedWorkflowPath provides the workflow path without the leading repository owner, name, and slash or the +// trailing git ref +func (c *Claims) TrimmedWorkflowPath() string { + return strings.Split(strings.TrimPrefix(c.WorkflowRef, c.Repository+"/"), "@")[0] +} + +func (c *Claims) WorkflowURL() string { + return fmt.Sprintf("https://github.com/%s/actions/runs/%s/attempts/%s", c.Repository, c.RunID, c.RunAttempt) +} diff --git a/sherlock/internal/authentication/gha_oidc/gha_oidc_mocks/mock_mockable_verifier.go b/sherlock/internal/authentication/gha_oidc/gha_oidc_mocks/mock_mockable_verifier.go new file mode 100644 index 000000000..d312f922f --- /dev/null +++ b/sherlock/internal/authentication/gha_oidc/gha_oidc_mocks/mock_mockable_verifier.go @@ -0,0 +1,91 @@ +// Code generated by mockery v2.32.4. DO NOT EDIT. + +package gha_oidc_mocks + +import ( + context "context" + + gha_oidc_claims "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc/gha_oidc_claims" + + mock "github.com/stretchr/testify/mock" +) + +// MockMockableVerifier is an autogenerated mock type for the mockableVerifier type +type MockMockableVerifier struct { + mock.Mock +} + +type MockMockableVerifier_Expecter struct { + mock *mock.Mock +} + +func (_m *MockMockableVerifier) EXPECT() *MockMockableVerifier_Expecter { + return &MockMockableVerifier_Expecter{mock: &_m.Mock} +} + +// VerifyAndParseClaims provides a mock function with given fields: ctx, rawIDToken +func (_m *MockMockableVerifier) VerifyAndParseClaims(ctx context.Context, rawIDToken string) (gha_oidc_claims.Claims, error) { + ret := _m.Called(ctx, rawIDToken) + + var r0 gha_oidc_claims.Claims + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string) (gha_oidc_claims.Claims, error)); ok { + return rf(ctx, rawIDToken) + } + if rf, ok := ret.Get(0).(func(context.Context, string) gha_oidc_claims.Claims); ok { + r0 = rf(ctx, rawIDToken) + } else { + r0 = ret.Get(0).(gha_oidc_claims.Claims) + } + + if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = rf(ctx, rawIDToken) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// MockMockableVerifier_VerifyAndParseClaims_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'VerifyAndParseClaims' +type MockMockableVerifier_VerifyAndParseClaims_Call struct { + *mock.Call +} + +// VerifyAndParseClaims is a helper method to define mock.On call +// - ctx context.Context +// - rawIDToken string +func (_e *MockMockableVerifier_Expecter) VerifyAndParseClaims(ctx interface{}, rawIDToken interface{}) *MockMockableVerifier_VerifyAndParseClaims_Call { + return &MockMockableVerifier_VerifyAndParseClaims_Call{Call: _e.mock.On("VerifyAndParseClaims", ctx, rawIDToken)} +} + +func (_c *MockMockableVerifier_VerifyAndParseClaims_Call) Run(run func(ctx context.Context, rawIDToken string)) *MockMockableVerifier_VerifyAndParseClaims_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string)) + }) + return _c +} + +func (_c *MockMockableVerifier_VerifyAndParseClaims_Call) Return(_a0 gha_oidc_claims.Claims, _a1 error) *MockMockableVerifier_VerifyAndParseClaims_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *MockMockableVerifier_VerifyAndParseClaims_Call) RunAndReturn(run func(context.Context, string) (gha_oidc_claims.Claims, error)) *MockMockableVerifier_VerifyAndParseClaims_Call { + _c.Call.Return(run) + return _c +} + +// NewMockMockableVerifier creates a new instance of MockMockableVerifier. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewMockMockableVerifier(t interface { + mock.TestingT + Cleanup(func()) +}) *MockMockableVerifier { + mock := &MockMockableVerifier{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/sherlock/internal/authentication/gha_oidc/init_verifier.go b/sherlock/internal/authentication/gha_oidc/init_verifier.go deleted file mode 100644 index a8055864d..000000000 --- a/sherlock/internal/authentication/gha_oidc/init_verifier.go +++ /dev/null @@ -1,36 +0,0 @@ -package gha_oidc - -import ( - "context" - "github.com/broadinstitute/sherlock/sherlock/internal/config" - "github.com/coreos/go-oidc" -) - -var verifier *oidc.IDTokenVerifier - -type extraConfigurationClaims struct { - IdTokenSigningAlgValuesSupported []string `json:"id_token_signing_alg_values_supported"` -} - -func InitVerifier(ctx context.Context) error { - provider, err := oidc.NewProvider(ctx, config.Config.MustString("auth.githubActionsOIDC.issuer")) - if err != nil { - return err - } - var claims extraConfigurationClaims - if err = provider.Claims(&claims); err != nil { - return err - } - - verifier = provider.Verifier(&oidc.Config{ - // The ClientID gets compared to the "aud" claim of the returned OIDC token. - // GitHub Actions actually allows customization of the "aud" claim when the ID token is created, so - // we can't rely on it as an actual security measure. What we're trying to do is match based the - // nonstandard "actor_id" claim to a stored GitHub user ID, so we can safely ignore the "aud" claim. - SkipClientIDCheck: true, - // The library says it defaults to RS256, but GitHub includes this information at its configuration - // endpoint, so we'll grab it to be safe. - SupportedSigningAlgs: claims.IdTokenSigningAlgValuesSupported, - }) - return nil -} diff --git a/sherlock/internal/authentication/gha_oidc/parse_header.go b/sherlock/internal/authentication/gha_oidc/parse_header.go index a2d3e1a6e..fc5bcf0ba 100644 --- a/sherlock/internal/authentication/gha_oidc/parse_header.go +++ b/sherlock/internal/authentication/gha_oidc/parse_header.go @@ -2,67 +2,29 @@ package gha_oidc import ( "fmt" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc/gha_oidc_claims" "github.com/broadinstitute/sherlock/sherlock/internal/config" - "github.com/broadinstitute/sherlock/sherlock/internal/errors" "github.com/broadinstitute/sherlock/sherlock/internal/slack" "github.com/gin-gonic/gin" - "strings" + "github.com/rs/zerolog/log" ) const ( - ghaOidcHeader = "X-GHA-OIDC-JWT" + Header = "X-GHA-OIDC-JWT" ) -type Claims struct { - // GitHub username that initiated the workflow - Actor string `json:"actor"` - // ID for the GitHub user that initiated the workflow - ActorID string `json:"actor_id"` - // Owner of the repo that the workflow ran in - RepositoryOwner string `json:"repository_owner"` - // Repo that the workflow ran in, given like / - Repository string `json:"repository"` - // The ref path to the workflow, given like //@ - WorkflowRef string `json:"workflow_ref"` - // The ID of the workflow - RunID string `json:"run_id"` - // The number of times the workflow has been attempted - RunAttempt string `json:"run_attempt"` -} - -// TrimmedRepositoryName provides the repository name without the leading owner and slash -func (c *Claims) TrimmedRepositoryName() string { - return strings.TrimPrefix(c.Repository, c.RepositoryOwner+"/") -} - -// TrimmedWorkflowPath provides the workflow path without the leading repository owner, name, and slash or the -// trailing git ref -func (c *Claims) TrimmedWorkflowPath() string { - return strings.Split(strings.TrimPrefix(c.WorkflowRef, c.Repository+"/"), "@")[0] -} - -func (c *Claims) WorkflowURL() string { - return fmt.Sprintf("https://github.com/%s/actions/runs/%s/attempts/%s", c.Repository, c.RunID, c.RunAttempt) -} - -func ParseHeader(ctx *gin.Context) (*Claims, error) { - if verifier == nil { - return nil, fmt.Errorf("(%s) gha_oidc.ParseHeader was called before gha_oidc.InitVerifier", errors.InternalServerError) - } - ghaOidcJwt := ctx.GetHeader(ghaOidcHeader) +func ParseHeader(ctx *gin.Context) (*gha_oidc_claims.Claims, error) { + ghaOidcJwt := ctx.GetHeader(Header) if ghaOidcJwt == "" { return nil, nil } - payload, err := verifier.Verify(ctx, ghaOidcJwt) - if err != nil { - return nil, fmt.Errorf("(%s) failed to validate GHA OIDC JWT in '%s' header: %w", errors.BadRequest, ghaOidcHeader, err) - } else if payload == nil { - return nil, fmt.Errorf("(%s) GHA OIDC JWT seemed to pass validation but payload was nil", errors.BadRequest) + if verifier == nil { + log.Info().Msgf("AUTH | GHA OIDC JWT observed in '%s' header but no verifier had been initialized", Header) + return nil, nil } - - var claims Claims - if err = payload.Claims(&claims); err != nil { - return nil, fmt.Errorf("(%s) GHA OIDC JWT seemed to pass validation but couldn't be unmarshalled to %T: %w", errors.BadRequest, claims, err) + claims, err := verifier.VerifyAndParseClaims(ctx, ghaOidcJwt) + if err != nil { + return nil, err } var repositoryOwnerAccepted bool diff --git a/sherlock/internal/authentication/gha_oidc/parse_header_test.go b/sherlock/internal/authentication/gha_oidc/parse_header_test.go new file mode 100644 index 000000000..d68f8cfa5 --- /dev/null +++ b/sherlock/internal/authentication/gha_oidc/parse_header_test.go @@ -0,0 +1,110 @@ +package gha_oidc + +import ( + "fmt" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc/gha_oidc_claims" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc/gha_oidc_mocks" + "github.com/broadinstitute/sherlock/sherlock/internal/config" + "github.com/broadinstitute/sherlock/sherlock/internal/slack" + "github.com/broadinstitute/sherlock/sherlock/internal/slack/slack_mocks" + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "net/http" + "net/http/httptest" + "testing" +) + +func TestParseHeader(t *testing.T) { + gin.SetMode(gin.TestMode) + config.LoadTestConfig() + engine := gin.New() + var gotClaims *gha_oidc_claims.Claims + var gotErr error + engine.Use(func(ctx *gin.Context) { + gotClaims, gotErr = ParseHeader(ctx) + ctx.JSON(http.StatusOK, struct{}{}) + }) + tests := []struct { + name string + headerContents string + mockConfig func(v *gha_oidc_mocks.MockMockableVerifier) + slackMockConfig func(c *slack_mocks.MockMockableClient) + wantClaims *gha_oidc_claims.Claims + wantErr assert.ErrorAssertionFunc + }{ + { + name: "empty", + headerContents: "", + mockConfig: func(v *gha_oidc_mocks.MockMockableVerifier) {}, + wantClaims: nil, + wantErr: assert.NoError, + }, + { + name: "no verifier", + headerContents: "some header", + mockConfig: nil, + wantClaims: nil, + wantErr: assert.NoError, + }, + { + name: "fails validation", + headerContents: "some header", + mockConfig: func(v *gha_oidc_mocks.MockMockableVerifier) { + v.EXPECT().VerifyAndParseClaims(mock.Anything, "some header").Return( + gha_oidc_claims.Claims{}, fmt.Errorf("some error")) + }, + wantClaims: nil, + wantErr: assert.Error, + }, + { + name: "repository owner not accepted", + headerContents: "some header", + mockConfig: func(v *gha_oidc_mocks.MockMockableVerifier) { + v.EXPECT().VerifyAndParseClaims(mock.Anything, "some header").Return( + gha_oidc_claims.Claims{ + RepositoryOwner: "some unknown owner", + }, nil) + }, + slackMockConfig: func(c *slack_mocks.MockMockableClient) { + c.EXPECT().SendMessageContext(mock.Anything, mock.AnythingOfType("string"), mock.Anything, mock.Anything). + Return("", "", "", nil) + }, + wantClaims: nil, + wantErr: assert.NoError, + }, + { + name: "repository owner not accepted", + headerContents: "some header", + mockConfig: func(v *gha_oidc_mocks.MockMockableVerifier) { + v.EXPECT().VerifyAndParseClaims(mock.Anything, "some header").Return( + gha_oidc_claims.Claims{ + RepositoryOwner: config.Config.Strings("auth.githubActionsOIDC.allowedOrganizations")[0], + Repository: "owner/name", + }, nil) + }, + wantClaims: &gha_oidc_claims.Claims{ + RepositoryOwner: config.Config.Strings("auth.githubActionsOIDC.allowedOrganizations")[0], + Repository: "owner/name", + }, + wantErr: assert.NoError, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + slack.UseMockedClient(t, tt.slackMockConfig, func() { + UseMockedVerifier(t, tt.mockConfig, func() { + request, err := http.NewRequest(http.MethodGet, "/", nil) + assert.NoError(t, err) + request.Header.Set(Header, tt.headerContents) + recorder := httptest.NewRecorder() + engine.ServeHTTP(recorder, request) + if !tt.wantErr(t, gotErr, fmt.Sprintf("verifier.Verify(ctx, %s)", tt.headerContents)) { + return + } + assert.Equal(t, tt.wantClaims, gotClaims) + }) + }) + }) + } +} diff --git a/sherlock/internal/authentication/gha_oidc/verifier.go b/sherlock/internal/authentication/gha_oidc/verifier.go new file mode 100644 index 000000000..7caab1825 --- /dev/null +++ b/sherlock/internal/authentication/gha_oidc/verifier.go @@ -0,0 +1,36 @@ +package gha_oidc + +import ( + "context" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc/gha_oidc_claims" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc/gha_oidc_mocks" + "testing" +) + +// `make generate-mocks` from the root of the repo to regenerate (you'll need to `brew install mockery`) +type mockableVerifier interface { + VerifyAndParseClaims(ctx context.Context, rawIDToken string) (gha_oidc_claims.Claims, error) +} + +var ( + // verifier is what functions in this package should use whenever possible. It exposes a mockable + // *oidc.IDTokenVerifier. + verifier mockableVerifier + // rawVerifier is the backup to verifier (mainly for use during development) and it will only ever + // hold a *realVerifierImplementation that exposes all of *oidc.IDTokenVerifier. + rawVerifier *realVerifierImplementation +) + +func UseMockedVerifier(t *testing.T, config func(v *gha_oidc_mocks.MockMockableVerifier), callback func()) { + if config == nil { + callback() + return + } + v := gha_oidc_mocks.NewMockMockableVerifier(t) + config(v) + temp := verifier + verifier = v + callback() + v.AssertExpectations(t) + verifier = temp +} diff --git a/sherlock/internal/authentication/iap/parse_header.go b/sherlock/internal/authentication/iap/parse_header.go index 6dfc9014f..31aac32a5 100644 --- a/sherlock/internal/authentication/iap/parse_header.go +++ b/sherlock/internal/authentication/iap/parse_header.go @@ -8,15 +8,15 @@ import ( ) const ( - iapHeader = "X-Goog-IAP-JWT-Assertion" + header = "X-Goog-IAP-JWT-Assertion" emailClaim = "email" subClaim = "sub" ) func ParseHeader(ctx *gin.Context) (email string, googleID string, err error) { - iapJWT := ctx.GetHeader(iapHeader) + iapJWT := ctx.GetHeader(header) if iapJWT == "" { - return "", "", fmt.Errorf("(%s) no '%s' header set, IAP authentication required", errors.ProxyAuthenticationRequired, iapHeader) + return "", "", fmt.Errorf("(%s) no '%s' header set, IAP authentication required", errors.ProxyAuthenticationRequired, header) } // Sherlock is deployed behind an Apache proxy that checks that it is correctly wrapped by IAP, so we don't @@ -24,7 +24,7 @@ func ParseHeader(ctx *gin.Context) (email string, googleID string, err error) { // this is just the easiest way to decode the JWT payload. payload, err := idtoken.Validate(ctx, iapJWT, "") if err != nil { - return "", "", fmt.Errorf("(%s) failed to validate IAP JWT in '%s' header: %w", errors.ProxyAuthenticationRequired, iapHeader, err) + return "", "", fmt.Errorf("(%s) failed to validate IAP JWT in '%s' header: %w", errors.ProxyAuthenticationRequired, header, err) } else if payload == nil { return "", "", fmt.Errorf("(%s) IAP JWT seemed to pass validation but payload was nil", errors.ProxyAuthenticationRequired) } diff --git a/sherlock/internal/authentication/local_user/parse_header.go b/sherlock/internal/authentication/local_user/parse_header.go index 7c5a161c7..fc17e04f7 100644 --- a/sherlock/internal/authentication/local_user/parse_header.go +++ b/sherlock/internal/authentication/local_user/parse_header.go @@ -9,32 +9,32 @@ import ( // TODO (Jack): Make this package actually grab and cache the ADC credentials, so we use the user's real email/ID const ( - EmailControlHeader = "X-SHERLOCK-DEBUG-EMAIL" - FallbackEmail = "fallback-fake-sherlock-user@broadinstitute.org" - GoogleIDControlHeader = "X-SHERLOCK-DEBUG-GOOGLE-ID" - FallbackGoogleID = "1234fallback1234" - SuitabilityControlHeader = "X-SHERLOCK-DEBUG-SUITABLE" + emailControlHeader = "X-SHERLOCK-DEBUG-EMAIL" + fallbackEmail = "fallback-fake-sherlock-user@broadinstitute.org" + googleIDControlHeader = "X-SHERLOCK-DEBUG-GOOGLE-ID" + fallbackGoogleID = "1234fallback1234" + suitabilityControlHeader = "X-SHERLOCK-DEBUG-SUITABLE" ) var ( - LocalUserEmail = FallbackEmail - LocalUserGoogleID = FallbackGoogleID + LocalUserEmail = fallbackEmail + LocalUserGoogleID = fallbackGoogleID LocalUserSuitable = true ) func ParseHeader(ctx *gin.Context) (email string, googleID string, err error) { - if emailHeader := ctx.GetHeader(EmailControlHeader); emailHeader != "" { + if emailHeader := ctx.GetHeader(emailControlHeader); emailHeader != "" { LocalUserEmail = emailHeader } - if googleIDHeader := ctx.GetHeader(GoogleIDControlHeader); googleIDHeader != "" { + if googleIDHeader := ctx.GetHeader(googleIDControlHeader); googleIDHeader != "" { LocalUserGoogleID = googleIDHeader } - if suitabilityHeader := ctx.GetHeader(SuitabilityControlHeader); suitabilityHeader == "" { + if suitabilityHeader := ctx.GetHeader(suitabilityControlHeader); suitabilityHeader == "" { LocalUserSuitable = true } else if LocalUserSuitable, err = strconv.ParseBool(suitabilityHeader); err != nil { - return "", "", fmt.Errorf("failed to parse boolean from %s header: %w", SuitabilityControlHeader, err) + return "", "", fmt.Errorf("failed to parse boolean from %s header: %w", suitabilityControlHeader, err) } return LocalUserEmail, LocalUserGoogleID, nil diff --git a/sherlock/internal/authentication/middleware.go b/sherlock/internal/authentication/middleware.go index 9b9717dbe..c402dbd60 100644 --- a/sherlock/internal/authentication/middleware.go +++ b/sherlock/internal/authentication/middleware.go @@ -14,25 +14,21 @@ import ( // Middleware returns an ordered list of middleware to authenticate requests, enabling request handlers to use functions // like ShouldUseDB, ShouldUseUser, and ShouldUseGithubClaims. func Middleware(db *gorm.DB) gin.HandlersChain { + var userWhoConnectedMiddleware gin.HandlerFunc if config.Config.String("mode") == "debug" { if config.Config.Bool("auth.createTestUsersInMiddleware") { log.Info().Msgf("AUTH | using test authentication; will create test users from middleware") - return gin.HandlersChain{ - setUserWhoConnected(db, test_users.ParseHeader, authentication_method.TEST), - setDatabaseWithUser(db), - } + userWhoConnectedMiddleware = setUserWhoConnected(db, test_users.ParseHeader, authentication_method.TEST) } else { log.Info().Msgf("AUTH | using local authentication; will create a local user from middleware") - return gin.HandlersChain{ - setUserWhoConnected(db, local_user.ParseHeader, authentication_method.LOCAL), - setDatabaseWithUser(db), - } + userWhoConnectedMiddleware = setUserWhoConnected(db, local_user.ParseHeader, authentication_method.LOCAL) } } else { - return gin.HandlersChain{ - setUserWhoConnected(db, iap.ParseHeader, authentication_method.IAP), - setGithubClaimsAndEscalateUser(db), - setDatabaseWithUser(db), - } + userWhoConnectedMiddleware = setUserWhoConnected(db, iap.ParseHeader, authentication_method.IAP) + } + return gin.HandlersChain{ + userWhoConnectedMiddleware, + setGithubClaimsAndEscalateUser(db), + setDatabaseWithUser(db), } } diff --git a/sherlock/internal/authentication/test_users/parse_header.go b/sherlock/internal/authentication/test_users/parse_header.go index a5c221136..ec70b366c 100644 --- a/sherlock/internal/authentication/test_users/parse_header.go +++ b/sherlock/internal/authentication/test_users/parse_header.go @@ -7,7 +7,7 @@ import ( ) const ( - SuitabilityControlHeader = "X-SHERLOCK-TEST-SUITABLE" + suitableControlHeader = "X-SHERLOCK-TEST-SUITABLE" SuitableTestUserEmail = "suitable-test-email@broadinstitute.org" SuitableTestUserGoogleID = "12341234" NonSuitableTestUserEmail = "non-suitable-test-email@broadinstitute.org" @@ -15,10 +15,10 @@ const ( ) func ParseHeader(ctx *gin.Context) (email string, googleID string, err error) { - if header := ctx.GetHeader(SuitabilityControlHeader); header == "" { + if header := ctx.GetHeader(suitableControlHeader); header == "" { return SuitableTestUserEmail, SuitableTestUserGoogleID, nil } else if suitable, err := strconv.ParseBool(header); err != nil { - return "", "", fmt.Errorf("failed to parse boolean from %s header: %w", SuitabilityControlHeader, err) + return "", "", fmt.Errorf("failed to parse boolean from %v suitableControlHeader: %w", suitable, err) } else if suitable { return SuitableTestUserEmail, SuitableTestUserGoogleID, nil } else { diff --git a/sherlock/internal/authentication/test_users/parse_header_test.go b/sherlock/internal/authentication/test_users/parse_header_test.go index 62833bcc7..3cddf6abb 100644 --- a/sherlock/internal/authentication/test_users/parse_header_test.go +++ b/sherlock/internal/authentication/test_users/parse_header_test.go @@ -12,32 +12,34 @@ func TestParseHeader(t *testing.T) { t.Run("default", func(t *testing.T) { req, err := http.NewRequest("GET", "/", nil) assert.NoError(t, err) - email, googleID := ParseHeader(&gin.Context{Request: req}) + email, googleID, err := ParseHeader(&gin.Context{Request: req}) + assert.NoError(t, err) assert.Equal(t, SuitableTestUserEmail, email) assert.Equal(t, SuitableTestUserGoogleID, googleID) }) t.Run("explicitly suitable", func(t *testing.T) { req, err := http.NewRequest("GET", "/", nil) assert.NoError(t, err) - req.Header.Set(SuitabilityControlHeader, strconv.FormatBool(true)) - email, googleID := ParseHeader(&gin.Context{Request: req}) + req.Header.Set(suitableControlHeader, strconv.FormatBool(true)) + email, googleID, err := ParseHeader(&gin.Context{Request: req}) + assert.NoError(t, err) assert.Equal(t, SuitableTestUserEmail, email) assert.Equal(t, SuitableTestUserGoogleID, googleID) }) t.Run("explicitly non-suitable", func(t *testing.T) { req, err := http.NewRequest("GET", "/", nil) assert.NoError(t, err) - req.Header.Set(SuitabilityControlHeader, strconv.FormatBool(false)) - email, googleID := ParseHeader(&gin.Context{Request: req}) + req.Header.Set(suitableControlHeader, strconv.FormatBool(false)) + email, googleID, err := ParseHeader(&gin.Context{Request: req}) + assert.NoError(t, err) assert.Equal(t, NonSuitableTestUserEmail, email) assert.Equal(t, NonSuitableTestUserGoogleID, googleID) }) - t.Run("panics if can't parse header", func(t *testing.T) { + t.Run("errors if can't parse suitableControlHeader", func(t *testing.T) { req, err := http.NewRequest("GET", "/", nil) assert.NoError(t, err) - req.Header.Set(SuitabilityControlHeader, "something that isn't boolean") - assert.Panics(t, func() { - ParseHeader(&gin.Context{Request: req}) - }) + req.Header.Set(suitableControlHeader, "something that isn't boolean") + _, _, err = ParseHeader(&gin.Context{Request: req}) + assert.ErrorContains(t, err, "failed to parse boolean") }) } diff --git a/sherlock/internal/authentication/test_users/test_suite_helper.go b/sherlock/internal/authentication/test_users/test_suite_helper.go index 7ede69eac..27f8629b6 100644 --- a/sherlock/internal/authentication/test_users/test_suite_helper.go +++ b/sherlock/internal/authentication/test_users/test_suite_helper.go @@ -13,19 +13,19 @@ import ( // headers from tests) type TestUserHelper struct{} -// UseSuitableUserFor sets SuitabilityControlHeader such that ParseHeader will supply SuitableTestUserEmail. +// UseSuitableUserFor sets suitableControlHeader such that ParseHeader will supply SuitableTestUserEmail. // This is ParseHeader's default behavior, but this function can be helpful for clarity or undoing // UseNonSuitableUserFor. func (h TestUserHelper) UseSuitableUserFor(req *http.Request) *http.Request { return h.selectUserForRequestBySuitability(req, true) } -// UseNonSuitableUserFor sets SuitabilityControlHeader such that ParseHeader will supply NonSuitableTestUserEmail. +// UseNonSuitableUserFor sets suitableControlHeader such that ParseHeader will supply NonSuitableTestUserEmail. func (h TestUserHelper) UseNonSuitableUserFor(req *http.Request) *http.Request { return h.selectUserForRequestBySuitability(req, false) } func (_ TestUserHelper) selectUserForRequestBySuitability(req *http.Request, suitable bool) *http.Request { - req.Header.Set(SuitabilityControlHeader, strconv.FormatBool(suitable)) + req.Header.Set(suitableControlHeader, strconv.FormatBool(suitable)) return req } diff --git a/sherlock/internal/authentication/test_users/test_suite_helper_test.go b/sherlock/internal/authentication/test_users/test_suite_helper_test.go index bfaea0262..e148a854c 100644 --- a/sherlock/internal/authentication/test_users/test_suite_helper_test.go +++ b/sherlock/internal/authentication/test_users/test_suite_helper_test.go @@ -11,7 +11,7 @@ func TestUseSuitableUserFor(t *testing.T) { req, err := http.NewRequest("GET", "/", nil) assert.NoError(t, err) TestUserHelper{}.UseSuitableUserFor(req) - suitable, err := strconv.ParseBool(req.Header.Get(SuitabilityControlHeader)) + suitable, err := strconv.ParseBool(req.Header.Get(suitableControlHeader)) assert.NoError(t, err) assert.True(t, suitable) } @@ -20,7 +20,7 @@ func TestUseNonSuitableUserFor(t *testing.T) { req, err := http.NewRequest("GET", "/", nil) assert.NoError(t, err) TestUserHelper{}.UseNonSuitableUserFor(req) - suitable, err := strconv.ParseBool(req.Header.Get(SuitabilityControlHeader)) + suitable, err := strconv.ParseBool(req.Header.Get(suitableControlHeader)) assert.NoError(t, err) assert.False(t, suitable) } diff --git a/sherlock/internal/authentication/user_provider.go b/sherlock/internal/authentication/user_provider.go index 28e445a27..d7d8f4704 100644 --- a/sherlock/internal/authentication/user_provider.go +++ b/sherlock/internal/authentication/user_provider.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/broadinstitute/sherlock/sherlock/internal/authentication/authentication_method" "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc/gha_oidc_claims" "github.com/broadinstitute/sherlock/sherlock/internal/errors" "github.com/broadinstitute/sherlock/sherlock/internal/models" "github.com/gin-gonic/gin" @@ -58,22 +59,26 @@ func setGithubClaimsAndEscalateUser(db *gorm.DB) gin.HandlerFunc { } else if claims != nil { ctx.Set(ctxGithubClaimsFieldName, claims) - var matchingGithubUsers []models.User - if err = db.Where(&models.User{GithubID: &claims.ActorID}).Limit(1).Find(&matchingGithubUsers).Error; err != nil { - errors.AbortRequest(ctx, fmt.Errorf("failed to query for users matching GitHub Actions claims: %w", err)) - return - } else if len(matchingGithubUsers) > 0 { - user := matchingGithubUsers[0] - if oldUser, err := ShouldUseUser(ctx); err != nil { - log.Warn().Err(err).Msg("AUTH | was unable to read old user to escalate user based on GitHub claims") + if claims.ActorID != "" { + var matchingGithubUsers []models.User + if err = db.Where(&models.User{GithubID: &claims.ActorID}).Limit(1).Find(&matchingGithubUsers).Error; err != nil { + errors.AbortRequest(ctx, fmt.Errorf("failed to query for users matching GitHub Actions claims: %w", err)) + return + } else if len(matchingGithubUsers) > 0 { + user := matchingGithubUsers[0] + if oldUser, err := ShouldUseUser(ctx); err != nil { + log.Warn().Err(err).Msg("AUTH | was unable to read old user to escalate user based on GitHub claims") + } else { + user.AuthenticationMethod = authentication_method.GHA + user.Via = oldUser + ctx.Set(ctxUserFieldName, &user) + log.Debug().Str("workflow", claims.WorkflowURL()).Msgf("AUTH | recognized %s connecting through GitHub Actions via %s", user.Email, oldUser.Email) + } } else { - user.AuthenticationMethod = authentication_method.GHA - user.Via = oldUser - ctx.Set(ctxUserFieldName, &user) - log.Debug().Str("workflow", claims.WorkflowURL()).Msgf("AUTH | recognized %s connecting through GitHub Actions via %s", user.Email, oldUser.Email) + log.Debug().Str("workflow", claims.WorkflowURL()).Msgf("AUTH | ignoring unknown user '%s' referenced by GitHub Actions claims", claims.Actor) } } else { - log.Debug().Str("workflow", claims.WorkflowURL()).Msgf("AUTH | ignoring unknown user %s referenced by GitHub Actions claims", claims.Actor) + log.Debug().Str("workflow", claims.WorkflowURL()).Msgf("AUTH | ignoring GitHub Actions claims for user escalation since actor ID was empty") } } } @@ -111,12 +116,12 @@ func MustUseUser(ctx *gin.Context) (*models.User, error) { // Note that the Actor/ActorID fields of the gha_oidc.Claims must not be correlated to a models.User. That correlation // is the responsibility of the authentication package; use ShouldUseUser or MustUseUser to access the models.User // associated with the request. -func ShouldUseGithubClaims(ctx *gin.Context) (*gha_oidc.Claims, error) { +func ShouldUseGithubClaims(ctx *gin.Context) (*gha_oidc_claims.Claims, error) { claimsValue, exists := ctx.Get(ctxGithubClaimsFieldName) if !exists { return nil, fmt.Errorf("(%s) GitHub OIDC claims were not present", errors.InternalServerError) } - claims, ok := claimsValue.(*gha_oidc.Claims) + claims, ok := claimsValue.(*gha_oidc_claims.Claims) if !ok { return nil, fmt.Errorf("(%s) GitHub OIDC claims middleware may be misconfigured: represented as %T", errors.InternalServerError, claimsValue) } diff --git a/sherlock/internal/github/client.go b/sherlock/internal/github/client.go index 7c309c5c2..ecb1ee61a 100644 --- a/sherlock/internal/github/client.go +++ b/sherlock/internal/github/client.go @@ -234,6 +234,10 @@ func (c *MockClient) assertExpectations(t *testing.T) { // UseMockedClient lets both internal and external callers take advantage of this // package's mocking capabilities by running tests inside the callback. func UseMockedClient(t *testing.T, config func(c *MockClient), callback func()) { + if config == nil { + callback() + return + } c := MockClient{ Actions: github_mocks.NewMockMockableActionsClient(t), Activity: github_mocks.NewMockMockableActivityClient(t), diff --git a/sherlock/internal/slack/client.go b/sherlock/internal/slack/client.go index d1b15aa6a..eb9e27177 100644 --- a/sherlock/internal/slack/client.go +++ b/sherlock/internal/slack/client.go @@ -29,6 +29,10 @@ var ( ) func UseMockedClient(t *testing.T, config func(c *slack_mocks.MockMockableClient), callback func()) { + if config == nil { + callback() + return + } c := slack_mocks.NewMockMockableClient(t) config(c) temp := client From 03e58bacab2953290007ae4a9fca844cdf56ef6b Mon Sep 17 00:00:00 2001 From: Jack Warren Date: Mon, 13 Nov 2023 16:05:34 -0500 Subject: [PATCH 5/6] claims tests --- .../gha_oidc/gha_oidc_claims/claims_test.go | 172 ++++++++++++++++++ 1 file changed, 172 insertions(+) create mode 100644 sherlock/internal/authentication/gha_oidc/gha_oidc_claims/claims_test.go diff --git a/sherlock/internal/authentication/gha_oidc/gha_oidc_claims/claims_test.go b/sherlock/internal/authentication/gha_oidc/gha_oidc_claims/claims_test.go new file mode 100644 index 000000000..ce6bb92c2 --- /dev/null +++ b/sherlock/internal/authentication/gha_oidc/gha_oidc_claims/claims_test.go @@ -0,0 +1,172 @@ +package gha_oidc_claims + +import "testing" + +func TestClaims_TrimmedRepositoryName(t *testing.T) { + type fields struct { + Actor string + ActorID string + RepositoryOwner string + Repository string + WorkflowRef string + RunID string + RunAttempt string + } + tests := []struct { + name string + fields fields + want string + }{ + { + name: "trims owner", + fields: fields{ + RepositoryOwner: "abc", + Repository: "abc/def", + }, + want: "def", + }, + { + name: "if owner empty", + fields: fields{ + Repository: "abc/def", + }, + want: "abc/def", + }, + { + name: "if repository empty", + fields: fields{ + RepositoryOwner: "abc", + }, + want: "", + }, + { + name: "if mismatched", + fields: fields{ + RepositoryOwner: "abc", + Repository: "def/ghi", + }, + want: "def/ghi", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &Claims{ + Actor: tt.fields.Actor, + ActorID: tt.fields.ActorID, + RepositoryOwner: tt.fields.RepositoryOwner, + Repository: tt.fields.Repository, + WorkflowRef: tt.fields.WorkflowRef, + RunID: tt.fields.RunID, + RunAttempt: tt.fields.RunAttempt, + } + if got := c.TrimmedRepositoryName(); got != tt.want { + t.Errorf("TrimmedRepositoryName() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestClaims_TrimmedWorkflowPath(t *testing.T) { + type fields struct { + Actor string + ActorID string + RepositoryOwner string + Repository string + WorkflowRef string + RunID string + RunAttempt string + } + tests := []struct { + name string + fields fields + want string + }{ + { + name: "trims repository", + fields: fields{ + Repository: "abc/def", + WorkflowRef: "abc/def/ghi", + }, + want: "ghi", + }, + { + name: "trims after @", + fields: fields{ + WorkflowRef: "123@456", + }, + want: "123", + }, + { + name: "together", + fields: fields{ + Repository: "broadinstitute/terra-github-workflows", + WorkflowRef: "broadinstitute/terra-github-workflows/.github/workflows/bee-create.yaml@refs/heads/main", + }, + want: ".github/workflows/bee-create.yaml", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &Claims{ + Actor: tt.fields.Actor, + ActorID: tt.fields.ActorID, + RepositoryOwner: tt.fields.RepositoryOwner, + Repository: tt.fields.Repository, + WorkflowRef: tt.fields.WorkflowRef, + RunID: tt.fields.RunID, + RunAttempt: tt.fields.RunAttempt, + } + if got := c.TrimmedWorkflowPath(); got != tt.want { + t.Errorf("TrimmedWorkflowPath() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestClaims_WorkflowURL(t *testing.T) { + type fields struct { + Actor string + ActorID string + RepositoryOwner string + Repository string + WorkflowRef string + RunID string + RunAttempt string + } + tests := []struct { + name string + fields fields + want string + }{ + { + name: "empty", + fields: fields{}, + want: "https://github.com//actions/runs//attempts/", + }, + { + name: "normal", + fields: fields{ + Repository: "broadinstitute/terra-github-workflows", + RunID: "123456", + RunAttempt: "1", + }, + want: "https://github.com/broadinstitute/terra-github-workflows/actions/runs/123456/attempts/1", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &Claims{ + Actor: tt.fields.Actor, + ActorID: tt.fields.ActorID, + RepositoryOwner: tt.fields.RepositoryOwner, + Repository: tt.fields.Repository, + WorkflowRef: tt.fields.WorkflowRef, + RunID: tt.fields.RunID, + RunAttempt: tt.fields.RunAttempt, + } + if got := c.WorkflowURL(); got != tt.want { + t.Errorf("WorkflowURL() = %v, want %v", got, tt.want) + } + }) + } +} From 10d91123c376cb0afadbf9daac48419559f93576 Mon Sep 17 00:00:00 2001 From: Jack Warren Date: Mon, 13 Nov 2023 16:05:42 -0500 Subject: [PATCH 6/6] the whole reason i did this --- .../api/sherlock/ci_runs_v3_upsert_test.go | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/sherlock/internal/api/sherlock/ci_runs_v3_upsert_test.go b/sherlock/internal/api/sherlock/ci_runs_v3_upsert_test.go index be425bac5..811c689ba 100644 --- a/sherlock/internal/api/sherlock/ci_runs_v3_upsert_test.go +++ b/sherlock/internal/api/sherlock/ci_runs_v3_upsert_test.go @@ -3,6 +3,9 @@ package sherlock import ( "fmt" "github.com/broadinstitute/sherlock/go-shared/pkg/utils" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc/gha_oidc_claims" + "github.com/broadinstitute/sherlock/sherlock/internal/authentication/gha_oidc/gha_oidc_mocks" "github.com/broadinstitute/sherlock/sherlock/internal/deployhooks" "github.com/broadinstitute/sherlock/sherlock/internal/deprecated_controllers/v2controllers" "github.com/broadinstitute/sherlock/sherlock/internal/deprecated_models/v2models" @@ -811,3 +814,34 @@ func (s *handlerSuite) TestCiRunsV3Upsert_makeSlackMessageText() { }, })) } + +func (s *handlerSuite) TestCiRunsV3Upsert_GithubActionsClaimDefaults() { + // Note that the request body is empty! + // Normally this would result in an error due to missing fields, but suppose a GHA OIDC JWT was passed... + request := s.NewRequest(http.MethodPut, "/api/ci-runs/v3", CiRunV3Upsert{}) + request.Header.Set(gha_oidc.Header, "some GHA OIDC token") + + var got CiRunV3 + var code int + gha_oidc.UseMockedVerifier(s.T(), func(v *gha_oidc_mocks.MockMockableVerifier) { + v.EXPECT().VerifyAndParseClaims(mock.AnythingOfType("*gin.Context"), "some GHA OIDC token"). + Return(gha_oidc_claims.Claims{ + RepositoryOwner: "broadinstitute", + Repository: "broadinstitute/terra-github-workflows", + WorkflowRef: "broadinstitute/terra-github-workflows/.github/workflows/bee-create.yaml@refs/heads/main", + RunID: "123456", + RunAttempt: "1", + }, nil) + }, func() { + code = s.HandleRequest(request, &got) + }) + + s.Equal(http.StatusCreated, code) + s.NotZero(got.ID) + s.Equal(got.Platform, "github-actions") + s.Equal(got.GithubActionsOwner, "broadinstitute") + s.Equal(got.GithubActionsRepo, "terra-github-workflows") + s.Equal(got.GithubActionsRunID, uint(123456)) + s.Equal(got.GithubActionsAttemptNumber, uint(1)) + s.Equal(got.GithubActionsWorkflowPath, ".github/workflows/bee-create.yaml") +}