Fixes for use_authenticated_cookie_encryption #30709
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
| key_len = ActiveSupport::MessageEncryptor.key_len(encrypted_cookie_cipher) | ||
| secret = request.key_generator.generate_key(request.authenticated_encrypted_cookie_salt, key_len) | ||
| @encryptor = ActiveSupport::MessageEncryptor.new(secret, cipher: encrypted_cookie_cipher, serializer: SERIALIZER) | ||
| if request.use_authenticated_cookie_encryption |
There was a problem hiding this comment.
Thought upgrade_legacy_hmac_aes_cbc_cookies? was would handle this? I don't understand the need for this if at all, and I certainly wouldn't just reading the code 😊
There was a problem hiding this comment.
I feel like the expectation of setting action_dispatch.use_authenticated_cookie_encryption to false would be encrypted cookies use CBC encryption.
There was a problem hiding this comment.
I really don't see who would assign this to false. It's an upgrade option.
Apps generated on 5.2 default to true and we hide the config in
— they're not meant to switch it off.Apps upgrading from 5.1 get a new_framework_defaults_5_2.rb containing a commented out # config.use_authenticated_cookie_encryption = true. In which case we fallback to adding a legacy encryptor.
Are you saying that we default — no matter the value of that config — generate cookies that can't be read by Rails 5.1 apps? E.g. making it impossible for apps to rollback in case of a haywire 5.2 upgrade?
There was a problem hiding this comment.
Would users upgrading from an older version to 5.2 ever set this to false?
Rolling back is a different question, I think. If a cookie has already been rewritten using GCM and the version is then rolled back an earlier version, there's nothing in place in 5.1 (or the earlier version) to rotate "downward" from GCM to CBC. This could also be problematic if people were to use different versions in production...
There was a problem hiding this comment.
From what I gather here, my concern is that for 5.1 upgrading users even if the config is commented out, # config.use_authenticated_cookie_encryption = true, that we're still going to be upgrading their cookies behind their backs.
Once users then comment the config in there's no going back to a previous Rails version. I'm okay with that as long as we're clear about that in the new_framework_defaults_5_2.rb file.
There was a problem hiding this comment.
From what I gather here, my concern is that for 5.1 upgrading users even if the config is commented out, # config.use_authenticated_cookie_encryption = true, that we're still going to be upgrading their cookies behind their backs.
I believe this is correct. For this case, we'd be writing cookies with GCM and not even trying to do upgrade rotation from CBC to GCM. Thus, I think if action_dispatch.use_authenticated_cookie_encryption is false, we should just use CBC for encrypted cookies.
Use CBC encryption is this configuration value is set to false
mjc-gh
force-pushed
the
actiondispatch-use-aead-encrypted-cookies-patch
branch
from
bfd351a
to
0fb6b2d
Compare
Summary
Have the cookies middleware use CBC encryption is this configuration value is set to
false.Other Information
/r? @kaspth