Add Multifernet.rotate method #3979

derwolfe · 2017-10-13T01:45:25Z

This addresses #3882.

Adds a rotate(msgs, ttl) method to Multifernet to make it a easier to re-encrypt old tokens under a new primary key.

Thanks!

derwolfe · 2017-10-13T01:53:29Z

I just realized that there should be some documentation for this as well.

reaperhulk · 2017-10-13T02:36:14Z

tests/test_fernet.py

+
+        rotated = mf2.rotate([mf1.encrypt(plaintext)])[0]
+
+        assert rotated != mf1_ciphertext


This assert would be true even if the rotation didn't occur because you're calling mf1.encrypt(plaintext) again.

whoops - I intended to reuse mf1_ciphertext instead of encrypting it again.

alex · 2017-10-13T12:57:45Z

src/cryptography/fernet.py

@@ -134,6 +134,9 @@ def __init__(self, fernets):
    def encrypt(self, msg):
        return self._fernets[0].encrypt(msg)

+    def rotate(self, msgs, ttl=None):
+        return [self.encrypt(self.decrypt(m, ttl)) for m in msgs]


Why does this work on lists of messages? No other fernet APIs do.

The main use case I was targeting was rotating an entire set of secrets from one key to another. That said, this could just operate on a single token, and we could leave it up to the consumer to handle larger groups. @alex is that what you'd prefer?

alex · 2017-10-13T13:15:44Z

Yes, I would :-)

…

On Fri, Oct 13, 2017 at 9:15 AM, Chris Wolfe ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/cryptography/fernet.py <#3979 (comment)>: > @@ -134,6 +134,9 @@ def __init__(self, fernets): def encrypt(self, msg): return self._fernets[0].encrypt(msg) + def rotate(self, msgs, ttl=None): + return [self.encrypt(self.decrypt(m, ttl)) for m in msgs] The main use case I was targeting was rotating an entire set of secrets from one key to another. That said, this could just operate on a single token, and we could leave it up to the consumer to handle larger groups. @alex <https://github.com/alex> is that what you'd prefer? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3979 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAADBOkW5lKFVR6HHjsyaFxDHSIQp9crks5sr2JYgaJpZM4P33Mm> .

-- "I disapprove of what you say, but I will defend to the death your right to say it." -- Evelyn Beatrice Hall (summarizing Voltaire) "The people's good is the highest law." -- Cicero GPG Key fingerprint: D1B3 ADC0 E023 8CA6

reaperhulk · 2017-10-13T14:26:19Z

docs/fernet.rst

@@ -109,6 +115,34 @@ 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)


Needs a .. versionadded:: 2.2

reaperhulk · 2017-10-13T14:27:28Z

docs/fernet.rst

+                                                  ``ttl``, it is malformed, or
+                                                  it does not have a valid
+                                                  signature.
+        :raises TypeError: This exception is raised if the ```msg`` is not


Double back ticks here

reaperhulk · 2017-10-13T14:27:52Z

docs/fernet.rst

+
+        :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


This is a petty style complaint but I prefer this with a single indent level for multiple lines and not aligned.

reaperhulk · 2017-10-13T15:01:54Z

docs/fernet.rst

@@ -99,6 +100,11 @@ has support for implementing key rotation via :class:`MultiFernet`.
        '...'
        >>> f.decrypt(token)
        'Secret message!'
+        >>> key3 = Fernet(Fernet.generate_key())


I think we should move the doctest example to under the rotate method itself

add versionadded, drop rotate from class doctest

alex · 2017-10-13T17:26:49Z

Can you pleaese add a changelog entry? (Haven't reviewed teh rest of this patch)

derwolfe · 2017-10-13T17:27:28Z

Sure!

On Fri, Oct 13, 2017 at 12:26 Alex Gaynor ***@***.***> wrote: Can you pleaese add a changelog entry? (Haven't reviewed teh rest of this patch) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3979 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnJTYgwYncmFBF1G9rrgxmmOOBdqtNMks5sr51cgaJpZM4P33Mm> .

-- - Chris

alex · 2017-10-14T00:51:51Z

docs/fernet.rst

+
+        Re-encrypts the provided fernet token. If the token has successfully
+        been re-encrypted, then the re-encrypted token will be returned. This
+        will raise an exception if re-encryption fails.


Please explain why someone would want this.

alex · 2017-10-14T00:52:36Z

tests/test_fernet.py

+        rotated = mf2.rotate(mf1_ciphertext)
+
+        assert rotated != mf1_ciphertext
+        assert mf2.decrypt(rotated) == plaintext


Please add an asseriotn that mf1 can't decrypt something rotated by mf2.

…t the token

alex · 2017-10-15T15:16:24Z

mmm, I just realized this resets the timestamp in the token. Is that intended? is that good?

derwolfe · 2017-10-15T23:37:25Z

@alex good point. I think it is also made somewhat worse by the caller being able to pass in the TTL, thereby setting up the assumption that token might contain the same timestamp that it did when encrypted.

I don't really have a good solution for this. The following two options come to mind:

I think it could be acceptable to have an enormous, flashing lights style warning in the docs stating that this will re-encrypt and thereby change the timestamp of the token. This does seem somewhat reasonable to me in that, were I to want to re-encrypt a bunch of fernet tokens due to a key compromise and were I relying on the timestamp stored on the token for something like a session, I'd likely want to to invalidate all sessions encrypted under the compromised key anyway.

Further, with the current interface provided by Fernet/MultiFernet, there isn't a way to pass in a custom timestamp with a token. Given that, I'd expect (guess) that consumers of this are likely already handling TTL adjustments themselves. That said, I'm sure there are use cases that I'm not really considering (I've not used TTLs in practice at all).
Another solution might be to make it possible to pass in a timestamp to use when rotating. I'm not sure if this would be advisable or in violation of the spec.

Conceptually, fernet takes a user-provided message (an arbitrary sequence of bytes), a key (256 bits), and the current time, and produces a token, which contains the message in a form that can't be read or altered without the key.

If we wanted to make a slightly different interface to Fernet that would allow the user to specify the current time, then tokens could be rotated using their original time. That said, we'd then have to make it relatively easy for users to extract that time from their tokens.

I do think that option #1 would be easier to understand and maintain. I also think that it most closely matches what a user would expect of a rotation function given Fernet's current interface.

What do you think?

Also, thank you for both for your reviews :)

alex · 2017-10-15T23:38:33Z

I think the right thing to do is probably reuse the original timestamp; though I'd be interested in hearing @reaperhulk's thought.

The way to accomplish that would be to refactor the decrypt method to have an internal method that returned the timestamp, and then use that + the internal encrypt method.

derwolfe · 2017-10-16T00:45:16Z

The way to accomplish that would be to refactor the decrypt method to have an internal method that returned the timestamp, and then use that + the internal encrypt method.

Didn't think of that! :) This does seem like a safer, more user friendly solution.

reaperhulk · 2017-10-16T01:09:02Z

My preference would be for the default of rotate be ttl preservation. We could entirely remove the ttl arg, but it does seem reasonable for callers who use ttl to rotate and update ttl.

derwolfe · 2017-10-16T01:44:17Z

Sounds good - when I get some time this week, I'll make the changes.

alex · 2017-10-16T01:46:06Z

I actually think removing the ttl arg makes sense - ttl is about decryption, if rotate preserves the original timestamp, it's a "noop" in that sense.

reaperhulk · 2017-10-16T01:57:24Z

@alex it doesn't seem out of the question for a caller to want to rotate the encryption key and update the ttl to a later time.

Of course, to allow ttl alteration and rotation the ttl arg actually needs to have 3 types of values. Preserve, no ttl, and a new ttl. In encrypt we have None as no ttl, but None can't be both no ttl and preserve ttl, so we'd need to do a sentinel or enum to handle that.

If we want to wait for a user to request ttl update we could minimize our API surface area by not supporting ttl update for now. A future PR could add the above with no backwards compatibility concerns.

alex · 2017-10-16T01:59:37Z

There's two distinct things here:

a) Changin the stored timestamp
b) Enforcing the ttl argument against the timestamp.

I don't believe we should do either (and therefore we can drop the ttl argument).

reaperhulk · 2017-10-16T02:13:39Z

I wrote a lot of words about various concerns I have but while doing so have talked myself into agreeing. Yes, since a Fernet token actually only encodes timestamp and the ttl is an arg to decrypt then I agree that rotation should not enforce a ttl.

I'm not convinced that we shouldn't support update_timestamp but in the absence of a user advocate for the feature I'm not going to fight for it.

reaperhulk · 2017-10-16T10:32:18Z

src/cryptography/fernet.py

+        return self._decrypt_data(data, timestamp, ttl)
+
+    @staticmethod
+    def _get_token_data(token):


Since this returns a timestamp that may be invalid (we haven't performed the HMAC verification), let's call this _get_unvalidated_token_data.

(For our purposes it's okay to not have validated it yet as the call to _decrypt_data does the HMAC check)

obviated by updates

reaperhulk · 2017-10-16T12:32:20Z

docs/fernet.rst

@@ -86,7 +86,8 @@ has support for implementing key rotation via :class:`MultiFernet`.
    .. versionadded:: 0.7

    This class implements key rotation for Fernet. It takes a ``list`` of
-    :class:`Fernet` instances, and implements the same API:
+    :class:`Fernet` instances, and implements the same API with the exception


nit: there shouldn't be a comma here (You didn't add it, but let's go ahead and fix it now)

reaperhulk · 2017-10-16T12:40:47Z

docs/fernet.rst

+
+        .. versionadded:: 2.2
+
+        Rotating tokens is necessary when any of the fernet keys of a


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.

@reaperhulk does this work a bit better?

Rotates a token by re-encrypting it under the :class:`MultiFernet` instance's 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 returned. If rotation fails this will raise an exception. Token rotation is a best practice and manner of cryptographic hygiene designed to limit damage in the event of an undetected event and to increase the difficulty of attacks. For example, it would be necessary to rotate all fernet tokens in the event of an employee leaving who previously had access to the fernet key used to encrypt/decrypt fernet tokens.

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 of rotate and into the docs for MultiFernet itself.

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.

reaperhulk · 2017-10-16T12:41:05Z

docs/fernet.rst

+        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


This comma and the one in the next sentence are both superfluous.

reaperhulk · 2017-10-16T12:42:31Z

docs/fernet.rst

+           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


"If a token is invalid this exception is raised" is probably all we need here.

reaperhulk · 2017-10-16T12:42:51Z

docs/spelling_wordlist.txt

@@ -21,6 +21,7 @@ cryptographic
 cryptographically
 Debian
 decrypt
+decrypts


I'd like to register my (continued) displeasure that enchant can't figure out plurals.

reaperhulk · 2017-10-16T12:44:39Z

src/cryptography/fernet.py

@@ -134,6 +141,20 @@ def __init__(self, fernets):
    def encrypt(self, msg):
        return self._fernets[0].encrypt(msg)

+    def rotate(self, msg):
+        timestamp, data = self._fernets[0]._get_unverified_token_data(msg)


Since this is a static method we can just call Fernet._get_unverified_token_data

reaperhulk · 2017-10-16T12:47:40Z

src/cryptography/fernet.py

+        else:
+            raise InvalidToken
+
+        iv = os.urandom(16)


Why does _encrypt_from_parts take an iv? The only other caller also does this os.urandom call. I don't think we want to have two places in our code where we generate the random IV. Maybe we should modify _encrypt_from_parts to just take data and timestamp and hoist iv generation into the method?

For the tests.

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.

reaperhulk · 2017-10-17T02:27:40Z

tests/test_fernet.py

+        mf1_ciphertext = mf1.encrypt(plaintext)
+
+        original_time, _ = Fernet._get_unverified_token_data(mf1_ciphertext)
+        rotated_time, _ = Fernet._get_unverified_token_data(


This test would pass most of the time even if we weren't preserving timestamp since fernet timestamps have second granularity. I think we need to monkeypatch time to test this properly.

… due to calls occuring in less than one second

derwolfe · 2017-10-18T16:23:24Z

tests/test_fernet.py

+        later = datetime.datetime.now() + datetime.timedelta(minutes=1)
+
+        t1 = time.mktime(earlier.timetuple())
+        t2 = time.mktime(later.timetuple())


I only need to patch time once, after encryption. I'll change the PR to do that.

… as part of re-encryption

reaperhulk · 2017-10-18T19:20:07Z

docs/fernet.rst

@@ -109,6 +110,50 @@ 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.

+    Token rotation as offered by :meth:`MultiFernet.rotate` is a best practice


This language still bugs me but I don't have constructive improvement to offer.

derwolfe added 2 commits October 12, 2017 19:45

add rotate method

c14309d

add some more tests for the failure modes

7bb56bb

reaperhulk reviewed Oct 13, 2017

View reviewed changes

start adding some documentation for the rotate method

bf6ec57

alex reviewed Oct 13, 2017

View reviewed changes

operate on a single token at a time, leave lists to the caller

e9e3b80

reaperhulk previously requested changes Oct 13, 2017

View reviewed changes

derwolfe added 3 commits October 13, 2017 10:55

add versionadded

e5983a2

add versionadded, drop rotate from class doctest

give rotate a doctest

6a40caf

single level, not aligned

ea34b0a

add changelog for mf.rotate

fc01194

alex reviewed Oct 14, 2017

View reviewed changes

derwolfe added 2 commits October 15, 2017 09:45

show that, once rotated, the old fernet instance can no longer decryp…

ef0ab94

…t the token

add the instead of just the how

ecf768d

derwolfe added 2 commits October 16, 2017 05:13

update docs to reflect removal of ttl from rotate

331bd36

update tests

dd22738

derwolfe added 5 commits October 16, 2017 05:14

refactor internal methods so that we can extract the timestamp

6ba329b

implement rotate

6aae7d9

update wordlist (case sensitive?)

b4629ee

lints

6bc8daf

consistent naming

414fdb5

reaperhulk reviewed Oct 16, 2017

View reviewed changes

get_token_data/get_unverified_token_data -> better name

5509ef9

reaperhulk reviewed Oct 16, 2017

View reviewed changes

derwolfe added 5 commits October 16, 2017 08:56

doc changes

054d64d

use the static method, do not treat as imethod

5ca5794

move up to MultiFernet docs

a6d2dff

add to authors

cb51edd

alter wording

0ed4ea8

reaperhulk reviewed Oct 17, 2017

View reviewed changes

monkeypatch time to make it less possible for the test to pass simply…

ef70387

… due to calls occuring in less than one second

derwolfe commented Oct 18, 2017

View reviewed changes

set the time after encryption to make sure that the time is preserved…

1b08fcd

… as part of re-encryption

reaperhulk reviewed Oct 18, 2017

View reviewed changes

reaperhulk approved these changes Oct 18, 2017

View reviewed changes

reaperhulk merged commit af6f990 into pyca:master Oct 18, 2017

github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2020


		rotated = mf2.rotate([mf1.encrypt(plaintext)])[0]

		assert rotated != mf1_ciphertext


		.. versionadded:: 2.2

		Rotating tokens is necessary when any of the fernet keys of a

Add Multifernet.rotate method #3979

Add Multifernet.rotate method #3979

Conversation

derwolfe commented Oct 13, 2017

derwolfe commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex commented Oct 13, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex commented Oct 13, 2017

derwolfe commented Oct 13, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex commented Oct 15, 2017

derwolfe commented Oct 15, 2017

alex commented Oct 15, 2017

derwolfe commented Oct 16, 2017

reaperhulk commented Oct 16, 2017

derwolfe commented Oct 16, 2017

alex commented Oct 16, 2017

reaperhulk commented Oct 16, 2017

alex commented Oct 16, 2017

reaperhulk commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment