Issue 341 modernise secret handling #392

eoinkelly · 2022-10-22T21:32:42Z

Closes #341

Load non-sensitive app config from config/app.yml
- Put explanatory comment at top of config/app.yml to explain that
  non-sensitive config should go here and sensitive config should go in
  config/secrets.yml
- Update existing references to mail_from config item in the app
Remove Rails encrypted credentials files to avoid confusion
- We have chosen not to use Rails encrypted credentials so we remove the
  files generated by Rails to avoid confusion.
Stop creating .env from example.env because bin/setup already does this
Modernise generated secrets.yml
- Stop generating both secrets.yml and secrets.example.yml. Our secrets.yml
  loads everything from the ENV so it is safe to check in and having a
  separate example file is unnecessary.
- Add secrets demonstrating how to configure ActiveRecord encrypted attributes
  without using Rails encrypted credentials.
Remove confusing comment about .envrc at top of .env file.

* Stop generating both `secrets.yml` and `secrets.example.yml`. Our `secrets.yml` loads everything from the ENV so it is safe to check in and having a separate example file is unnecessary. * Add secrets demonstrating how to configure ActiveRecord encrypted attributes without using Rails encrypted credentials.

We have chosen not to use Rails encrypted credentials so we remove the files generated by Rails to avoid confusion.

* Put explanatory comment at top of `config/app.yml` to explain that non-sensitive config should go here and sensitive config should go in `config/secrets.yml` * Update existing references to `mail_from` config item in the app

eoinkelly · 2022-10-22T21:36:00Z

variants/backend-base/app/template.rb

@@ -19,4 +19,4 @@
 # Configure the default mailer to use the our default from address
 gsub_file "app/mailers/application_mailer.rb",
          "default from: 'from@example.com'",
-          "default from: Rails.application.secrets.mail_from"
+          "default from: Rails.application.config.app.mail_from"


We cannot load things directly in to Rails.application.config using a YAML file - we have to pick a namespace under config. I chose config.app but totally open to changing this.

Alternatively we could skip the YAML file completely and just have a ruby initializer which loads things directly into Rails.application.config. I slightly favour the YAML file because it makes it easier when secrets vary between environments.

eoinkelly · 2022-10-22T21:37:04Z

variants/backend-base/config/application.rb

+    config.active_record.encryption.primary_key = Rails.application.secrets.active_record_encryption_primary_key
+    config.active_record.encryption.deterministic_key = Rails.application.secrets.active_record_encryption_deterministic_key
+    config.active_record.encryption.key_derivation_salt = Rails.application.secrets.active_record_encryption_key_derivation_salt


@G-Rath It turns out we can use encrypted attributes without encrypted credentials. It is very poorly documented so I think it's worth baking into the template.

eoinkelly · 2022-10-22T21:38:58Z

variants/backend-base/config/secrets.yml

+  secret_key_base: "<%= ENV['RAILS_SECRET_KEY_BASE'] %>"
+  active_record_encryption_primary_key: "<%= ENV['ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY'] %>"
+  active_record_encryption_deterministic_key: "<%= ENV['ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY'] %>"
+  active_record_encryption_key_derivation_salt: "<%= ENV['ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT'] %>"


If you nest keys here then you have to use hash key syntax to get the value e.g.

aaa: bb: "foo" # Rails.application.secrets.aaa.bb # breaks # Rails.application.secrets.aaa[:bb] # works

This felt a bit ugly so I flattened the secrets instead

variants/backend-base/config/secrets.yml

variants/backend-base/example.env.tt

variants/devise/template.rb

eoinkelly · 2022-10-22T21:40:06Z

variants/backend-base/config/template.rb

+remove_file "config/master.key"
+remove_file "config/credentials.yml.enc"


We aren't using these but by default Rails creates them and puts a secret_key_base value in there which will be preferred by Rails if it exists and can be decrypted.

Co-authored-by: Gareth Jones <gareth.jones@ackama.com>

…r values

eoinkelly · 2022-10-23T22:02:41Z

variants/backend-base/example.env.tt

+# to create new versions of these secrets for each deployed environment (e.g.
+# staging, production)
+ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY="PLACEHOLDER_PRIMARY_KEY"
+ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY="PLACEHOLDER_DETERMINISTIC_KEY"
+ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT="PLACEHOLDER_KEY_DERIVATION_SALT"


These env vars need to exist to load Rails because we are loading them with ENV.fetch. We need to run Rails to set realistic values so we need to set these env vars to an initial placeholder value and later set better values.

robotdana · 2023-03-23T23:46:53Z

variants/backend-base/set_active_record_encryption_secrets.rb

+
+gsub_file("example.env", "PLACEHOLDER_PRIMARY_KEY", db_secrets.fetch("primary_key"))
+gsub_file("example.env", "PLACEHOLDER_DETERMINISTIC_KEY", db_secrets.fetch("deterministic_key"))
+gsub_file("example.env", "PLACEHOLDER_KEY_DERIVATION_SALT", db_secrets.fetch("key_derivation_salt"))


if we're using generated ones here, i wonder if we should add these keys to the initializer that checks for using the example.env secret key base. #313

Good idea, will do.

… .env

eoinkelly · 2023-05-06T00:52:27Z

variants/backend-base/config/initializers/check_env.rb

+class VerifyPlaceholderSecretsNotUsedForReal
+  class << self
+    PLACEHOLDER_PREFIX_REGEX = /(PLACEHOLDER|FAILED_TO_GENERATE)/.freeze

-# RAILS_SECRET_KEY_BASE should be set to something other than its value in example.env
+    def run
+      return if local?

-if Rails.env.production? && Rails.root.join("example.env").read.include?(ENV.fetch("RAILS_SECRET_KEY_BASE"))
-  fail "RAILS_SECRET_KEY_BASE is unchanged from example.env. " \
-       "Generate a new one with `bundle exec rails secret`"
+      verify_secret_key_base
+      verify_activerecord_encryption_secrets
+    end
+
+    private
+
+    def verify_secret_key_base
+      return unless Rails.root.join("example.env").read.include?(ENV.fetch("RAILS_SECRET_KEY_BASE"))
+
+      fail "RAILS_SECRET_KEY_BASE is unchanged from example.env. Generate a new one with `bundle exec rails secret`"
+    end
+
+    ##
+    # Verify that placeholder values created by the Ackama rails template are
+    # not being used for real.
+    #
+    def verify_activerecord_encryption_secrets # rubocop:disable Metrics/AbcSize
+      secrets = [
+        Rails.application.config.active_record.encryption.primary_key,
+        Rails.application.config.active_record.encryption.deterministic_key,
+        Rails.application.config.active_record.encryption.key_derivation_salt
+      ]
+
+      secrets.each do |secret|
+        fail "Insecure ENV: ActiveRecored encrypted credentials env contain in insecure placeholder value." if secret.match?(PLACEHOLDER_PREFIX_REGEX)
+      end
+    end
+
+    def local?
+      Rails.env.development? || Rails.env.test?
+    end
+  end
 end
+
+VerifyPlaceholderSecretsNotUsedForReal.run


@robotdana You may want to review this because I've changed it quite a bit

variants/backend-base/config/app.yml

variants/backend-base/config/initializers/check_env.rb

G-Rath · 2023-05-06T19:21:35Z

variants/backend-base/config/secrets.yml

+# Do NOT put secrets directly into this file. All secrets should be loaded from ENV!
+# Be sure to restart your server when you modify this file.


🐘 I think it would be worth mentioning config/app.yml here

variants/backend-base/example.env.tt

G-Rath · 2023-05-06T19:32:08Z

variants/backend-base/config/app.yml

+staging:
+  <<: *default


🐘 ideally we should actually only add this if a capistrano variant is enabled, because Heroku doesn't do staging so it isn't reasonable to assume all apps will have it

I think we should keep as is because:

Heroku is the odd one out in not supporting a staging env out of the box - every other way of deploying Rails uses one AFAIK (not just capistrano based deploys).

Having this here doesn't harm a Heroku deploy.

We don't currently have a template config variable for "deploying to heroku". I don't think this use-case is enough to add that complexity.

I'm not sure that can be the case because Rails/UnknownEnv does not include staging by default, but its a while since I looked at this so can't remember.

I'm fine with whatever you think is worth doing

G-Rath · 2023-05-06T19:32:21Z

variants/backend-base/config/secrets.yml

+staging:
+  <<: *default


🐘 ideally we should actually only add this if a capistrano variant is enabled, because Heroku doesn't do staging so it isn't reasonable to assume all apps will have it

I think we should keep as is because:

Heroku is the odd one out in not supporting a staging env out of the box - every other way of deploying Rails uses one AFAIK (not just capistrano based deploys).

Having this here doesn't harm a Heroku deploy.

We don't currently have a template config variable for "deploying to heroku". I don't think this use-case is enough to add that complexity.

I'm not sure that can be the case because Rails/UnknownEnv does not include staging by default, but its a while since I looked at this so can't remember.

I'm fine with whatever you think is worth doing

Co-authored-by: Gareth Jones <gareth.jones@ackama.com>

…-template into issue-341-modernise-secrets

Co-authored-by: Gareth Jones <gareth.jones@ackama.com>

G-Rath · 2023-07-28T01:07:24Z

variants/backend-base/config/app.yml

+staging:
+  <<: *default


I'm not sure that can be the case because Rails/UnknownEnv does not include staging by default, but its a while since I looked at this so can't remember.

I'm fine with whatever you think is worth doing

G-Rath · 2023-07-28T01:07:52Z

variants/backend-base/config/secrets.yml

+staging:
+  <<: *default


I'm not sure that can be the case because Rails/UnknownEnv does not include staging by default, but its a while since I looked at this so can't remember.

I'm fine with whatever you think is worth doing

lukeify · 2023-08-25T00:18:18Z

Merging as discussed at our Ruby catchup.

eoinkelly added 5 commits October 23, 2022 10:21

Remove confusing comment

1ad3bc1

Stop creating .env from example.env because bin/setup already does this

cde294b

Remove Rails encrypted credentials files to avoid confusion

983d868

We have chosen not to use Rails encrypted credentials so we remove the files generated by Rails to avoid confusion.

Load non-sensitive app config from config/app.yml

d100d0c

* Put explanatory comment at top of `config/app.yml` to explain that non-sensitive config should go here and sensitive config should go in `config/secrets.yml` * Update existing references to `mail_from` config item in the app

eoinkelly marked this pull request as ready for review October 22, 2022 21:33

eoinkelly commented Oct 22, 2022

View reviewed changes

G-Rath reviewed Oct 22, 2022

View reviewed changes

variants/backend-base/config/secrets.yml Outdated Show resolved Hide resolved

variants/backend-base/example.env.tt Outdated Show resolved Hide resolved

variants/devise/template.rb Outdated Show resolved Hide resolved

eoinkelly commented Oct 22, 2022

View reviewed changes

eoinkelly requested review from G-Rath, joshmcarthur and a team October 22, 2022 21:40

eoinkelly and others added 11 commits October 23, 2022 10:42

Update variants/devise/template.rb

96f2c05

Co-authored-by: Gareth Jones <gareth.jones@ackama.com>

Generate sensible default secrets for AR encryption

3a6ef51

Fail early if an expected secret is not available in the ENV

8a5492d

Set new required env vars in CI

7a0a026

Fix CI

0019a73

CI fix attempt 2

64fb8f2

Add debug logging to db:encryption:init

4c8981a

Fix chicken & egg of needing ENV vars to exist before we can set thei…

59d8370

…r values

Undo previous attempted fix

1633c3c

Use safe_load as suggested by rubocop

08737b9

Tone down output

6a7dc47

eoinkelly commented Oct 23, 2022

View reviewed changes

Merge branch 'main' into issue-341-modernise-secrets

9461bb2

robotdana reviewed Mar 23, 2023

View reviewed changes

robotdana approved these changes Mar 23, 2023

View reviewed changes

Merge branch 'main' into issue-341-modernise-secrets

a93845c

eoinkelly added 2 commits May 6, 2023 12:50

Check for placeholder secrets at app boot

d161f74

Generate separate ActiveRecord encryption secrets for example.env and…

d8f3293

… .env

eoinkelly commented May 6, 2023

View reviewed changes

eoinkelly requested a review from robotdana May 6, 2023 00:52

G-Rath requested changes May 6, 2023

View reviewed changes

G-Rath reviewed May 6, 2023

View reviewed changes

eoinkelly and others added 10 commits July 24, 2023 07:41

Update variants/backend-base/config/app.yml

944a40e

Co-authored-by: Gareth Jones <gareth.jones@ackama.com>

Update variants/backend-base/config/initializers/check_env.rb

61a18ba

Co-authored-by: Gareth Jones <gareth.jones@ackama.com>

Update variants/backend-base/config/initializers/check_env.rb

3247099

Co-authored-by: Gareth Jones <gareth.jones@ackama.com>

Merge branch 'main' into issue-341-modernise-secrets

6a78028

Merge branch 'issue-341-modernise-secrets' of github.com:ackama/rails…

c5eacb6

…-template into issue-341-modernise-secrets

Improve comment based on PR feedback

8f91295

Update variants/backend-base/example.env.tt

24f1153

Co-authored-by: Gareth Jones <gareth.jones@ackama.com>

Add missing end removed by merge

3801920

Ensure new auto-generated example.env secrets not used in prod

31acec3

Remove unnecessary tweaking of db:encryption secrets

c6f4dc3

eoinkelly requested a review from G-Rath July 23, 2023 20:53

G-Rath approved these changes Jul 28, 2023

View reviewed changes

Merge branch 'main' into issue-341-modernise-secrets

7cc48c8

eoinkelly merged commit b06d8bf into main Aug 25, 2023
22 checks passed

eoinkelly deleted the issue-341-modernise-secrets branch August 25, 2023 02:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 341 modernise secret handling #392

Issue 341 modernise secret handling #392

eoinkelly commented Oct 22, 2022

eoinkelly Oct 22, 2022

eoinkelly Oct 22, 2022

eoinkelly Oct 22, 2022

eoinkelly Oct 22, 2022

eoinkelly Oct 23, 2022

robotdana Mar 23, 2023

eoinkelly May 6, 2023

eoinkelly May 6, 2023

robotdana May 8, 2023

G-Rath May 6, 2023

G-Rath May 6, 2023

eoinkelly Jul 23, 2023

G-Rath Jul 28, 2023

G-Rath May 6, 2023

eoinkelly Jul 23, 2023

G-Rath Jul 28, 2023

G-Rath Jul 28, 2023

G-Rath Jul 28, 2023

lukeify commented Aug 25, 2023

		remove_file "config/master.key"
		remove_file "config/credentials.yml.enc"

		# Do NOT put secrets directly into this file. All secrets should be loaded from ENV!
		# Be sure to restart your server when you modify this file.

Issue 341 modernise secret handling #392

Issue 341 modernise secret handling #392

Conversation

eoinkelly commented Oct 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukeify commented Aug 25, 2023