From: Tom Clegg <tom@tomclegg.ca>
Date: Mon, 7 Dec 2020 19:53:05 +0000 (-0500)
Subject: Warn about missing/short secrets. Delete Rails session key.
X-Git-Tag: 2.2.0~174^2~1
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/83898701e9c75661a240cadcf31f80cbccbb698e

Warn about missing/short secrets. Delete Rails session key.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@tomclegg.ca>
---

diff --git a/doc/admin/upgrading.html.textile.liquid b/doc/admin/upgrading.html.textile.liquid
index 3f622112e9..86ac509626 100644
--- a/doc/admin/upgrading.html.textile.liquid
+++ b/doc/admin/upgrading.html.textile.liquid
@@ -39,6 +39,12 @@ h2(#main). development main (as of 2020-10-28)
 
 "Upgrading from 2.1.0":#v2_1_0
 
+h3. System token requirements
+
+System services now log a warning at startup if any of the system tokens (@ManagementToken@, @SystemRootToken@, and @Collections.BlobSigningKey@) are less than 32 characters, or contain characters other than a-z, A-Z, and 0-9. After upgrading, run @arvados-server config-check@ and update your configuration file if needed to resolve any warnings.
+
+The @API.RailsSessionSecretToken@ configuration key has been removed. Delete this entry from your configuration file after upgrading.
+
 h3. Centos7 Python 3 dependency upgraded to python3
 
 Now that Python 3 is part of the base repository in CentOS 7, the Python 3 dependency for Centos7 Arvados packages was changed from SCL rh-python36 to python3.
diff --git a/doc/install/install-api-server.html.textile.liquid b/doc/install/install-api-server.html.textile.liquid
index b8442eb060..2893111e35 100644
--- a/doc/install/install-api-server.html.textile.liquid
+++ b/doc/install/install-api-server.html.textile.liquid
@@ -48,8 +48,6 @@ h3. Tokens
 <notextile>
 <pre><code>    SystemRootToken: <span class="userinput">"$system_root_token"</span>
     ManagementToken: <span class="userinput">"$management_token"</span>
-    API:
-      RailsSessionSecretToken: <span class="userinput">"$rails_secret_token"</span>
     Collections:
       BlobSigningKey: <span class="userinput">"blob_signing_key"</span>
 </code></pre>
@@ -59,8 +57,6 @@ h3. Tokens
 
 @ManagementToken@ is used to authenticate access to system metrics.
 
-@API.RailsSessionSecretToken@ is required by the API server.
-
 @Collections.BlobSigningKey@ is used to control access to Keep blocks.
 
 You can generate a random token for each of these items at the command line like this:
diff --git a/lib/boot/supervisor.go b/lib/boot/supervisor.go
index 1e8e83ff3b..0e4472b8a0 100644
--- a/lib/boot/supervisor.go
+++ b/lib/boot/supervisor.go
@@ -611,9 +611,6 @@ func (super *Supervisor) autofillConfig(cfg *arvados.Config) error {
 	if cluster.ManagementToken == "" {
 		cluster.ManagementToken = randomHexString(64)
 	}
-	if cluster.API.RailsSessionSecretToken == "" {
-		cluster.API.RailsSessionSecretToken = randomHexString(64)
-	}
 	if cluster.Collections.BlobSigningKey == "" {
 		cluster.Collections.BlobSigningKey = randomHexString(64)
 	}
diff --git a/lib/config/cmd_test.go b/lib/config/cmd_test.go
index 74c3cc9694..bb8d7dca10 100644
--- a/lib/config/cmd_test.go
+++ b/lib/config/cmd_test.go
@@ -49,10 +49,12 @@ func (s *CommandSuite) TestCheck_NoWarnings(c *check.C) {
 	in := `
 Clusters:
  z1234:
-  ManagementToken: xyzzy
-  SystemRootToken: xyzzy
+  ManagementToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+  SystemRootToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
   API:
     MaxItemsPerResponse: 1234
+  Collections:
+    BlobSigningKey: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
   PostgreSQL:
     Connection:
       sslmode: require
diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml
index 005d2738da..5763b595d2 100644
--- a/lib/config/config.default.yml
+++ b/lib/config/config.default.yml
@@ -212,11 +212,6 @@ Clusters:
       # serving a single incoming multi-cluster (federated) request.
       MaxRequestAmplification: 4
 
-      # RailsSessionSecretToken is a string of alphanumeric characters
-      # used by Rails to sign session tokens. IMPORTANT: This is a
-      # site secret. It should be at least 50 characters.
-      RailsSessionSecretToken: ""
-
       # Maximum wall clock time to spend handling an incoming request.
       RequestTimeout: 5m
 
diff --git a/lib/config/deprecated_test.go b/lib/config/deprecated_test.go
index ca376ba0bb..0dd03583d7 100644
--- a/lib/config/deprecated_test.go
+++ b/lib/config/deprecated_test.go
@@ -150,7 +150,7 @@ func (s *LoadSuite) TestLegacyKeepWebConfig(c *check.C) {
 }
 `)
 	cluster, err := testLoadLegacyConfig(content, "-legacy-keepweb-config", c)
-	c.Check(err, check.IsNil)
+	c.Assert(err, check.IsNil)
 
 	c.Check(cluster.Services.Controller.ExternalURL, check.Equals, arvados.URL{Scheme: "https", Host: "example.com", Path: "/"})
 	c.Check(cluster.SystemRootToken, check.Equals, "abcdefg")
@@ -183,7 +183,7 @@ func (s *LoadSuite) TestLegacyKeepWebConfigDoesntDisableMissingItems(c *check.C)
 }
 `)
 	cluster, err := testLoadLegacyConfig(content, "-legacy-keepweb-config", c)
-	c.Check(err, check.IsNil)
+	c.Assert(err, check.IsNil)
 	// The resulting ManagementToken should be the one set up on the test server.
 	c.Check(cluster.ManagementToken, check.Equals, TestServerManagementToken)
 }
@@ -193,8 +193,8 @@ func (s *LoadSuite) TestLegacyKeepproxyConfig(c *check.C) {
 	content := []byte(fmtKeepproxyConfig("", true))
 	cluster, err := testLoadLegacyConfig(content, f, c)
 
-	c.Check(err, check.IsNil)
-	c.Check(cluster, check.NotNil)
+	c.Assert(err, check.IsNil)
+	c.Assert(cluster, check.NotNil)
 	c.Check(cluster.Services.Controller.ExternalURL, check.Equals, arvados.URL{Scheme: "https", Host: "example.com", Path: "/"})
 	c.Check(cluster.SystemRootToken, check.Equals, "abcdefg")
 	c.Check(cluster.ManagementToken, check.Equals, "xyzzy")
@@ -262,8 +262,8 @@ func (s *LoadSuite) TestLegacyArvGitHttpdConfig(c *check.C) {
 	f := "-legacy-git-httpd-config"
 	cluster, err := testLoadLegacyConfig(content, f, c)
 
-	c.Check(err, check.IsNil)
-	c.Check(cluster, check.NotNil)
+	c.Assert(err, check.IsNil)
+	c.Assert(cluster, check.NotNil)
 	c.Check(cluster.Services.Controller.ExternalURL, check.Equals, arvados.URL{Scheme: "https", Host: "example.com", Path: "/"})
 	c.Check(cluster.SystemRootToken, check.Equals, "abcdefg")
 	c.Check(cluster.ManagementToken, check.Equals, "xyzzy")
@@ -285,7 +285,7 @@ func (s *LoadSuite) TestLegacyArvGitHttpdConfigDoesntDisableMissingItems(c *chec
 }
 `)
 	cluster, err := testLoadLegacyConfig(content, "-legacy-git-httpd-config", c)
-	c.Check(err, check.IsNil)
+	c.Assert(err, check.IsNil)
 	// The resulting ManagementToken should be the one set up on the test server.
 	c.Check(cluster.ManagementToken, check.Equals, TestServerManagementToken)
 }
@@ -295,8 +295,8 @@ func (s *LoadSuite) TestLegacyKeepBalanceConfig(c *check.C) {
 	content := []byte(fmtKeepBalanceConfig(""))
 	cluster, err := testLoadLegacyConfig(content, f, c)
 
-	c.Check(err, check.IsNil)
-	c.Check(cluster, check.NotNil)
+	c.Assert(err, check.IsNil)
+	c.Assert(cluster, check.NotNil)
 	c.Check(cluster.ManagementToken, check.Equals, "xyzzy")
 	c.Check(cluster.Services.Keepbalance.InternalURLs[arvados.URL{Host: ":80"}], check.Equals, arvados.ServiceInstance{})
 	c.Check(cluster.Collections.BalanceCollectionBuffers, check.Equals, 1000)
diff --git a/lib/config/export.go b/lib/config/export.go
index 0735354b1b..e4917032ff 100644
--- a/lib/config/export.go
+++ b/lib/config/export.go
@@ -69,7 +69,6 @@ var whitelist = map[string]bool{
 	"API.MaxKeepBlobBuffers":                       false,
 	"API.MaxRequestAmplification":                  false,
 	"API.MaxRequestSize":                           true,
-	"API.RailsSessionSecretToken":                  false,
 	"API.RequestTimeout":                           true,
 	"API.SendTimeout":                              true,
 	"API.WebsocketClientEventQueue":                false,
diff --git a/lib/config/generated_config.go b/lib/config/generated_config.go
index 885bb4b8c5..aeae214ef3 100644
--- a/lib/config/generated_config.go
+++ b/lib/config/generated_config.go
@@ -218,11 +218,6 @@ Clusters:
       # serving a single incoming multi-cluster (federated) request.
       MaxRequestAmplification: 4
 
-      # RailsSessionSecretToken is a string of alphanumeric characters
-      # used by Rails to sign session tokens. IMPORTANT: This is a
-      # site secret. It should be at least 50 characters.
-      RailsSessionSecretToken: ""
-
       # Maximum wall clock time to spend handling an incoming request.
       RequestTimeout: 5m
 
diff --git a/lib/config/load.go b/lib/config/load.go
index be6181bbe9..7eb4039100 100644
--- a/lib/config/load.go
+++ b/lib/config/load.go
@@ -13,6 +13,7 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
+	"regexp"
 	"strings"
 
 	"git.arvados.org/arvados.git/sdk/go/arvados"
@@ -270,6 +271,9 @@ func (ldr *Loader) Load() (*arvados.Config, error) {
 	// Check for known mistakes
 	for id, cc := range cfg.Clusters {
 		for _, err = range []error{
+			ldr.checkToken(fmt.Sprintf("Clusters.%s.ManagementToken", id), cc.ManagementToken),
+			ldr.checkToken(fmt.Sprintf("Clusters.%s.SystemRootToken", id), cc.SystemRootToken),
+			ldr.checkToken(fmt.Sprintf("Clusters.%s.Collections.BlobSigningKey", id), cc.Collections.BlobSigningKey),
 			checkKeyConflict(fmt.Sprintf("Clusters.%s.PostgreSQL.Connection", id), cc.PostgreSQL.Connection),
 			ldr.checkEmptyKeepstores(cc),
 			ldr.checkUnlistedKeepstores(cc),
@@ -282,6 +286,20 @@ func (ldr *Loader) Load() (*arvados.Config, error) {
 	return &cfg, nil
 }
 
+var acceptableTokenRe = regexp.MustCompile(`^[a-zA-Z0-9]+$`)
+var acceptableTokenLength = 32
+
+func (ldr *Loader) checkToken(label, token string) error {
+	if token == "" {
+		ldr.Logger.Warnf("%s: secret token is not set (use %d+ random characters from a-z, A-Z, 0-9)", label, acceptableTokenLength)
+	} else if !acceptableTokenRe.MatchString(token) {
+		return fmt.Errorf("%s: unacceptable characters in token (only a-z, A-Z, 0-9 are acceptable)", label)
+	} else if len(token) < acceptableTokenLength {
+		ldr.Logger.Warnf("%s: token is too short (should be at least %d characters)", label, acceptableTokenLength)
+	}
+	return nil
+}
+
 func checkKeyConflict(label string, m map[string]string) error {
 	saw := map[string]bool{}
 	for k := range m {
diff --git a/lib/config/load_test.go b/lib/config/load_test.go
index 58ddf950ef..c9ed37b835 100644
--- a/lib/config/load_test.go
+++ b/lib/config/load_test.go
@@ -192,6 +192,10 @@ func (s *LoadSuite) TestDeprecatedOrUnknownWarning(c *check.C) {
 	_, err := testLoader(c, `
 Clusters:
   zzzzz:
+    ManagementToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+    SystemRootToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+    Collections:
+     BlobSigningKey: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
     postgresql: {}
     BadKey: {}
     Containers: {}
@@ -261,6 +265,10 @@ func (s *LoadSuite) TestNoUnrecognizedKeysInDefaultConfig(c *check.C) {
 	err = yaml.Unmarshal(buf, &loaded)
 	c.Assert(err, check.IsNil)
 
+	c.Check(logbuf.String(), check.Matches, `(?ms).*SystemRootToken: secret token is not set.*`)
+	c.Check(logbuf.String(), check.Matches, `(?ms).*ManagementToken: secret token is not set.*`)
+	c.Check(logbuf.String(), check.Matches, `(?ms).*Collections.BlobSigningKey: secret token is not set.*`)
+	logbuf.Reset()
 	loader.logExtraKeys(loaded, supplied, "")
 	c.Check(logbuf.String(), check.Equals, "")
 }
@@ -269,7 +277,13 @@ func (s *LoadSuite) TestNoWarningsForDumpedConfig(c *check.C) {
 	var logbuf bytes.Buffer
 	logger := logrus.New()
 	logger.Out = &logbuf
-	cfg, err := testLoader(c, `{"Clusters":{"zzzzz":{}}}`, &logbuf).Load()
+	cfg, err := testLoader(c, `
+Clusters:
+ zzzzz:
+  ManagementToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+  SystemRootToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+  Collections:
+   BlobSigningKey: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa`, &logbuf).Load()
 	c.Assert(err, check.IsNil)
 	yaml, err := yaml.Marshal(cfg)
 	c.Assert(err, check.IsNil)
@@ -279,6 +293,31 @@ func (s *LoadSuite) TestNoWarningsForDumpedConfig(c *check.C) {
 	c.Check(logbuf.String(), check.Equals, "")
 }
 
+func (s *LoadSuite) TestUnacceptableTokens(c *check.C) {
+	for _, trial := range []struct {
+		short      bool
+		configPath string
+		example    string
+	}{
+		{false, "SystemRootToken", "SystemRootToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa_b_c"},
+		{false, "ManagementToken", "ManagementToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa b c"},
+		{false, "ManagementToken", "ManagementToken: \"$aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabc\""},
+		{false, "Collections.BlobSigningKey", "Collections: {BlobSigningKey: \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa⛵\"}"},
+		{true, "SystemRootToken", "SystemRootToken: a_b_c"},
+		{true, "ManagementToken", "ManagementToken: a b c"},
+		{true, "ManagementToken", "ManagementToken: \"$abc\""},
+		{true, "Collections.BlobSigningKey", "Collections: {BlobSigningKey: \"⛵\"}"},
+	} {
+		c.Logf("trying bogus config: %s", trial.example)
+		_, err := testLoader(c, "Clusters:\n zzzzz:\n  "+trial.example, nil).Load()
+		if trial.short {
+			c.Check(err, check.ErrorMatches, `Clusters.zzzzz.`+trial.configPath+`: unacceptable characters in token.*`)
+		} else {
+			c.Check(err, check.ErrorMatches, `Clusters.zzzzz.`+trial.configPath+`: unacceptable characters in token.*`)
+		}
+	}
+}
+
 func (s *LoadSuite) TestPostgreSQLKeyConflict(c *check.C) {
 	_, err := testLoader(c, `
 Clusters:
diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go
index a8d601d5f6..9dc9e17dd8 100644
--- a/sdk/go/arvados/config.go
+++ b/sdk/go/arvados/config.go
@@ -86,7 +86,6 @@ type Cluster struct {
 		MaxKeepBlobBuffers             int
 		MaxRequestAmplification        int
 		MaxRequestSize                 int
-		RailsSessionSecretToken        string
 		RequestTimeout                 Duration
 		SendTimeout                    Duration
 		WebsocketClientEventQueue      int
diff --git a/sdk/python/tests/run_test_server.py b/sdk/python/tests/run_test_server.py
index 0cb4151ac3..4562a06546 100644
--- a/sdk/python/tests/run_test_server.py
+++ b/sdk/python/tests/run_test_server.py
@@ -762,7 +762,6 @@ def setup_config():
                 "SystemRootToken": auth_token('system_user'),
                 "API": {
                     "RequestTimeout": "30s",
-                    "RailsSessionSecretToken": "e24205c490ac07e028fd5f8a692dcb398bcd654eff1aef5f9fe6891994b18483",
                 },
                 "Login": {
                     "SSO": {
diff --git a/services/api/config/arvados_config.rb b/services/api/config/arvados_config.rb
index 69b20420ab..5327713f69 100644
--- a/services/api/config/arvados_config.rb
+++ b/services/api/config/arvados_config.rb
@@ -93,7 +93,6 @@ arvcfg.declare_config "API.MaxRequestSize", Integer, :max_request_size
 arvcfg.declare_config "API.MaxIndexDatabaseRead", Integer, :max_index_database_read
 arvcfg.declare_config "API.MaxItemsPerResponse", Integer, :max_items_per_response
 arvcfg.declare_config "API.AsyncPermissionsUpdateInterval", ActiveSupport::Duration, :async_permissions_update_interval
-arvcfg.declare_config "API.RailsSessionSecretToken", NonemptyString, :secret_token
 arvcfg.declare_config "Users.AutoSetupNewUsers", Boolean, :auto_setup_new_users
 arvcfg.declare_config "Users.AutoSetupNewUsersWithVmUUID", String, :auto_setup_new_users_with_vm_uuid
 arvcfg.declare_config "Users.AutoSetupNewUsersWithRepository", Boolean, :auto_setup_new_users_with_repository
@@ -297,5 +296,9 @@ Server::Application.configure do
   # Rails.configuration.API["Blah"]
   ConfigLoader.copy_into_config $arvados_config, config
   ConfigLoader.copy_into_config $remaining_config, config
-  secrets.secret_key_base = $arvados_config["API"]["RailsSessionSecretToken"]
+
+  # We don't rely on cookies for authentication, so instead of
+  # requiring a signing key in config, we assign a new random one at
+  # startup.
+  secrets.secret_key_base = rand(1<<255).to_s(36)
 end
diff --git a/tools/arvbox/lib/arvbox/docker/cluster-config.sh b/tools/arvbox/lib/arvbox/docker/cluster-config.sh
index 708af17d5c..2a286637f6 100755
--- a/tools/arvbox/lib/arvbox/docker/cluster-config.sh
+++ b/tools/arvbox/lib/arvbox/docker/cluster-config.sh
@@ -125,8 +125,6 @@ Clusters:
         password: ${database_pw}
         dbname: arvados_${database_env}
         client_encoding: utf8
-    API:
-      RailsSessionSecretToken: $secret_token
     Collections:
       BlobSigningKey: $blob_signing_key
       DefaultReplication: 1