Add rails credentials support #355

noxasch · 2024-03-08T19:58:39Z

Added rails credentials support with config flag addressing #68

noxasch · 2024-03-14T05:40:36Z

I'm currently having trouble with different ruby version in the test, any clue ?

cjlarose · 2024-03-16T07:14:03Z

Not 100% sure what's going on with the tests on CI. Tests pass for me locally. I suspect it has something to do with Rails 7.0 or 7.1 because we don't run tests for those Rails versions when running the test suite for Ruby 2.7, jruby, or truffleruby.

I merged in a change that address the deprecation warnings for fixture_path in Rails 7.1 to help clear up the test output. Hoping that will help clear things up 🤞

lib/config.rb

noxasch · 2024-03-16T12:17:46Z

Update:

Rebased to latest master

I look around and found the solution for rails 7.1 fail test. For Rails 7.1 and above it seems we need to use ActiveSupport::EncryptedConfiguration config method instead. But prior 7.1, Rails.credentials.to_h and Rails.secret.to_h will work just fine.

All test should pass now.

cjlarose

Awesome work! Requested a few changes

cjlarose · 2024-03-16T16:03:30Z

lib/config.rb

@@ -48,6 +49,14 @@ def self.load_files(*sources)

    config.add_source!(Sources::EnvSource.new(ENV)) if Config.use_env

+    if defined?(::Rails::Railtie) && Config.use_rails_credentials
+      if Rails.version < '7.1'


Because the version numbers are represented as strings here, this comparison can lead to unexpected results. For example, if Rails.version is 10.0.0, then '10.0.0' < '7.1' == true

To compare version numbers correctly, I think we need to either use something like

if [Rails::VERSION::MAJOR, Rails::VERSION::MINOR] < [7, 1]

or

if Gem::Version.new(Rails.version) < Gem::Version.new('7.1')

cjlarose · 2024-03-16T16:12:25Z

lib/config.rb

@@ -48,6 +49,14 @@ def self.load_files(*sources)

    config.add_source!(Sources::EnvSource.new(ENV)) if Config.use_env

+    if defined?(::Rails::Railtie) && Config.use_rails_credentials
+      if Rails.version < '7.1'
+        config.add_source!(Sources::HashSource.new(secret: Rails.application.secrets.to_h.deep_stringify_keys))


Let's remove the nesting of the credentials under the top-level secret key. I think that'll be more useful as the default behavior in order to support merging with other config sources. For example, if my config.yaml file has

aws: region: 'us-west-1'

And my Rails credentials has

aws: secret_access_key: '123456'

...then I'd want to be able to access Settings.aws, which would contain both region and secret_access_key.

If someone really wants to keep their secrets in a separate key, they can always turn off use_rails_credentials add it themselves in an initializer

Settings.add_source!({ secret: Rails.application.credentials.config.deep_stringify_keys }) Settings.reload!

Awesome, agree on this.

cjlarose · 2024-03-16T16:13:18Z

lib/config.rb

+      if Rails.version < '7.1'
+        config.add_source!(Sources::HashSource.new(secret: Rails.application.secrets.to_h.deep_stringify_keys))
+      else
+        config.add_source!(Sources::HashSource.new(secret: Rails.application.credentials.config))


I think Rails.application.credentials.config returns a hash with symbol keys. I think for merging to work correctly, we need to .deep_stringify_keys here.

cjlarose · 2024-03-16T16:14:08Z

lib/config.rb

@@ -48,6 +49,14 @@ def self.load_files(*sources)

    config.add_source!(Sources::EnvSource.new(ENV)) if Config.use_env


Let's load credentials before the environment. My expectation is usually that env vars should "win" against every other configuration source.

cjlarose · 2024-03-16T16:16:33Z

spec/app/rails_7.1/config/credentials/test.yml.enc

@@ -0,0 +1 @@
+qU0cAsjfKz4lnoJPU36JuM7Yh3qm6B2YV7LhqJROKCu4AsOW0AfFY9FM+aTeRsZOIdIBHyyargCI1xmq3N5o4rdVZRXxWIt2PD93xZPlcMrlAb645m8hyni1cW4=--dPokgPsIyoKhzMzD--nb/fnuH1FaTNT5iF8J3TfQ==


I tried looking at these values locally with

cd spec/app/rails_7.1 RAILS_MASTER_KEY='0e29551e5c31acf7c769d64397af54e4' bundle exec rails credentials:edit

but I got an error

Editing config/credentials.yml.enc... Couldn't decrypt config/credentials.yml.enc. Perhaps you passed the wrong key?

...but it seems to work on CI, so it's a little weird. Any idea how I can look at these values locally?

@cjlarose just remove the single quotes

noxasch · 2024-03-17T14:56:01Z

Update:

Addressed the changes and refactor the code.
Update the crendentails to include the aws patter below to be more realistic

aws:
  secret_access_key: '123456'

Apparently we don't need to check for rails version, just need to require the master_key in test environment and for rails5.2 untiil 6.1 need test.key and master.key, otherwise it somehow ignore the RAILS_MASTER_KEY in environment variable

noxasch · 2024-04-23T07:47:58Z

@cjlarose let me know if there is anything else require.

utilize ActiveSupport::EncryptedConfiguration config method

nickpoorman · 2024-09-06T22:24:17Z

@cjlarose @noxasch can we please get this merged? Thanks!

pkuczynski · 2024-09-17T20:55:15Z

@cjlarose since you reviewed in the past, would you mind doing a re-review? Seems that all your feedback was addressed?

@noxasch would you mind adding a CHANGELOG entry?

noxasch force-pushed the feat/add-rails-credentials-support branch from a24c238 to 61810ff Compare March 8, 2024 20:09

pkuczynski requested review from cjlarose and BuonOmo March 9, 2024 20:52

pkuczynski added the feature label Mar 9, 2024

pkuczynski added this to the Next milestone Mar 9, 2024

cjlarose reviewed Mar 16, 2024

View reviewed changes

lib/config.rb Outdated Show resolved Hide resolved

noxasch force-pushed the feat/add-rails-credentials-support branch 9 times, most recently from 075d284 to ef7ccc8 Compare March 16, 2024 12:17

noxasch requested a review from cjlarose March 16, 2024 12:18

cjlarose requested changes Mar 16, 2024

View reviewed changes

noxasch force-pushed the feat/add-rails-credentials-support branch 8 times, most recently from 36b06f6 to f67fadf Compare March 17, 2024 14:52

noxasch requested a review from cjlarose March 18, 2024 00:08

BuonOmo removed their request for review March 24, 2024 14:28

noxasch added 7 commits May 16, 2024 23:50

feat: add rails crendentails support

2eec958

fix: fix rails 7.1 support

7f6bfb7

utilize ActiveSupport::EncryptedConfiguration config method

docs: update rails crendetial usage

11b8941

refactor: change crendetials

12a1d42

fix: rails 5.2 and rails 6.0 support

5c51e58

fix: update credentials

40dcc07

refactor: load crendentials before env

de1bb93

noxasch force-pushed the feat/add-rails-credentials-support branch from 4eddca8 to de1bb93 Compare May 16, 2024 15:52

Merge branch 'master' into feat/add-rails-credentials-support

4d4ffda

chore: update changelog

6b28f21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rails credentials support #355

Add rails credentials support #355

noxasch commented Mar 8, 2024 •

edited

Loading

noxasch commented Mar 14, 2024 •

edited

Loading

cjlarose commented Mar 16, 2024

noxasch commented Mar 16, 2024 •

edited

Loading

cjlarose left a comment

cjlarose Mar 16, 2024

cjlarose Mar 16, 2024

noxasch Mar 16, 2024

cjlarose Mar 16, 2024

cjlarose Mar 16, 2024

cjlarose Mar 16, 2024

noxasch Mar 16, 2024

noxasch commented Mar 17, 2024 •

edited

Loading

noxasch commented Apr 23, 2024

nickpoorman commented Sep 6, 2024

pkuczynski commented Sep 17, 2024

		@@ -48,6 +49,14 @@ def self.load_files(*sources)

		config.add_source!(Sources::EnvSource.new(ENV)) if Config.use_env

		@@ -0,0 +1 @@
		qU0cAsjfKz4lnoJPU36JuM7Yh3qm6B2YV7LhqJROKCu4AsOW0AfFY9FM+aTeRsZOIdIBHyyargCI1xmq3N5o4rdVZRXxWIt2PD93xZPlcMrlAb645m8hyni1cW4=--dPokgPsIyoKhzMzD--nb/fnuH1FaTNT5iF8J3TfQ==

Add rails credentials support #355

Are you sure you want to change the base?

Add rails credentials support #355

Conversation

noxasch commented Mar 8, 2024 • edited Loading

noxasch commented Mar 14, 2024 • edited Loading

cjlarose commented Mar 16, 2024

noxasch commented Mar 16, 2024 • edited Loading

cjlarose left a comment

Choose a reason for hiding this comment

cjlarose Mar 16, 2024

Choose a reason for hiding this comment

cjlarose Mar 16, 2024

Choose a reason for hiding this comment

noxasch Mar 16, 2024

Choose a reason for hiding this comment

cjlarose Mar 16, 2024

Choose a reason for hiding this comment

cjlarose Mar 16, 2024

Choose a reason for hiding this comment

cjlarose Mar 16, 2024

Choose a reason for hiding this comment

noxasch Mar 16, 2024

Choose a reason for hiding this comment

noxasch commented Mar 17, 2024 • edited Loading

noxasch commented Apr 23, 2024

nickpoorman commented Sep 6, 2024

pkuczynski commented Sep 17, 2024

noxasch commented Mar 8, 2024 •

edited

Loading

noxasch commented Mar 14, 2024 •

edited

Loading

noxasch commented Mar 16, 2024 •

edited

Loading

noxasch commented Mar 17, 2024 •

edited

Loading