[DDO-3785] Ignore suitability within Sherlock

.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

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

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)

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

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)

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)

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)

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
 }
 

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)
 }
 

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
 }
 

sherlock/internal/models/environment_test.go

@@ -193,22 +193,22 @@ func (s *modelSuite) TestEnvironment_errorIfForbidden_nullRequiresSuitability()
 }
 
 func (s *modelSuite) TestEnvironmentCreationForbidden() {
-	s.SetNonSuitableTestUserForDB()
 	environment := Environment{
 		Base:                      "live",
 		Lifecycle:                 "static",
 		Name:                      "prod",
 		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"),
 		PreventDeletion:           utils.PointerTo(true),
 		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)
 }
 

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.

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 {