-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add Multifernet.rotate method #3979
Changes from 7 commits
c14309d
7bb56bb
bf6ec57
e9e3b80
e5983a2
6a40caf
ea34b0a
fc01194
ef0ab94
ecf768d
331bd36
dd22738
6ba329b
6aae7d9
b4629ee
6bc8daf
414fdb5
5509ef9
054d64d
5ca5794
a6d2dff
cb51edd
0ed4ea8
ef70387
1b08fcd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,14 +110,15 @@ has support for implementing key rotation via :class:`MultiFernet`. | |
the front of the list to start encrypting new messages, and remove old keys | ||
as they are no longer needed. | ||
|
||
.. method:: rotate(msg, ttl=None) | ||
.. method:: rotate(msg) | ||
|
||
.. versionadded:: 2.2 | ||
|
||
Rotating tokens is necessary when any of the fernet keys of a | ||
:class:`MultiFernet` instance have been compromised. This decrypts the | ||
token `msg` using any of the keys attached to the :class:`MultiFernet` | ||
instance and then re-encrypts the token using the primary key. | ||
instance and then re-encrypts the token using the primary key. This | ||
preserves the timestamp that was originally saved with the token. | ||
|
||
If a token has successfully been rotated, then the rotated token will be | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comma and the one in the next sentence are both superfluous. |
||
returned. If rotation fails, this will raise an exception. | ||
|
@@ -140,19 +141,13 @@ has support for implementing key rotation via :class:`MultiFernet`. | |
'Secret message!' | ||
|
||
:param bytes msg: The token to re-encrypt. | ||
:param int ttl: Optionally, the number of seconds old a message may be | ||
for it to be valid. If the message is older than ``ttl`` seconds | ||
(from the time it was originally created) an exception will be | ||
raised. If ``ttl`` is not provided (or is ``None``), the age of the | ||
message is not considered. If any token is older than the ``ttl`` | ||
then an exception will be raised. | ||
:returns bytes: A secure message that cannot be read or altered without | ||
the key. This is URL-safe base64-encoded. This is referred to as a | ||
"Fernet token". | ||
:raises cryptography.fernet.InvalidToken: If a ``token`` is in any | ||
way invalid, this exception is raised. A token may be invalid for a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "If a token is invalid this exception is raised" is probably all we need here. |
||
number of reasons: it is older than the ``ttl``, it is malformed, or | ||
it does not have a valid signature. | ||
number of reasons: it is malformed, or it does not have a valid | ||
signature. | ||
:raises TypeError: This exception is raised if the ``msg`` is not | ||
``bytes``. | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ cryptographic | |
cryptographically | ||
Debian | ||
decrypt | ||
decrypts | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to register my (continued) displeasure that enchant can't figure out plurals. |
||
Decrypts | ||
decrypted | ||
decrypting | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,11 +71,14 @@ def _encrypt_from_parts(self, data, current_time, iv): | |
return base64.urlsafe_b64encode(basic_parts + hmac) | ||
|
||
def decrypt(self, token, ttl=None): | ||
timestamp, data = self._get_token_data(token) | ||
return self._decrypt_data(data, timestamp, ttl) | ||
|
||
@staticmethod | ||
def _get_token_data(token): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this returns a timestamp that may be invalid (we haven't performed the HMAC verification), let's call this (For our purposes it's okay to not have validated it yet as the call to |
||
if not isinstance(token, bytes): | ||
raise TypeError("token must be bytes.") | ||
|
||
current_time = int(time.time()) | ||
|
||
try: | ||
data = base64.urlsafe_b64decode(token) | ||
except (TypeError, binascii.Error): | ||
|
@@ -88,6 +91,10 @@ def decrypt(self, token, ttl=None): | |
timestamp, = struct.unpack(">Q", data[1:9]) | ||
except struct.error: | ||
raise InvalidToken | ||
return timestamp, data | ||
|
||
def _decrypt_data(self, data, timestamp, ttl): | ||
current_time = int(time.time()) | ||
if ttl is not None: | ||
if timestamp + ttl < current_time: | ||
raise InvalidToken | ||
|
@@ -134,8 +141,19 @@ def __init__(self, fernets): | |
def encrypt(self, msg): | ||
return self._fernets[0].encrypt(msg) | ||
|
||
def rotate(self, msg, ttl=None): | ||
return self.encrypt(self.decrypt(msg, ttl)) | ||
def rotate(self, msg): | ||
timestamp, data = self._fernets[0]._get_token_data(msg) | ||
for f in self._fernets: | ||
try: | ||
p = f._decrypt_data(data, timestamp, None) | ||
break | ||
except InvalidToken: | ||
pass | ||
else: | ||
raise InvalidToken | ||
|
||
iv = os.urandom(16) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah. Well that's a good reason. I still don't like os.urandom being called in two places, but maybe I need to just be okay with that. |
||
return self._fernets[0]._encrypt_from_parts(p, timestamp, iv) | ||
|
||
def decrypt(self, msg, ttl=None): | ||
for f in self._fernets: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this first sentence. Token rotation isn't just to limit the compromise window -- It's also for policy (rotation post-employee departure). Rotation in the event of actual compromise is a much more involved affair. Rotation as a best practice/cryptographic hygiene policy is designed to limit the damage in the event of an undetected event/increase the difficulty of attacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reaperhulk does this work a bit better?
I initially added the sentence "rotating tokens is necessary..." to try provide a reason for someone to want to use
rotate
. I'm wondering if it might make more sense to pull the second part of the docstring I've proposed out ofrotate
and into the docs forMultiFernet
itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we have a section about key rotation in the multifernet class introduction so it probably does make sense to expand on it up there and link to the
rotate
method.