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

Key rotate #141

Merged
merged 4 commits into from May 12, 2020
Merged

Key rotate #141

merged 4 commits into from May 12, 2020

Conversation

lepture
Copy link
Contributor

@lepture lepture commented Sep 24, 2019

This change is designed for pallets/flask#1574

With this change, Signer and Serializer can accept a list of secret keys. For instance,

  1. In 2019/09, we are using a as the secret key
  2. In 2019/10, we are going to use b as the secret key

For 2019/09, we will use Serializer(['a']), and later in 10, we will use Serializer(['a', 'b']). Now the old dumped values can still be loaded, because a is in secret keys. But it will dump new value with secret key b in 2019/10.

Later, we can remove a from the secret keys, since it is not used anymore. Maybe in 2019/11.

@lepture
Copy link
Contributor Author

lepture commented Sep 24, 2019

Documentation is still missing. This should be a 1.2.0 release. I'm not sure if the parameter secret_key should be renamed to secret_keys.

@ThiefMaster
Copy link
Member

I think removing secret_key breaks backwards compatibility. This could be avoided by making it a property returning self.secret_keys[0].

src/itsdangerous/signer.py Outdated Show resolved Hide resolved
src/itsdangerous/jws.py Show resolved Hide resolved
@davidism
Copy link
Member

davidism commented Apr 15, 2020

I'll write some documentation and push it before merging. The most important part seems to be documenting that the secret_key list is in order of oldest to newest key. This makes sense (use .append to add a new key), but my first instinct was that the first item was the first key.

@davidism
Copy link
Member

I think you removed secret_key= so it wasn't implied to be a named parameter? After thinking about it, I think it should remain secret_key, as the common use case is probably to use one key rather than rotation. Internally the name secret_keys is fine, and I'll ad a secret_key property as ThiefMaster mentioned.

@davidism
Copy link
Member

Ended up making significant changes to the docs, so I'm going to make a separate PR for that.

@davidism davidism merged commit 2881d93 into pallets:master May 12, 2020
@lepture lepture deleted the key-rotate branch July 7, 2020 14:22
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants