Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 341 modernise secret handling #392

Merged
merged 31 commits into from
Aug 25, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
1ad3bc1
Remove confusing comment
eoinkelly Oct 22, 2022
70c7dd4
Modernise generated secrets.yml
eoinkelly Oct 22, 2022
cde294b
Stop creating .env from example.env because bin/setup already does this
eoinkelly Oct 22, 2022
983d868
Remove Rails encrypted credentials files to avoid confusion
eoinkelly Oct 22, 2022
d100d0c
Load non-sensitive app config from config/app.yml
eoinkelly Oct 22, 2022
96f2c05
Update variants/devise/template.rb
eoinkelly Oct 22, 2022
3a6ef51
Generate sensible default secrets for AR encryption
eoinkelly Oct 22, 2022
8a5492d
Fail early if an expected secret is not available in the ENV
eoinkelly Oct 22, 2022
7a0a026
Set new required env vars in CI
eoinkelly Oct 23, 2022
0019a73
Fix CI
eoinkelly Oct 23, 2022
64fb8f2
CI fix attempt 2
eoinkelly Oct 23, 2022
4c8981a
Add debug logging to db:encryption:init
eoinkelly Oct 23, 2022
59d8370
Fix chicken & egg of needing ENV vars to exist before we can set thei…
eoinkelly Oct 23, 2022
1633c3c
Undo previous attempted fix
eoinkelly Oct 23, 2022
08737b9
Use safe_load as suggested by rubocop
eoinkelly Oct 23, 2022
6a7dc47
Tone down output
eoinkelly Oct 23, 2022
9461bb2
Merge branch 'main' into issue-341-modernise-secrets
eoinkelly Mar 23, 2023
a93845c
Merge branch 'main' into issue-341-modernise-secrets
eoinkelly May 5, 2023
d161f74
Check for placeholder secrets at app boot
eoinkelly May 6, 2023
d8f3293
Generate separate ActiveRecord encryption secrets for example.env and…
eoinkelly May 6, 2023
944a40e
Update variants/backend-base/config/app.yml
eoinkelly Jul 23, 2023
61a18ba
Update variants/backend-base/config/initializers/check_env.rb
eoinkelly Jul 23, 2023
3247099
Update variants/backend-base/config/initializers/check_env.rb
eoinkelly Jul 23, 2023
6a78028
Merge branch 'main' into issue-341-modernise-secrets
eoinkelly Jul 23, 2023
c5eacb6
Merge branch 'issue-341-modernise-secrets' of github.com:ackama/rails…
eoinkelly Jul 23, 2023
8f91295
Improve comment based on PR feedback
eoinkelly Jul 23, 2023
24f1153
Update variants/backend-base/example.env.tt
eoinkelly Jul 23, 2023
3801920
Add missing end removed by merge
eoinkelly Jul 23, 2023
31acec3
Ensure new auto-generated example.env secrets not used in prod
eoinkelly Jul 23, 2023
c6f4dc3
Remove unnecessary tweaking of db:encryption secrets
eoinkelly Jul 23, 2023
7cc48c8
Merge branch 'main' into issue-341-modernise-secrets
eoinkelly Aug 25, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -149,4 +149,8 @@ jobs:
PGUSER: postgres
PGPASSWORD: postgres
PGHOST: localhost
RAILS_SECRET_KEY_BASE: "placeholder"
ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY: "placeholder"
ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY: "placeholder"
ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT: "placeholder"
run: ./ci/bin/build-and-test
3 changes: 2 additions & 1 deletion template.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ def apply_template! # rubocop:disable Metrics/MethodLength, Metrics/AbcSize, Met
template "variants/backend-base/Gemfile.tt", "Gemfile", force: true

template "variants/backend-base/example.env.tt", "example.env"
template "variants/backend-base/example.env.tt", ".env"
copy_file "variants/backend-base/editorconfig", ".editorconfig"
copy_file "variants/backend-base/gitignore", ".gitignore", force: true
copy_file "variants/backend-base/overcommit.yml", ".overcommit.yml"
Expand Down Expand Up @@ -118,6 +117,8 @@ def apply_template! # rubocop:disable Metrics/MethodLength, Metrics/AbcSize, Met

run_with_clean_bundler_env "bin/setup"

apply "variants/backend-base/set_active_record_encryption_secrets.rb"

apply "variants/frontend-base/template.rb"
apply "variants/frontend-base/sentry/template.rb"
apply "variants/frontend-base/js-lint/template.rb"
Expand Down
2 changes: 1 addition & 1 deletion variants/backend-base/app/template.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

1 change: 0 additions & 1 deletion variants/backend-base/bin/setup
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ def setup!
run "yarn install" if File.exist?("yarn.lock")
run "bundle exec overcommit --install"
copy "example.env"
copy "config/secrets.example.yml"
test_local_env_contains_required_keys
run "bin/rake tmp:create"
run "bin/rake db:prepare"
Expand Down
24 changes: 24 additions & 0 deletions variants/backend-base/config/app.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Be sure to restart your server when you modify this file.
#
# Use this file to load non-sensitive app config from ENV. Config values here
# will be loaded into `Rails.application.config.app`.
#
# Sensitive config should be put in `config/secrets.yml` (which will load it
# into `Rails.application.secrets`)

default: &default
# The default `From:` address to use for email sent by this application
# obviously isn't a secret per se, but configuring it here is convenient
mail_from: "<%= ENV['MAIL_FROM'] %>"
eoinkelly marked this conversation as resolved.
Show resolved Hide resolved

development:
<<: *default

test:
<<: *default

staging:
<<: *default
Comment on lines +19 to +20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep as is because:

  1. 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).
  2. Having this here doesn't harm a Heroku deploy.
  3. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


production:
<<: *default
13 changes: 13 additions & 0 deletions variants/backend-base/config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,21 @@
# the empty line at the beginning of this string is required
<<-'RUBY'

# load config/app.yml into Rails.application.config.app.*
config.app = config_for(:app)

config.middleware.insert_before Rack::Sendfile, HttpBasicAuth
config.action_dispatch.default_headers["Permissions-Policy"] = "interest-cohort=()"

# ActiveRecord encrypted attributes expectes to find the key secrets under
# `config.active_record.encryption.*`. If the secrets were stored in Rails
# encrypted credentials file then Rails would map them automatically for us.
# We prefer to store the secrets in the ENV and load them through
# `config/secrets.yml` so we have to manually assign them here.
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
Comment on lines +28 to +30
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


config.action_dispatch.default_headers["X-Frame-Options"] = "DENY"
RUBY
end
44 changes: 39 additions & 5 deletions variants/backend-base/config/initializers/check_env.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,42 @@
# frozen-string-literal: true
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.
#
eoinkelly marked this conversation as resolved.
Show resolved Hide resolved
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)
eoinkelly marked this conversation as resolved.
Show resolved Hide resolved
end
end

def local?
Rails.env.development? || Rails.env.test?
end
end
end

VerifyPlaceholderSecretsNotUsedForReal.run
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

32 changes: 0 additions & 32 deletions variants/backend-base/config/secrets.example.yml.tt

This file was deleted.

26 changes: 26 additions & 0 deletions variants/backend-base/config/secrets.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


default: &default
# Your secret key is used for verifying the integrity of signed cookies.
# If you change this key, all old signed cookies will become invalid!
# Make sure the secret is at least 30 characters and all random,
# no regular words or you'll be exposed to dictionary attacks.
# You can use `rails secret` to generate a secure secret key.
secret_key_base: "<%= ENV.fetch('RAILS_SECRET_KEY_BASE') %>"

active_record_encryption_primary_key: "<%= ENV.fetch('ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY') %>"
active_record_encryption_deterministic_key: "<%= ENV.fetch('ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY') %>"
active_record_encryption_key_derivation_salt: "<%= ENV.fetch('ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT') %>"

development:
<<: *default

test:
<<: *default

staging:
<<: *default
Comment on lines +24 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep as is because:

  1. 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).
  2. Having this here doesn't harm a Heroku deploy.
  3. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


production:
<<: *default
6 changes: 4 additions & 2 deletions variants/backend-base/config/template.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

template "variants/backend-base/config/database.yml.tt", "config/database.yml", force: true

template "variants/backend-base/config/secrets.example.yml.tt", "config/secrets.example.yml"
remove_file "config/secrets.yml"
copy_file "variants/backend-base/config/secrets.yml", "config/secrets.yml", force: true
copy_file "variants/backend-base/config/app.yml", "config/app.yml"
remove_file "config/master.key"
remove_file "config/credentials.yml.enc"
Comment on lines +7 to +8
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


copy_file "variants/backend-base/config/puma.rb", "config/puma.rb", force: true

Expand Down
16 changes: 10 additions & 6 deletions variants/backend-base/example.env.tt
Original file line number Diff line number Diff line change
@@ -1,9 +1,3 @@
# Copy this file into a new file called ".envrc" in the root of the project.
# Access values like this: ENV["RAILS_SECRET_KEY_BASE"]
#
# The purpose of this file is to keep secrets out of source control.
# For more information, see: direnv.net

# The environment variables below can be uncommented to enable HTTP basic
# authentication
# HTTP_BASIC_AUTH_USERNAME=example
Expand All @@ -26,3 +20,13 @@ PORT=3000
# SENTRY_DSN=http://public@example.com/project-id
SENTRY_CSP_HEADER_REPORT_ENDPOINT=https://SOMECODE.ingest.sentry.io/api/SOMENUMS/security/?sentry_key=SOMETHING
SENTRY_ENV=development

# NEVER just copy these secrets from development env to a production env. Run:
#
# bin/rails db:encryption:init
#
# 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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

eoinkelly marked this conversation as resolved.
Show resolved Hide resolved
28 changes: 28 additions & 0 deletions variants/backend-base/set_active_record_encryption_secrets.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
def generate_db_secrets
puts "DB_ENCRYPTION_INIT: Running rails db:encryption:init to set realistic values in ENV variables"

raw = `bin/rails db:encryption:init`
unparsed_yaml = raw.sub(/Add.+\n/, "")
parsed = YAML.safe_load(unparsed_yaml)
parsed.fetch("active_record_encryption")
rescue StandardError => e
puts "DB_ENCRYPTION_INIT: Recovering from error: #{e.inspect}"
{
"primary_key" => "FAILED_TO_GENERATE_DEFAULT_PRIMARY_KEY",
"deterministic_key" => "FAILED_TO_GENERATE_DEFAULT_DETERMINISTIC_KEY",
"key_derivation_salt" => "FAILED_TO_GENERATE_DEFAULT_KEY_DERIVATION_SALT"
}
end

# To avoid setting a bad security example we don't use the same secrets for the
# example.env (which is checked in) and your local .env file.
example_env_db_secrets = generate_db_secrets
dot_env_db_secrets = generate_db_secrets

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

gsub_file(".env", "PLACEHOLDER_PRIMARY_KEY", dot_env_db_secrets.fetch("primary_key"))
gsub_file(".env", "PLACEHOLDER_DETERMINISTIC_KEY", dot_env_db_secrets.fetch("deterministic_key"))
gsub_file(".env", "PLACEHOLDER_KEY_DERIVATION_SALT", dot_env_db_secrets.fetch("key_derivation_salt"))
2 changes: 1 addition & 1 deletion variants/devise/template.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@

gsub_file "config/initializers/devise.rb",
" config.mailer_sender = 'please-change-me-at-config-initializers-devise@example.com'",
" config.mailer_sender = Rails.application.secrets.mail_from"
" config.mailer_sender = Rails.application.config.app.mail_from"

gsub_file "config/initializers/devise.rb",
" # config.scoped_views = false",
Expand Down