Move CSRF implementation into session

This drops the need for two separate cookies to maintain CSRF and removes a few layers of indirection for CSRF handling. CSRF values are now also tied to session duration. This intentionally leaves the configuration and secret in place until the _next_ release to provide a seamless transition.
google · Mar 30, 2021 · a61c434 · a61c434
1 parent db5d1cc
commit a61c434
Show file tree

Hide file tree

Showing 19 changed files with 232 additions and 95 deletions.
diff --git a/assets/server/codes/issue.html b/assets/server/codes/issue.html
@@ -391,7 +391,7 @@ <h1>{{t $.locale "codes.issue.header"}}</h1>
         contentType: 'application/json',
         data: JSON.stringify(data),
         headers: {
-          'X-CSRF-Token': '{{.csrfToken}}',
+          'X-CSRF-Token': getCSRFToken(),
         },
         success: function(result) {
           if(result.error && result.error != "") {

diff --git a/assets/server/login/_loginscripts.html b/assets/server/login/_loginscripts.html
@@ -15,7 +15,7 @@
         data: {
           idToken: idToken,
         },
-        headers: { 'X-CSRF-Token': '{{.csrfToken}}' },
+        headers: { 'X-CSRF-Token': getCSRFToken() },
         contentType: 'application/x-www-form-urlencoded',
         {{if not .currentUser}}
         success: function(returnData) {

diff --git a/assets/server/login/register-phone.html b/assets/server/login/register-phone.html
@@ -319,7 +319,7 @@
               idToken: idToken,
               factorCount: factorCount,
             },
-            headers: { 'X-CSRF-Token': '{{.csrfToken}}' },
+            headers: { 'X-CSRF-Token': getCSRFToken() },
             contentType: 'application/x-www-form-urlencoded',
             success: function(returnData) {
               flash.clear();

diff --git a/assets/server/static/js/application.js b/assets/server/static/js/application.js
@@ -34,7 +34,7 @@ $(function() {
       }
     }
 
-    let csrfToken = $("meta[name=csrf-token]").attr("content");
+    let csrfToken = getCSRFToken();
     let $csrfField = $("<input>")
       .attr("type", "hidden")
       .attr("name", "gorilla.csrf.Token")
@@ -238,6 +238,10 @@ $(function() {
   window.flash = flash;
 });
 
+function getCSRFToken() {
+  return document.querySelector('meta[name="csrf-token"]').content;
+}
+
 function setCookie(cname, cvalue, exdays) {
   let d = new Date();
   d.setTime(d.getTime() + (exdays * 24 * 60 * 60 * 1000));

diff --git a/assets/server/users/import.html b/assets/server/users/import.html
@@ -207,7 +207,7 @@ <h1>Import users</h1>
           "users": data,
           "sendInvites": sendInvites,
        }),
-        headers: { 'X-CSRF-Token': '{{.csrfToken}}' },
+        headers: { 'X-CSRF-Token': getCSRFToken() },
         contentType: 'application/json',
         success: function(result) {
           totalUsersAdded += result.newUsers.length;

diff --git a/docs/development.md b/docs/development.md
@@ -75,12 +75,6 @@ represent best practices.
     # Disable local observability.
     export OBSERVABILITY_EXPORTER="NOOP"
 
-    # Configure CSRF for preventing request forgery. Create your own with:
-    #
-    #     openssl rand -base64 32
-    #
-    export CSRF_AUTH_KEY="RcCNhTkS9tSDMSGcl4UCa1FUg9GmctkJpdI+eqZ+3v4="
-
     # Configure cookie encryption, the first is 64 bytes, the second is 32.
     # Create your own values with:
     #

diff --git a/docs/production.md b/docs/production.md
@@ -184,22 +184,6 @@ provide its _reference_ in the environment. If you are not using a secret
 manager, provide this value directly in the environment.
 
 
-### Cross-site request forgery (CSRF) keys
-
-**Recommended frequency:** 90 days, on breach
-
-To rotate the key, generate a new 32-byte key. You can use `openssl` or similar:
-
-```sh
-openssl rand -base64 32 | tr -d "\n"
-```
-
-Update the `CSRF_AUTH_KEY` environment variable and re-deploy. The system [only
-supports a single key for CSRF](https://github.com/gorilla/csrf/issues/65). When
-you deploy the new key, any existing open HTML forms will fail to submit as an
-invalid request.
-
-
 ### Database encryption keys
 
 **Recommend frequency:** 30 days, on breach

diff --git a/go.mod b/go.mod
@@ -20,7 +20,6 @@ require (
 	github.com/google/exposure-notifications-server v0.24.1-0.20210322233555-ecf9dee504c1
 	github.com/google/go-cmp v0.5.5
 	github.com/google/uuid v1.2.0
-	github.com/gorilla/csrf v1.7.0
 	github.com/gorilla/handlers v1.5.1
 	github.com/gorilla/mux v1.8.0
 	github.com/gorilla/schema v1.2.0

diff --git a/go.sum b/go.sum
@@ -430,8 +430,6 @@ github.com/googleapis/gax-go/v2 v2.0.5 h1:sjZBwGj9Jlw33ImPtvFviGYvseOtDM7hkSKB7+
 github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5mhpdKc/us6bOk=
 github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=
 github.com/gorilla/context v1.1.1/go.mod h1:kBGZzfjB9CEq2AlWe17Uuf7NDRt0dE0s8S51q0aT7Yg=
-github.com/gorilla/csrf v1.7.0 h1:mMPjV5/3Zd460xCavIkppUdvnl5fPXMpv2uz2Zyg7/Y=
-github.com/gorilla/csrf v1.7.0/go.mod h1:+a/4tCmqhG6/w4oafeAZ9pEa3/NZOWYVbD9fV0FwIQA=
 github.com/gorilla/css v1.0.0 h1:BQqNyPTi50JCFMTw/b67hByjMVXZRwGha6wxVGkeihY=
 github.com/gorilla/css v1.0.0/go.mod h1:Dn721qIggHpt4+EFCcTLTU/vk5ySda2ReITrtgBl60c=
 github.com/gorilla/handlers v1.4.2/go.mod h1:Qkdc/uu4tH4g6mTK6auzZ766c4CA0Ng8+o/OAirnOIQ=

diff --git a/internal/envstest/server.go b/internal/envstest/server.go
@@ -232,8 +232,7 @@ func NewServerConfig(tb testing.TB, testDatabaseInstance *database.TestInstance)
 			EnableAuthenticatedSMS: true,
 		},
 
-		CookieKeys:  config.Base64ByteSlice{randomBytes(tb, 64), randomBytes(tb, 32)},
-		CSRFAuthKey: randomBytes(tb, 32),
+		CookieKeys: config.Base64ByteSlice{randomBytes(tb, 64), randomBytes(tb, 32)},
 
 		CertificateSigning: config.CertificateSigningConfig{
 			Keys:                  *harness.KeyManagerConfig,

diff --git a/internal/routes/server.go b/internal/routes/server.go
@@ -125,10 +125,6 @@ func Server(
 	processDebug := middleware.ProcessDebug()
 	r.Use(processDebug)
 
-	// Install the CSRF protection middleware.
-	configureCSRF := middleware.ConfigureCSRF(cfg, h)
-	r.Use(configureCSRF)
-
 	// Sessions
 	requireSession := middleware.RequireSession(sessions, h)
 	r.Use(requireSession)
@@ -137,6 +133,10 @@ func Server(
 	currentPath := middleware.InjectCurrentPath()
 	r.Use(currentPath)
 
+	// Install the CSRF protection middleware.
+	handleCSRF := middleware.HandleCSRF(h)
+	r.Use(handleCSRF)
+
 	// Create common middleware
 	requireAuth := middleware.RequireAuth(cacher, authProvider, db, h, cfg.SessionIdleTimeout, cfg.SessionDuration)
 	checkIdleNoAuth := middleware.CheckSessionIdleNoAuth(h, cfg.SessionIdleTimeout)

diff --git a/pkg/api/api.go b/pkg/api/api.go
@@ -172,15 +172,6 @@ func (p *Padding) UnmarshalJSON(b []byte) error {
 	return nil
 }
 
-// CSRFResponse is the return type when requesting an AJAX CSRF token.
-type CSRFResponse struct {
-	Padding Padding `json:"padding"`
-
-	CSRFToken string `json:"csrftoken"`
-	Error     string `json:"error"`
-	ErrorCode string `json:"errorCode"`
-}
-
 // UserBatchRequest is a request for bulk creation of users.
 // This is called by the Web frontend.
 // API is served at /users/import/userbatch

diff --git a/pkg/config/server_config.go b/pkg/config/server_config.go
@@ -17,6 +17,7 @@ package config
 import (
 	"context"
 	"fmt"
+	"os"
 	"strings"
 	"time"
 
@@ -27,6 +28,7 @@ import (
 	"github.com/microcosm-cc/bluemonday"
 	"github.com/russross/blackfriday/v2"
 
+	"github.com/google/exposure-notifications-server/pkg/logging"
 	"github.com/google/exposure-notifications-server/pkg/observability"
 
 	firebase "firebase.google.com/go"
@@ -85,10 +87,6 @@ type ServerConfig struct {
 	// CookieDomain is the domain for which cookie should be valid.
 	CookieDomain string `env:"COOKIE_DOMAIN"`
 
-	// CSRFAuthKey is the authentication key. It must be 32-bytes and can be
-	// generated with tools/gen-secret. The value's should be base64 encoded.
-	CSRFAuthKey envconfig.Base64Bytes `env:"CSRF_AUTH_KEY,required"`
-
 	// Application Config
 	ServerName string `env:"SERVER_NAME,default=Diagnosis Verification Server"`
 
@@ -113,6 +111,12 @@ func NewServerConfig(ctx context.Context) (*ServerConfig, error) {
 		return nil, err
 	}
 
+	// Deprecations
+	logger := logging.FromContext(ctx).Named("config.ServerConfig")
+	if v := os.Getenv("CSRF_AUTH_KEY"); v != "" {
+		logger.Warnw("CSRF_AUTH_KEY is no longer used, please remove it from your configuration")
+	}
+
 	// Parse system notice - do this once since it's displayed on every page.
 	if v := project.TrimSpace(config.SystemNotice); v != "" {
 		raw := blackfriday.Run([]byte(v))