[DDO-3763] Do CORS from Sherlock

sherlock/config/default_config.yaml

@@ -5,13 +5,11 @@
 #   It will use mock authentication and authorization.
 mode: debug
 
-hooks:
-  # If Sherlock should act on CiRun state transitions. Hooks are still subject
-  # to the third-party-specific flags elsewhere in this file.
-  enable: true
-  # If true, hooks will be run asynchronously after the initial call into the
-  # hooks package.
-  asynchronous: true
+# Origins is the list of allowed origins for requests (used for CORS and CSRF protection).
+# Each entry should include the scheme, like "https://example.com".
+# When empty, all origins will be allowed, which is only suitable for local development where
+# cross-origin requests with credentials aren't required.
+origins: []
 
 log:
   # Always true when mode="release"
@@ -93,6 +91,14 @@ auth:
     #- email: [email protected]
     #  suitable: false
 
+hooks:
+  # If Sherlock should act on CiRun state transitions. Hooks are still subject
+  # to the third-party-specific flags elsewhere in this file.
+  enable: true
+  # If true, hooks will be run asynchronously after the initial call into the
+  # hooks package.
+  asynchronous: true
+
 metrics:
   v2:
     enable: true

sherlock/config/test_config.yaml

@@ -3,8 +3,8 @@
 
 mode: debug
 
-hooks:
-  asynchronous: false
+origins:
+  - http://localhost:8080
 
 log:
   level: warn
@@ -27,6 +27,9 @@ auth:
     - email: [email protected]
       suitable: false
 
+hooks:
+  asynchronous: false
+
 pagerduty:
   enable: false
 

sherlock/go.mod

@@ -11,6 +11,7 @@ require (
 	github.com/coreos/go-oidc v2.2.1+incompatible
 	github.com/creasty/defaults v1.7.0
 	github.com/dustinkirkland/golang-petname v0.0.0-20230626224747-e794b9370d49
+	github.com/gin-contrib/cors v1.7.2
 	github.com/gin-gonic/gin v1.10.0
 	github.com/golang-migrate/migrate/v4 v4.17.1
 	github.com/google/go-cmp v0.6.0

sherlock/go.sum

@@ -150,6 +150,8 @@ github.com/fsnotify/fsnotify v1.6.0/go.mod h1:sl3t1tCWJFWoRz9R8WJCbQihKKwmorjAbS
 github.com/gabriel-vasile/mimetype v1.4.3 h1:in2uUcidCuFcDKtdcBxlR0rJ1+fsokWf+uqxgUFjbI0=
 github.com/gabriel-vasile/mimetype v1.4.3/go.mod h1:d8uq/6HKRL6CGdk+aubisF/M5GcPfT7nKyLpA0lbSSk=
 github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04=
+github.com/gin-contrib/cors v1.7.2 h1:oLDHxdg8W/XDoN/8zamqk/Drgt4oVZDvaV0YmvVICQw=
+github.com/gin-contrib/cors v1.7.2/go.mod h1:SUJVARKgQ40dmrzgXEVxj2m7Ig1v1qIboQkPDTQ9t2E=
 github.com/gin-contrib/gzip v0.0.6 h1:NjcunTcGAj5CO1gn4N8jHOSIeRFHIbn51z6K+xaN4d4=
 github.com/gin-contrib/gzip v0.0.6/go.mod h1:QOJlmV2xmayAjkNS2Y8NQsMneuRShOU/kjovCXNuzzk=
 github.com/gin-contrib/sse v0.1.0 h1:Y/yl/+YNO8GZSjAhjMsSuLt29uWRFHdHYUb5lYOV9qE=

sherlock/internal/boot/router.go

@@ -12,6 +12,8 @@ import (
 	"github.com/broadinstitute/sherlock/sherlock/internal/errors"
 	"github.com/broadinstitute/sherlock/sherlock/internal/metrics"
 	"github.com/broadinstitute/sherlock/sherlock/internal/middleware/authentication"
+	"github.com/broadinstitute/sherlock/sherlock/internal/middleware/cors"
+	"github.com/broadinstitute/sherlock/sherlock/internal/middleware/csrf_protection"
 	"github.com/broadinstitute/sherlock/sherlock/internal/middleware/headers"
 	"github.com/broadinstitute/sherlock/sherlock/internal/middleware/logger"
 	"github.com/gin-gonic/gin"
@@ -52,6 +54,7 @@ func BuildRouter(ctx context.Context, db *gorm.DB) *gin.Engine {
 	router.Use(
 		gin.Recovery(),
 		logger.Logger(),
+		cors.Cors(),
 		headers.Headers())
 
 	// Replace Gin's standard fallback responses with our standard error format for friendlier client behavior
@@ -78,7 +81,9 @@ 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.Middleware(db)...)
+	apiRouter := router.Group("api")
+	apiRouter.Use(csrf_protection.CsrfProtection())
+	apiRouter.Use(authentication.Middleware(db)...)
 
 	// refactored sherlock API, under /api/{type}/v3
 	sherlock.ConfigureRoutes(apiRouter)

sherlock/internal/middleware/cors/middleware.go

@@ -0,0 +1,30 @@
+package cors
+
+import (
+	"github.com/broadinstitute/sherlock/sherlock/internal/config"
+	"github.com/gin-contrib/cors"
+	"github.com/gin-gonic/gin"
+)
+
+// Cors does the CORS thing. IAP requires its cookie to be sent cross-origin, so origins must be
+// specifically enumerated for cross-origin AJAX to work.
+//
+// If no origins are specified, this middleware will allow all origins since that's at least
+// useful for local development where credentials aren't in play (browsers require origins to be
+// enumerated to send credentials, so operation behind IAP should enumerate origins).
+func Cors() gin.HandlerFunc {
+	c := cors.DefaultConfig()
+	c.AllowHeaders = append(c.AllowHeaders,
+		// "X-Requested-With" being set to "XMLHttpRequest" helps IAP understand the
+		// AJAX nature of the request so it can return a 401 rather than a 302.
+		// https://cloud.google.com/iap/docs/sessions-howto#understanding_the_response
+		"X-Requested-With")
+	if origins := config.Config.Strings("origins"); len(origins) > 0 {
+		c.AllowCredentials = true
+		c.AllowOrigins = origins
+	} else {
+		c.AllowCredentials = false
+		c.AllowAllOrigins = true
+	}
+	return cors.New(c)
+}

sherlock/internal/middleware/cors/middleware_test.go

@@ -0,0 +1,56 @@
+package cors
+
+import (
+	"github.com/broadinstitute/sherlock/sherlock/internal/config"
+	"github.com/gin-gonic/gin"
+	"github.com/rs/zerolog"
+	"github.com/stretchr/testify/assert"
+	"net/http/httptest"
+	"testing"
+)
+
+func TestHeaders(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	config.LoadTestConfig()
+	zerolog.SetGlobalLevel(zerolog.Disabled)
+	router := gin.New()
+	router.Use(Cors())
+	router.DELETE("/", func(ctx *gin.Context) {
+		ctx.JSON(200, gin.H{})
+	})
+	t.Run("successful preflight", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("OPTIONS", "/", nil)
+		request.Header.Set("Access-Control-Request-Method", "DELETE")
+		request.Header.Set("Access-Control-Request-Headers", "X-Requested-With")
+		request.Header.Set("Origin", "http://localhost:8080")
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, 204, recorder.Code)
+		assert.Equal(t, "http://localhost:8080", recorder.Header().Get("Access-Control-Allow-Origin"))
+		assert.Equal(t, "true", recorder.Header().Get("Access-Control-Allow-Credentials"))
+		assert.Contains(t, recorder.Header().Get("Access-Control-Allow-Headers"), "X-Requested-With")
+		assert.Contains(t, recorder.Header().Get("Access-Control-Allow-Methods"), "DELETE")
+	})
+	t.Run("failed origin preflight", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("OPTIONS", "/", nil)
+		request.Header.Set("Access-Control-Request-Method", "DELETE")
+		request.Header.Set("Access-Control-Request-Headers", "X-Requested-With")
+		request.Header.Set("Origin", "https://example.com")
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, 404, recorder.Code)
+		assert.Empty(t, recorder.Header().Get("Access-Control-Allow-Origin"))
+		assert.Empty(t, recorder.Header().Get("Access-Control-Allow-Credentials"))
+		assert.Empty(t, recorder.Header().Get("Access-Control-Allow-Headers"))
+		assert.Empty(t, recorder.Header().Get("Access-Control-Allow-Methods"))
+	})
+	t.Run("successful request", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("DELETE", "/", nil)
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, 200, recorder.Code)
+	})
+}

sherlock/internal/middleware/csrf_protection/middleware.go

@@ -0,0 +1,57 @@
+package csrf_protection
+
+import (
+	"fmt"
+	"github.com/broadinstitute/sherlock/go-shared/pkg/utils"
+	"github.com/broadinstitute/sherlock/sherlock/internal/config"
+	"github.com/broadinstitute/sherlock/sherlock/internal/errors"
+	"github.com/gin-gonic/gin"
+	"github.com/rs/zerolog/log"
+	"strings"
+)
+
+// CsrfProtection is a layer of defense against Cross-Site-Request-Forgery attacks.
+//
+// It checks three things:
+//  1. The content type of the request is application/json or not passed at all. HTML forms are a common
+//     vector for CSRF attacks, and they won't ever be application/json without JavaScript being involved
+//     in sending the request.
+//  2. The Origin header is in the list of allowed origins or not passed at all. Assuming JavaScript is
+//     involved in sending the request, we're in the land of JavaScript's same-origin-policy, so we can
+//     filter based on this header. An attacker won't be able to convince a modern browser *not* to send
+//     this header cross-origin.
+//  3. We do the same check for the Referer header as we do for the Origin header too. It's less reliable
+//     than the Origin header but it has older support.
+//
+// This isn't perfect -- it's not bulletproof like a double-submit cookie can be -- but for an API
+// it's fairly strong.
+func CsrfProtection() gin.HandlerFunc {
+	origins := config.Config.Strings("origins")
+	return func(ctx *gin.Context) {
+		if ct := ctx.ContentType(); ct != "" && ct != "application/json" {
+			log.Warn().Str("content-type", ct).Msgf("unsupported content type %s observed, rejecting request for CSRF protection", ct)
+			errors.AbortRequest(ctx, fmt.Errorf("(%s) unsupported content type; see logs for more details", errors.BadRequest))
+			return
+		} else if len(origins) > 0 {
+			if origin := ctx.GetHeader("Origin"); origin != "" && !utils.Contains(origins, origin) {
+				log.Warn().Str("origin", origin).Msgf("origin %s not allowed, rejecting request for CSRF protection", origin)
+				errors.AbortRequest(ctx, fmt.Errorf("(%s) origin not allowed; see logs for more details", errors.Forbidden))
+				return
+			}
+			if referer := ctx.GetHeader("Referer"); referer != "" && !refererHasOriginAsPrefix(referer, origins) {
+				log.Warn().Str("referer", referer).Msgf("referer %s not allowed, rejecting request for CSRF protection", referer)
+				errors.AbortRequest(ctx, fmt.Errorf("(%s) referer not allowed; see logs for more details", errors.Forbidden))
+				return
+			}
+		}
+	}
+}
+
+func refererHasOriginAsPrefix(referer string, origins []string) bool {
+	for _, origin := range origins {
+		if strings.HasPrefix(referer, origin) {
+			return true
+		}
+	}
+	return false
+}

sherlock/internal/middleware/csrf_protection/middleware_test.go

@@ -0,0 +1,92 @@
+package csrf_protection
+
+import (
+	"github.com/broadinstitute/sherlock/sherlock/internal/config"
+	"github.com/gin-gonic/gin"
+	"github.com/rs/zerolog"
+	"github.com/stretchr/testify/assert"
+	"net/http/httptest"
+	"testing"
+)
+
+func TestHeaders(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	config.LoadTestConfig()
+	zerolog.SetGlobalLevel(zerolog.Disabled)
+	router := gin.New()
+	router.Use(CsrfProtection())
+	router.DELETE("/", func(ctx *gin.Context) {
+		ctx.JSON(200, gin.H{})
+	})
+	t.Run("no content type", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("DELETE", "/", nil)
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, 200, recorder.Code)
+	})
+	t.Run("accepted content type", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("DELETE", "/", nil)
+		request.Header.Set("Content-Type", "application/json")
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, 200, recorder.Code)
+	})
+	t.Run("rejected content type", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("DELETE", "/", nil)
+		request.Header.Set("Content-Type", "text/html")
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, 400, recorder.Code)
+	})
+	t.Run("accepted origin", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("DELETE", "/", nil)
+		request.Header.Set("Origin", "http://localhost:8080")
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, 200, recorder.Code)
+	})
+	t.Run("rejected origin", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("DELETE", "/", nil)
+		request.Header.Set("Origin", "https://example.com")
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, 403, recorder.Code)
+	})
+	t.Run("accepted referer", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("DELETE", "/", nil)
+		request.Header.Set("Referer", "http://localhost:8080")
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, 200, recorder.Code)
+	})
+	t.Run("accepted referer on another url", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("DELETE", "/", nil)
+		request.Header.Set("Referer", "http://localhost:8080/blah/blah")
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, 200, recorder.Code)
+	})
+	t.Run("rejected referer", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("DELETE", "/", nil)
+		request.Header.Set("Referer", "https://example.com")
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, 403, recorder.Code)
+	})
+	t.Run("rejected referer on another url", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("DELETE", "/", nil)
+		request.Header.Set("Referer", "https://example.com/blah/blah")
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, 403, recorder.Code)
+	})
+}

sherlock/internal/middleware/headers/middleware_test.go

@@ -0,0 +1,28 @@
+package headers
+
+import (
+	"github.com/broadinstitute/sherlock/sherlock/internal/config"
+	"github.com/gin-gonic/gin"
+	"github.com/rs/zerolog"
+	"github.com/stretchr/testify/assert"
+	"net/http/httptest"
+	"testing"
+)
+
+func TestHeaders(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	config.LoadTestConfig()
+	zerolog.SetGlobalLevel(zerolog.Disabled)
+	router := gin.New()
+	router.Use(Headers())
+	router.GET("/", func(ctx *gin.Context) {
+		ctx.JSON(200, gin.H{})
+	})
+	t.Run("headers", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		request := httptest.NewRequest("GET", "/", nil)
+		router.ServeHTTP(recorder, request)
+
+		assert.Equal(t, "no-store", recorder.Header().Get("Cache-Control"))
+	})
+}