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

Create config/secrets.yml file for store of tokens #13298

Merged
merged 7 commits into from Dec 13, 2013
Merged

Conversation

guilleiguaran
Copy link
Member

Create config/secrets.yml file for store of tokens and move config.secret_key_base to this file.

@lukaszx0
Copy link
Member

I propose to rename file tokens.yml to config/secrets.yml. That way it'll make it consistent with config.secrets

@dhh
Copy link
Member

dhh commented Dec 12, 2013

👍 on secrets.yml. Just sounds cooler too.

@lukaszx0
Copy link
Member

@guilleiguaran It would be nice to have it in one commit, can you squash it?

@@ -195,7 +196,7 @@ def env_config
"action_dispatch.parameter_filter" => config.filter_parameters,
"action_dispatch.redirect_filter" => config.filter_redirect,
"action_dispatch.secret_token" => config.secret_token,
"action_dispatch.secret_key_base" => config.secret_key_base,
"action_dispatch.secret_key_base" => secrets.secret_key_base,
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 would be redundant secrets.secret_key_base, I'd like to use secrets.key_base

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this should be changed. secret_key_base will be the
name of the key in yml file and will give you more idea about what it is,
rather then just key_base which means nothing and everything at the same
time. When new user will open secrets.yml for the first time it'll have now
idea what key_base is.

2013/12/12 Roberto Miranda notifications@github.com

In railties/lib/rails/application.rb:

@@ -195,7 +196,7 @@ def env_config
"action_dispatch.parameter_filter" => config.filter_parameters,
"action_dispatch.redirect_filter" => config.filter_redirect,
"action_dispatch.secret_token" => config.secret_token,

  •      "action_dispatch.secret_key_base" => config.secret_key_base,
    
  •      "action_dispatch.secret_key_base" => secrets.secret_key_base,
    

I think would be redundant secrets.secret_key_base, I'd like to use
secrets.key_base


Reply to this email directly or view it on GitHubhttps://github.com//pull/13298/files#r8305785
.

Copy link

Choose a reason for hiding this comment

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

I agree with both robertomiranda and strzalek . secrets.secret... is redundant. secret.key_base means nothing and everything.

I suggest we make it more meaningful by changing it to something like secret.rails_key_base. To distinguish it from other custom secrets that the user (developer) might put there.

Other alternatives that I like even more are

  • secret.rails_key_base
  • secret.rails_key
  • secret.app_key
  • secret.rails_app_key

I think all these are more meaningful then the current one.

@jipiboily
Copy link
Contributor

I too prefer secrets.yml, fwiw.

Also, most probably out of scope, but would it make sense to output a warning if this is not stored in an env var?

@spastorino
Copy link
Contributor

@guilleiguaran other than what I mentioned, looks good!

@csaunders
Copy link

Is this for storing various secrets such as S3 credentials and such?

I like the approach Figaro takes to this to be honest. This just helps encourage a naughty habit of keeping credentials in source control.

@lucianosousa
Copy link
Contributor

Instead of put another stuff/xml/yml config, why don't integrate this with figaro or settingslogic for example.[1]

Sounds not friendly another config file...
Most of people use another settings config file to do that, and not integrated with the rails core just.

I mean, now, I've another config file to fill before run my app.

[1] just a suggestion. Another solution is: do nothing about that!
Keep it simple.

cheers

@carlosantoniodasilva
Copy link
Member

@lucianosousa you won't need to feel anything before you start developing your app, it will work just as before, you'll just need to handle it pretty much the same way you handle your database.yml file in production.

@lucianosousa
Copy link
Contributor

  • database.yml
  • s3.yml
  • settings.yml (settingslogic/figaro/gaston/etc)
  • secrets.yml (new one)

anything else?
Cool idea, but, I really don't think it's pretty good. IHMO.

@codeodor
Copy link
Contributor

I'd propose at the least this file should make its way into the .gitignore Rails generates when you use rails new ... but I think database.yml should be there too.

@carlosantoniodasilva
Copy link
Member

My point is: not everyone uses other configuration tool (at @plataformatec for instance we do not rely on any other tool for that), and managing a file like that has nearly no cost. In any case, if people do not want the file, they can just drop it, and set the config value, as it will keep working as before. Thanks 👍

@lucianosousa
Copy link
Contributor

as @codeodor we cannot ignore database.yml. If we can do that with this file. sounds fine

@csaunders
Copy link

Managing a file like that absolutely has a cost. If you have a poor key rotation strategy for example you are leaving yourself open to having those keys leaked which could have some really bad results. Yes, the same can be said for database.yml but it's pretty much a legacy problem at this point.

Encouraging good key storage practices is better in my opinion and this just makes it too easy to be lazy.

guilleiguaran added a commit that referenced this pull request Dec 13, 2013
Create config/secrets.yml file for store of tokens
@guilleiguaran guilleiguaran merged commit eed8c85 into master Dec 13, 2013
@carlosantoniodasilva carlosantoniodasilva deleted the secret_tokens branch December 15, 2013 19:04
secret_key_base: <%= app_secret %>

production:
secret_key_base: <%= app_secret %>

Choose a reason for hiding this comment

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

Generating the same keys, including a "production" one here, could be a problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this, maybe it should just be a comment giving an example of using ENV or something else like that?

Copy link
Member Author

Choose a reason for hiding this comment

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

This generates three different keys, one per environment 😄

development:
  secret_key_base: 4e58aa438c2483acfb424aa25ba00edc65a97f9ae992cfefd13bad9e4a78f37983775c5c262ab3f7d48fd801ceb50450e616f9213b8f66accf846b23d9d12f48

test:
  secret_key_base: a643bb32a574f288c1fc3b17611450cfdb3a9da40390364242d12d619bc4448a50c8e0ac70bc3d44365123801301e2b3dc6a7d00ac51814e26adfe76b742a721

production:
  secret_key_base: ee1033fd4d4f4693aadeb4bb7f11acb9e082fd794adb1b7b5a10899a2fe4502a4a42aef9195029810325023006b7d0052daa0fdba7b31055b7245606261abaeb

Choose a reason for hiding this comment

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

👍

Choose a reason for hiding this comment

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

While I'm sure there is a lot of history I am unaware of, and not trying to start a potentially larger discussion, but I'd prefer to see rails move AWAY from having files that contain configuration for multiple environments and TOWARDS a more ENV/Foremanesque setup.

The first thing I do on any Rails project is gut it's concept of "environments", it's very limiting and difficult to maintain.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dpehrson I prefer myself the ENV approach (since I prefer to host my apps in Heroku) but it is not the default of rails apps.

In my case I'll edit this file and replace the secret_key_base in production section with <%= ENV["SECRET_KEY_BASE"] %>, I allowed ERB intentionally in this file to do it.

Choose a reason for hiding this comment

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

Yeah, that’s exactly what I’ll be doing via chef. I realize that my comment touches on a much, much bigger point that won’t be addressed here.

Thanks for your work though, it’d be great to see secrets grouped in one place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet