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

feat: Rotation Function to HMAC Strategy #662

Closed
wants to merge 3 commits into from
Closed

Conversation

erictg
Copy link

@erictg erictg commented Apr 6, 2022

The usage of key rotation for the HMAC Strategy was unclear so I added a function to do key rotation.

Related Issue or Design Document

Checklist

  • I have read the contributing guidelines
    and signed the CLA.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added necessary documentation within the code base (if
    appropriate).

Further comments

@erictg erictg requested a review from aeneasr as a code owner April 6, 2022 03:18
@erictg erictg changed the title Add Rotation Function to HMAC Strategy feat: Rotation Function to HMAC Strategy Apr 6, 2022
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Unfortunately, on second thought, I don't think it makes sense to merge it. The code itself is perfect, and the idea solid. But unfortunately we are planning to deprecate all static config options to finally introduce hot reloading into Ory Fosite and Ory Hydra. To do hot reloading, we can no longer configure things as static fields in services or strategies, but will instead rely on function calls.

For that reason, we would probably need to remove this piece of code once that feature lands, so it's better to not introduce it and prevent any breaking changes.

Your contribution is still valuable, it just does not align with the things we have planned for the next couple of weeks and months. Sorry :(

@aeneasr aeneasr closed this Apr 11, 2022
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.

2 participants