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

Allow `on_rotation` in MessageEncryptor to be passed in constructor: #36270

Merged
merged 1 commit into from Jun 6, 2019

Conversation

Projects
None yet
3 participants
@Edouard-chin
Copy link
Contributor

commented May 13, 2019

Allow on_rotation in MessageEncryptor to be passed in constructor:

  • Use case:

    I'm writing a wrapper around MessageEncryptor to make things easier
    to rotate a secret in our app.

    It currently works something like

    crypt = RotatableSecret.new(['old_secret', 'new_secret'])
    crypt.decrypt_and_verify(message, on_rotation: -> { ... })

    I'd like the caller to not have to care about passing the
    on_rotation option and have the wrapper deal with it when
    instantiating the MessageEncryptor object.

    Also, almost all of the time the on_rotation should be the same when
    rotating a secret (logging something or StatsD event) so I think
    it's not worth having to repeat ourselves each time we decrypt a message.

@rails-bot rails-bot bot added the activesupport label May 13, 2019

@Edouard-chin Edouard-chin force-pushed the Edouard-chin:ec-on-rotation-constructor branch 2 times, most recently from ac1a51f to 59c20fd May 14, 2019

@Edouard-chin

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

cc/ @rafaelfranca (since I know you declared bankruptcy on notifications recently 😄)

@kaspth

This comment has been minimized.

Copy link
Member

commented May 26, 2019

I'm curious what you're doing in on_rotation just passing data to statsd?

@Edouard-chin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

thanks for the feedbacks, pushed a new commit.

I'm curious what you're doing in on_rotation just passing data to statsd?

Yeah just passing data to statsd to know when the old secrets is no longer used.

@Edouard-chin Edouard-chin force-pushed the Edouard-chin:ec-on-rotation-constructor branch 2 times, most recently from 8b8e0a5 to 95e9974 Jun 6, 2019

@kaspth

kaspth approved these changes Jun 6, 2019

Copy link
Member

left a comment

Now we just got to get a passing build.

I've been trying to rebuild but then the checkout fails https://buildkite.com/rails/rails/builds/61568#_

@Edouard-chin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Thanks I'll try to amend/force push a bit later today. Might just be some network blips

Allow `on_rotation` in MessageEncryptor to be passed in constructor:
- Use case:

  I'm writing a wrapper around MessageEncryptor to make things easier
  to rotate a secret in our app.

  It works something like
  ```ruby
  crypt = RotatableSecret.new(['old_secret', 'new_secret'])
  crypt.decrypt_and_verify(message)
  ```

  I'd like the caller to not have to care about passing the
  `on_rotation` option and have the wrapper deal with it when
  instantiating the MessageEncryptor object.

  Also, almost all of the time the on_rotation should be the same when
  rotating a secret (logging something or StatsD event) so I think
  it's not worth having to repeat ourselves each time we decrypt a message.

@Edouard-chin Edouard-chin force-pushed the Edouard-chin:ec-on-rotation-constructor branch from 95e9974 to a5502f4 Jun 6, 2019

@Edouard-chin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

should be good now

@kaspth kaspth merged commit 480d9f2 into rails:master Jun 6, 2019

2 checks passed

buildkite/rails Build #61569 passed (9 minutes, 1 second)
Details
codeclimate All good!
Details

@Edouard-chin Edouard-chin deleted the Edouard-chin:ec-on-rotation-constructor branch Jun 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.