diff --git a/.github/workflows/sherlock-build.yaml b/.github/workflows/sherlock-build.yaml index 2abfaeb67..d0fe3fe63 100644 --- a/.github/workflows/sherlock-build.yaml +++ b/.github/workflows/sherlock-build.yaml @@ -177,6 +177,9 @@ jobs: BUILD_VERSION=${{ needs.generate-tag.outputs.tag }} - name: Run Trivy vulnerability scanner + # We only run Trivy here as dependabot, because otherwise we run it afterwards so as to not block an intentional + # build (so we can fix or work around Trivy issues) + if: ${{ github.actor == 'dependabot[bot]' }} uses: broadinstitute/dsp-appsec-trivy-action@v1 with: image: us-central1-docker.pkg.dev/dsp-artifact-registry/sherlock/sherlock:${{ needs.generate-tag.outputs.tag }} @@ -246,6 +249,16 @@ jobs: build-args: | BUILD_VERSION=${{ needs.generate-tag.outputs.tag }} + trivy-scan: + needs: [ generate-tag, build-and-publish ] + runs-on: ubuntu-latest + if: ${{ needs.generate-tag.outputs.tag != '' && github.actor != 'dependabot[bot]' }} + steps: + - name: Run Trivy vulnerability scanner + uses: broadinstitute/dsp-appsec-trivy-action@v1 + with: + image: us-central1-docker.pkg.dev/dsp-artifact-registry/sherlock/sherlock:${{ needs.generate-tag.outputs.tag }} + comment-published-image: needs: [ generate-tag, build-and-publish ] runs-on: ubuntu-latest diff --git a/sherlock/config/default_config.yaml b/sherlock/config/default_config.yaml index fd6b1a9d5..c0d3bc8b2 100644 --- a/sherlock/config/default_config.yaml +++ b/sherlock/config/default_config.yaml @@ -74,18 +74,30 @@ db: oidc: enable: true # The issuer URL of Sherlock itself. This should be scheme + host + "/oidc", because Sherlock - # serves its own OIDC provider at that sub-path. This should also generally be an in-cluster - # address, because the IAP in front of the public endpoints isn't in-spec. This is fine as long - # as we just use Sherlock as an in-cluster authorization service. + # serves its own OIDC provider at that sub-path. + # + # You can choose whether to provide a publicly addressable URL or not (e.g. a .svc.cluster.local address). + # If you provide an in-cluster address, that'll work only for stuff that's in the cluster. CLIs or other + # services won't be able to authenticate against Sherlock (at least not without /etc/hosts tweaks). + # If you provide a publicly addressable URL (presumably behind IAP), you may need to worry about services + # being able to get through IAP to contact it. For stuff in the same cluster you can use either hostAliases + # with a static clusterIP on Sherlock or a CoreDNS rewrite to make in-cluster clients route in-cluster and + # dodge IAP. + # + # (In either case, you can find yourself needing to tweak hostname resolution. Be careful that you don't + # accidentally set it up such that /api/* requests dodge IAP, because Sherlock will reject that outright. + # This is why the examples make use of sherlock-oidc as a subdomain and Sherlock supports multiple origins: + # you can use a separate hostname that goes to the same place as a way to more safely tweak resolution.) # # An example is https://sherlock-api-service.sherlock.svc.cluster.local/oidc + # Another example is https://sherlock-oidc.dsp-devops-prod.broadinstitute.org/oidc issuerUrl: http://localhost:8080/oidc # The *public* side of Sherlock's OIDC issuer. This should be a normally-accessible URL that should # go to the same destination as issuerUrl above. This is automatically used in the OIDC discovery - # config to tell clients how to have *users* authenticate against Sherlock. The downstream system - # will talk to issuerUrl in-cluster, but end-users will need to talk to publicIssuerUrl. + # config to tell clients how to have *users* authenticate against Sherlock. It's okay for this to be + # the same as the issuerUrl if that's publicly addressable. # - # An example is https://sherlock.dsp-devops-prod.broadinstitute.org/oidc + # An example is https://sherlock-oidc.dsp-devops-prod.broadinstitute.org/oidc publicIssuerUrl: http://localhost:8080/oidc # The key that Sherlock should use to AES-256 encrypt internal data it sends to clients. This is diff --git a/sherlock/internal/api/sherlock/clusters_v3_create_test.go b/sherlock/internal/api/sherlock/clusters_v3_create_test.go index 8e50abb4d..b9e898efb 100644 --- a/sherlock/internal/api/sherlock/clusters_v3_create_test.go +++ b/sherlock/internal/api/sherlock/clusters_v3_create_test.go @@ -85,10 +85,10 @@ func (s *handlerSuite) TestClustersV3Create_suitability() { GoogleProject: "google-project", Location: "some location", ClusterV3Edit: ClusterV3Edit{ - Base: utils.PointerTo("some-base"), - Address: utils.PointerTo("0.0.0.0"), - RequiresSuitability: utils.PointerTo(true), - HelmfileRef: utils.PointerTo("some-ref"), + Base: utils.PointerTo("some-base"), + Address: utils.PointerTo("0.0.0.0"), + RequiredRole: s.TestData.Role_TerraSuitableEngineer().Name, + HelmfileRef: utils.PointerTo("some-ref"), }, })), &got) diff --git a/sherlock/internal/api/sherlock/clusters_v3_delete_test.go b/sherlock/internal/api/sherlock/clusters_v3_delete_test.go index 78dec4edc..1ca33d211 100644 --- a/sherlock/internal/api/sherlock/clusters_v3_delete_test.go +++ b/sherlock/internal/api/sherlock/clusters_v3_delete_test.go @@ -51,14 +51,14 @@ func (s *handlerSuite) TestClusterV3Delete() { func (s *handlerSuite) TestClusterV3Delete_suitability() { s.SetSuitableTestUserForDB() s.NoError(s.DB.Create(&models.Cluster{ - Name: "some-name", - Provider: "azure", - AzureSubscription: "some-subscription", - Location: "some-location", - Base: utils.PointerTo("some base"), - Address: utils.PointerTo("0.0.0.0"), - RequiresSuitability: utils.PointerTo(true), - HelmfileRef: utils.PointerTo("some-ref"), + Name: "some-name", + Provider: "azure", + AzureSubscription: "some-subscription", + Location: "some-location", + Base: utils.PointerTo("some base"), + Address: utils.PointerTo("0.0.0.0"), + RequiredRoleID: utils.PointerTo(s.TestData.Role_TerraSuitableEngineer().ID), + HelmfileRef: utils.PointerTo("some-ref"), }).Error) var got errors.ErrorResponse diff --git a/sherlock/internal/api/sherlock/clusters_v3_edit_test.go b/sherlock/internal/api/sherlock/clusters_v3_edit_test.go index 4165252b1..1f56beaff 100644 --- a/sherlock/internal/api/sherlock/clusters_v3_edit_test.go +++ b/sherlock/internal/api/sherlock/clusters_v3_edit_test.go @@ -99,14 +99,14 @@ func (s *handlerSuite) TestClustersV3Edit() { func (s *handlerSuite) TestClustersV3Edit_suitability() { s.SetSuitableTestUserForDB() edit := models.Cluster{ - Name: "some-name", - Provider: "azure", - AzureSubscription: "some-subscription", - Location: "some-location", - Base: utils.PointerTo("some base"), - Address: utils.PointerTo("0.0.0.0"), - RequiresSuitability: utils.PointerTo(true), - HelmfileRef: utils.PointerTo("some-ref"), + Name: "some-name", + Provider: "azure", + AzureSubscription: "some-subscription", + Location: "some-location", + Base: utils.PointerTo("some base"), + Address: utils.PointerTo("0.0.0.0"), + RequiredRoleID: utils.PointerTo(s.TestData.Role_TerraSuitableEngineer().ID), + HelmfileRef: utils.PointerTo("some-ref"), } s.NoError(s.DB.Create(&edit).Error) @@ -124,14 +124,14 @@ func (s *handlerSuite) TestClustersV3Edit_suitability() { func (s *handlerSuite) TestClustersV3Edit_suitabilityBefore() { s.SetSuitableTestUserForDB() edit := models.Cluster{ - Name: "some-name", - Provider: "azure", - AzureSubscription: "some-subscription", - Location: "some-location", - Base: utils.PointerTo("some base"), - Address: utils.PointerTo("0.0.0.0"), - RequiresSuitability: utils.PointerTo(true), - HelmfileRef: utils.PointerTo("some-ref"), + Name: "some-name", + Provider: "azure", + AzureSubscription: "some-subscription", + Location: "some-location", + Base: utils.PointerTo("some base"), + Address: utils.PointerTo("0.0.0.0"), + RequiredRoleID: utils.PointerTo(s.TestData.Role_TerraSuitableEngineer().ID), + HelmfileRef: utils.PointerTo("some-ref"), } s.NoError(s.DB.Create(&edit).Error) @@ -150,23 +150,23 @@ func (s *handlerSuite) TestClustersV3Edit_suitabilityBefore() { func (s *handlerSuite) TestClustersV3Edit_suitabilityAfter() { s.SetNonSuitableTestUserForDB() edit := models.Cluster{ - Name: "some-name", - Provider: "azure", - AzureSubscription: "some-subscription", - Location: "some-location", - Base: utils.PointerTo("some base"), - Address: utils.PointerTo("0.0.0.0"), - RequiresSuitability: utils.PointerTo(false), - HelmfileRef: utils.PointerTo("some-ref"), + Name: "some-name", + Provider: "azure", + AzureSubscription: "some-subscription", + Location: "some-location", + Base: utils.PointerTo("some base"), + Address: utils.PointerTo("0.0.0.0"), + RequiredRoleID: utils.PointerTo(s.TestData.Role_TerraEngineer().ID), + HelmfileRef: utils.PointerTo("some-ref"), } s.NoError(s.DB.Create(&edit).Error) var got errors.ErrorResponse code := s.HandleRequest( s.UseNonSuitableUserFor(s.NewRequest("PATCH", fmt.Sprintf("/api/clusters/v3/%d", edit.ID), ClusterV3Edit{ - Base: utils.PointerTo("some other base"), - Address: utils.PointerTo("0.0.0.0"), - RequiresSuitability: utils.PointerTo(true), + Base: utils.PointerTo("some other base"), + Address: utils.PointerTo("0.0.0.0"), + RequiredRole: s.TestData.Role_TerraSuitableEngineer().Name, })), &got) s.Equal(http.StatusForbidden, code) diff --git a/sherlock/internal/api/sherlock/environments_v3_create_test.go b/sherlock/internal/api/sherlock/environments_v3_create_test.go index d8cb45c80..7142eeeb2 100644 --- a/sherlock/internal/api/sherlock/environments_v3_create_test.go +++ b/sherlock/internal/api/sherlock/environments_v3_create_test.go @@ -142,7 +142,7 @@ func (s *handlerSuite) TestEnvironmentsV3Create_suitability() { Lifecycle: "template", Name: "tempy-temp", EnvironmentV3Edit: EnvironmentV3Edit{ - RequiresSuitability: utils.PointerTo(true), + RequiredRole: s.TestData.Role_TerraSuitableEngineer().Name, }, })), &got) diff --git a/sherlock/internal/api/sherlock/environments_v3_edit_test.go b/sherlock/internal/api/sherlock/environments_v3_edit_test.go index 6ff176d16..19d027afa 100644 --- a/sherlock/internal/api/sherlock/environments_v3_edit_test.go +++ b/sherlock/internal/api/sherlock/environments_v3_edit_test.go @@ -113,7 +113,7 @@ func (s *handlerSuite) TestEnvironmentsV3Edit_suitabilityAfter() { var got errors.ErrorResponse code := s.HandleRequest( s.UseNonSuitableUserFor(s.NewRequest("PATCH", fmt.Sprintf("/api/environments/v3/%d", edit.ID), EnvironmentV3Edit{ - RequiresSuitability: utils.PointerTo(true), + RequiredRole: s.TestData.Role_TerraSuitableEngineer().Name, })), &got) s.Equal(http.StatusForbidden, code) diff --git a/sherlock/internal/models/cluster.go b/sherlock/internal/models/cluster.go index c36edaddf..2dfc7dcb3 100644 --- a/sherlock/internal/models/cluster.go +++ b/sherlock/internal/models/cluster.go @@ -1,8 +1,6 @@ package models import ( - "fmt" - "github.com/broadinstitute/sherlock/sherlock/internal/errors" "gorm.io/gorm" ) @@ -39,11 +37,6 @@ func (c *Cluster) errorIfForbidden(tx *gorm.DB) error { if err = user.ErrIfNotActiveInRole(tx, c.RequiredRoleID); err != nil { return err } - if c.RequiresSuitability != nil && *c.RequiresSuitability { - if err = user.ErrIfNotSuitable(); err != nil { - return fmt.Errorf("(%s) suitability required: %w", errors.Forbidden, err) - } - } return nil } diff --git a/sherlock/internal/models/cluster_test.go b/sherlock/internal/models/cluster_test.go index cdeb7fca8..c24d2ab08 100644 --- a/sherlock/internal/models/cluster_test.go +++ b/sherlock/internal/models/cluster_test.go @@ -203,17 +203,17 @@ func (s *modelSuite) TestCluster_errorIfForbidden_nullRequiresSuitability() { } func (s *modelSuite) TestClusterCreationForbidden() { - s.SetNonSuitableTestUserForDB() cluster := Cluster{ - Name: "some-name", - Provider: "google", - GoogleProject: "some-project", - Location: "some-location", - Base: utils.PointerTo("some base"), - Address: utils.PointerTo("0.0.0.0"), - RequiresSuitability: utils.PointerTo(true), - HelmfileRef: utils.PointerTo("some-ref"), + Name: "some-name", + Provider: "google", + GoogleProject: "some-project", + Location: "some-location", + Base: utils.PointerTo("some base"), + Address: utils.PointerTo("0.0.0.0"), + RequiredRoleID: utils.PointerTo(s.TestData.Role_TerraSuitableEngineer().ID), + HelmfileRef: utils.PointerTo("some-ref"), } + s.SetNonSuitableTestUserForDB() s.ErrorContains(s.DB.Create(&cluster).Error, errors.Forbidden) } @@ -222,7 +222,7 @@ func (s *modelSuite) TestClusterEditEscalateForbidden() { s.SetNonSuitableTestUserForDB() s.ErrorContains(s.DB. Model(&cluster). - Updates(&Cluster{RequiresSuitability: utils.PointerTo(true)}). + Updates(&Cluster{RequiredRoleID: utils.PointerTo(s.TestData.Role_TerraSuitableEngineer().ID)}). Error, errors.Forbidden) } @@ -231,7 +231,7 @@ func (s *modelSuite) TestClusterEditDeescalateForbidden() { s.SetNonSuitableTestUserForDB() s.ErrorContains(s.DB. Model(&cluster). - Updates(&Cluster{RequiresSuitability: utils.PointerTo(false)}). + Updates(&Cluster{RequiredRoleID: utils.PointerTo(s.TestData.Role_TerraSuitableEngineer().ID)}). Error, errors.Forbidden) } diff --git a/sherlock/internal/models/environment.go b/sherlock/internal/models/environment.go index d33409739..e436cd8d7 100644 --- a/sherlock/internal/models/environment.go +++ b/sherlock/internal/models/environment.go @@ -71,11 +71,6 @@ func (e *Environment) errorIfForbidden(tx *gorm.DB) error { if err = user.ErrIfNotActiveInRole(tx, e.RequiredRoleID); err != nil { return err } - if e.RequiresSuitability != nil && *e.RequiresSuitability { - if err = user.ErrIfNotSuitable(); err != nil { - return fmt.Errorf("(%s) suitability required: %w", errors.Forbidden, err) - } - } return nil } diff --git a/sherlock/internal/models/environment_test.go b/sherlock/internal/models/environment_test.go index fe84f9a1f..820d96ab0 100644 --- a/sherlock/internal/models/environment_test.go +++ b/sherlock/internal/models/environment_test.go @@ -193,7 +193,6 @@ func (s *modelSuite) TestEnvironment_errorIfForbidden_nullRequiresSuitability() } func (s *modelSuite) TestEnvironmentCreationForbidden() { - s.SetNonSuitableTestUserForDB() environment := Environment{ Base: "live", Lifecycle: "static", @@ -201,7 +200,7 @@ func (s *modelSuite) TestEnvironmentCreationForbidden() { ValuesName: "prod", AutoPopulateChartReleases: utils.PointerTo(false), DefaultNamespace: "terra-prod", - RequiresSuitability: utils.PointerTo(true), + RequiredRoleID: utils.PointerTo(s.TestData.Role_TerraSuitableEngineer().ID), BaseDomain: utils.PointerTo("dsde-prod.broadinstitute.org"), NamePrefixesDomain: utils.PointerTo(false), HelmfileRef: utils.PointerTo("HEAD"), @@ -209,6 +208,7 @@ func (s *modelSuite) TestEnvironmentCreationForbidden() { Description: utils.PointerTo("Terra's production environment"), Offline: utils.PointerTo(false), } + s.SetNonSuitableTestUserForDB() s.ErrorContains(s.DB.Create(&environment).Error, errors.Forbidden) } @@ -217,7 +217,7 @@ func (s *modelSuite) TestEnvironmentEditEscalateForbidden() { s.SetNonSuitableTestUserForDB() s.ErrorContains(s.DB. Model(&environment). - Updates(&Environment{RequiresSuitability: utils.PointerTo(true)}). + Updates(&Environment{RequiredRoleID: utils.PointerTo(s.TestData.Role_TerraSuitableEngineer().ID)}). Error, errors.Forbidden) } diff --git a/sherlock/internal/models/user.go b/sherlock/internal/models/user.go index 1e1c240c1..6efe8f7ea 100644 --- a/sherlock/internal/models/user.go +++ b/sherlock/internal/models/user.go @@ -206,29 +206,6 @@ func (u *User) ErrIfNotActiveInRole(db *gorm.DB, roleID *uint) error { } } -// ErrIfNotSuitable may be removed in the future: we'll want to be doing Sherlock's internal RBAC based on -// Role and RoleAssignment instead of Suitability, and the only usage of Suitability *should* be for -// suspending RoleAssignment entries. An error isn't really helpful for us there, so we may have no need -// for this method. For the moment, though, it offers a very similar logical interface to the now-removed -// `User.Suitable().SuitableOrError()` method, so it makes sense for now. -func (u *User) ErrIfNotSuitable() error { - if u.Email == self.Email && u.GoogleID == self.GoogleID && u.AuthenticationMethod == authentication_method.SHERLOCK_INTERNAL { - // Short-circuit to respect Sherlock's own user; see SelfUser. - // We only respect this with an internal authentication method as defense-in-depth (it should be impossible to - // actually make a request as Sherlock, but we don't want to find out). - return nil - } - if u.Suitability != nil && u.Suitability.Suitable != nil && u.Suitability.Description != nil { - if *u.Suitability.Suitable { - return nil - } else { - return fmt.Errorf("(%s) user is unsuitable: %s", errors.Forbidden, *u.Suitability.Description) - } - } else { - return fmt.Errorf("(%s) no matching suitability record found or loaded; assuming unsuitable", errors.Forbidden) - } -} - func (u *User) ErrIfNotSuperAdmin() error { if u.Email == self.Email && u.GoogleID == self.GoogleID && u.AuthenticationMethod == authentication_method.SHERLOCK_INTERNAL { // Short-circuit to respect Sherlock's own user; see SelfUser. diff --git a/sherlock/internal/oidc_models/storage.go b/sherlock/internal/oidc_models/storage.go index aeabb3fe4..a9e7985cb 100644 --- a/sherlock/internal/oidc_models/storage.go +++ b/sherlock/internal/oidc_models/storage.go @@ -5,6 +5,7 @@ import ( "context" "crypto/rand" "crypto/sha512" + "encoding/base64" "errors" "fmt" "github.com/broadinstitute/sherlock/go-shared/pkg/utils" @@ -467,12 +468,14 @@ func (s *storageImpl) createRefreshToken(clientID string, scopes []string, userI if err != nil { return nil, "", fmt.Errorf("failed to create UUID: %w", err) } - refreshToken := make([]byte, 256) - _, err = rand.Read(refreshToken) + refreshTokenBytes := make([]byte, 256) + _, err = rand.Read(refreshTokenBytes) if err != nil { return nil, "", fmt.Errorf("failed to generate refresh token: %w", err) } - hash := sha512.Sum512(refreshToken) + // b64 encode to make these safer to store in JSON/YAML files, which ArgoCD's CLI likes to do + refreshToken := base64.URLEncoding.EncodeToString(refreshTokenBytes) + hash := sha512.Sum512([]byte(refreshToken)) refreshTokenModel := &RefreshToken{ ID: id, TokenHash: hash[:], @@ -485,7 +488,7 @@ func (s *storageImpl) createRefreshToken(clientID string, scopes []string, userI if err != nil { return nil, "", fmt.Errorf("failed to create refresh token: %w", err) } - return refreshTokenModel, string(refreshToken), nil + return refreshTokenModel, refreshToken, nil } func (s *storageImpl) revokeRefreshToken(refreshTokenID uuid.UUID) error { diff --git a/sherlock/internal/oidc_models/storage_test.go b/sherlock/internal/oidc_models/storage_test.go index 32e2576bb..763464d22 100644 --- a/sherlock/internal/oidc_models/storage_test.go +++ b/sherlock/internal/oidc_models/storage_test.go @@ -3,6 +3,7 @@ package oidc_models import ( "context" "database/sql" + "encoding/base64" "github.com/broadinstitute/sherlock/go-shared/pkg/utils" "github.com/broadinstitute/sherlock/sherlock/internal/models" "github.com/go-jose/go-jose/v4" @@ -464,16 +465,20 @@ func (s *oidcModelsSuite) TestStorageImpl_Health() { func (s *oidcModelsSuite) TestStorageImpl_createRefreshToken() { clientID, _, err := s.GenerateClient(s.DB) s.NoError(err) - refreshToken, _, err := s.storage.createRefreshToken(clientID, []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail, groupsClaim}, s.TestData.User_Suitable().ID, time.Now()) + refreshTokenModel, refreshTokenItself, err := s.storage.createRefreshToken(clientID, []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail, groupsClaim}, s.TestData.User_Suitable().ID, time.Now()) s.NoError(err) - s.NotEmpty(refreshToken.ID) - s.Equal(clientID, refreshToken.ClientID) - s.Equal(oidc.SpaceDelimitedArray{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail, groupsClaim}, refreshToken.Scopes) - s.Equal(s.TestData.User_Suitable().ID, refreshToken.UserID) + s.NotEmpty(refreshTokenModel.ID) + s.Equal(clientID, refreshTokenModel.ClientID) + s.Equal(oidc.SpaceDelimitedArray{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail, groupsClaim}, refreshTokenModel.Scopes) + s.Equal(s.TestData.User_Suitable().ID, refreshTokenModel.UserID) var refreshTokens []RefreshToken s.NoError(s.DB.Find(&refreshTokens).Error) s.Len(refreshTokens, 1) + + // Check that the refresh token itself is base64 encoded by checking that we can decode it + _, err = base64.URLEncoding.DecodeString(refreshTokenItself) + s.NoError(err) } func (s *oidcModelsSuite) TestStorageImpl_revokeRefreshToken() {