Restore some config.secret_key_base functionality #52062

skipkayhil · 2024-06-08T15:00:53Z

Motivation / Background

The deprecated secrets removal ended up removing a bit of non-deprecated functionality related to config.secret_key_base:

the original implementation prioritized the value of config.secret_key_base over other sources in all environments
if unset, the value of config.secret_key_base would be updated to whichever fallback value was found

The new implementation only sets config.secret_key_base to a fallback value when Rails.env.local?, and never considers it at all in production.

Detail

This commit aims to restore this missing functionality as well as simplify the implementation:

Rails.application.secret_key_base now always delegates to config.secret_key_base (like the pre-secret-removal implementation)
secret_key_base validation was moved from the reader to the writer
config.secret_key_base now handles setting itself to a fallback value when unset
In addition, generate_local_secret was simplified because it previously did 3 things: file manipulation, setting config.secret_key_base, and returning a value. Now it only creates the file if necessary and returns the value stored in it

The new implementation has an additional benefit, which is that manually set config.secret_key_base values are now validated, whereas previously only fallback values were validated.

Detail

Add it back with a test

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

skipkayhil · 2024-06-08T15:01:24Z

railties/test/application/configuration_test.rb

    end

-    test "config.secret_key_base over-writes a blank app.secret_key_base" do
+    test "app.secret_key_base falls back to config.secret_key_base in production" do


I repurposed this test because it was doing the exact same thing as the test above

p8 · 2024-06-10T18:51:04Z

railties/lib/rails/application/configuration.rb

+          self.secret_key_base = if Rails.env.local? || ENV["SECRET_KEY_BASE_DUMMY"]
+            generate_local_secret
+          else
+            ENV["SECRET_KEY_BASE"] || Rails.application.credentials.secret_key_base


Is credentials already available as an accessor?

Suggested change

ENV["SECRET_KEY_BASE"] || Rails.application.credentials.secret_key_base

ENV["SECRET_KEY_BASE"] || credentials.secret_key_base

I initially had the same thought. There is a credentials accessor here, however its for configuration and not the actual credentials object on Rails.application

https://guides.rubyonrails.org/configuring.html#config-credentials-content-path

railties/lib/rails/application/configuration.rb

The [deprecated secrets removal][1] ended up removing a bit of non-deprecated functionality related to config.secret_key_base: - the original implementation prioritized the value of config.secret_key_base over other sources in all environments - if unset, the value of config.secret_key_base would be updated to whichever fallback value was found The new implementation only sets config.secret_key_base to a fallback value when Rails.env.local?, and never considers it at all in production. This commit aims to restore this missing functionality as well as simplify the implementation: - Rails.application.secret_key_base now always delegates to config.secret_key_base (like the pre-secret-removal implementation) - secret_key_base validation was moved from the reader to the writer - config.secret_key_base now handles setting itself to a fallback value when unset - In addition, generate_local_secret was simplified because it previously did 3 things: file manipulation, setting config.secret_key_base, and returning a value. Now it only creates the file if necessary and returns the value stored in it The new implementation has an additional benefit, which is that manually set config.secret_key_base values are now validated, whereas previously only fallback values were validated. [1]: 0c76f17 Co-authored-by: Petrik <[email protected]>

Restore some config.secret_key_base functionality

ghiculescu · 2024-06-18T02:12:33Z

Before this change, Rails.application.config.secret_key_base = nil worked locally. A dummy value would get used:

Now it raises here:

rails/railties/lib/rails/application/configuration.rb

Line 517 in 5d34172

    
           raise ArgumentError, "Missing `secret_key_base` for '#{Rails.env}' environment, set this string with `bin/rails credentials:edit`"

Would you consider a PR that brings back support for setting this config to nil, or was that never meant to be supported?

ghiculescu · 2024-06-18T02:16:11Z

I think nil support is valid so that you can have code like this:

# config/init/secrets.rb

Rails.application.config.secret_key_base = ENV["SECRET_KEY_BASE"]

SECRET_KEY_BASE_DUMMY=1 bin/rails runner 'puts 1'

This will now crash if you don't have SECRET_KEY_BASE in your environment.

The workaround is not as nice:

if ENV["SECRET_KEY_BASE"]
  Rails.application.config.secret_key_base = ENV["SECRET_KEY_BASE"]
end

skipkayhil · 2024-06-18T03:36:31Z

That's interesting, I definitely didn't expect applications to be setting it to nil intentionally. I'm happy to open a PR to restore validating on read instead of write, I'll ask Rafael tomorrow to see what he thinks.

Is there a reason you're explicitly using ENV["SECRET_KEY_BASE"] instead of just letting the value fall through to ENV anyways?

ghiculescu · 2024-06-18T03:49:06Z

This code is many years old, so I'm not sure. I guess I could see if removing it breaks anything.

chrishulton · 2024-07-16T21:27:42Z

I also am noticing this in a legacy application pointed against 7.2.0.beta3 set up like this:

# config/application.rb

config.secret_key_base = Configster.config_for('Global').secret_key_base

This would return nil in the development environment, and worked previously. Now it raises with "ArgumentError: Missing secret_key_base for 'development' environment".

Did you end up deciding on whether this behavior should be restored?

skipkayhil · 2024-07-17T15:15:56Z

I opened a PR above, since I think we should either restore the old behavior or at least deprecate to not break application unnecessarily 👍

rails-bot bot added the railties label Jun 8, 2024

skipkayhil commented Jun 8, 2024

View reviewed changes

skipkayhil added regression needs backport labels Jun 8, 2024

skipkayhil added this to the 7.2.0 milestone Jun 8, 2024

skipkayhil force-pushed the hm-restore-config-skb branch 2 times, most recently from 937c090 to 4842e3f Compare June 8, 2024 19:49

skipkayhil changed the title ~~Restore support for config.secret_key_base in prod~~ Restore some config.secret_key_base functionality Jun 8, 2024

p8 reviewed Jun 10, 2024

View reviewed changes

railties/lib/rails/application/configuration.rb Outdated Show resolved Hide resolved

skipkayhil force-pushed the hm-restore-config-skb branch from 4842e3f to c2901eb Compare June 11, 2024 03:05

rafaelfranca merged commit c60dbbd into rails:main Jun 11, 2024

rafaelfranca added a commit that referenced this pull request Jun 11, 2024

Merge pull request #52062 from skipkayhil/hm-restore-config-skb

cb3011d

Restore some config.secret_key_base functionality

skipkayhil deleted the hm-restore-config-skb branch June 12, 2024 00:18

skipkayhil mentioned this pull request Jul 17, 2024

Reallow setting secret_key_base to nil when local #52351

Merged

4 tasks

denisahearn mentioned this pull request Nov 18, 2024

Rails 7.2 "secret_key_base must be a type of String" issue when running tests in parallel in a CI build #53661

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Restore some config.secret_key_base functionality #52062

Restore some config.secret_key_base functionality #52062

Uh oh!

skipkayhil commented Jun 8, 2024 •

edited

Loading

Uh oh!

skipkayhil Jun 8, 2024

Uh oh!

p8 Jun 10, 2024

Uh oh!

skipkayhil Jun 11, 2024

Uh oh!

Uh oh!

ghiculescu commented Jun 18, 2024

Uh oh!

ghiculescu commented Jun 18, 2024

Uh oh!

skipkayhil commented Jun 18, 2024

Uh oh!

ghiculescu commented Jun 18, 2024 •

edited

Loading

Uh oh!

chrishulton commented Jul 16, 2024

Uh oh!

skipkayhil commented Jul 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	ENV["SECRET_KEY_BASE"] \|\| Rails.application.credentials.secret_key_base
	ENV["SECRET_KEY_BASE"] \|\| credentials.secret_key_base

Restore some config.secret_key_base functionality #52062

Restore some config.secret_key_base functionality #52062

Uh oh!

Conversation

skipkayhil commented Jun 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation / Background

Detail

Detail

Checklist

Uh oh!

skipkayhil Jun 8, 2024

Choose a reason for hiding this comment

Uh oh!

p8 Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

skipkayhil Jun 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghiculescu commented Jun 18, 2024

Uh oh!

ghiculescu commented Jun 18, 2024

Uh oh!

skipkayhil commented Jun 18, 2024

Uh oh!

ghiculescu commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrishulton commented Jul 16, 2024

Uh oh!

skipkayhil commented Jul 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

skipkayhil commented Jun 8, 2024 •

edited

Loading

ghiculescu commented Jun 18, 2024 •

edited

Loading