move fernet creation to helper method #198

terencehonles · 2023-10-07T22:10:46Z

This change moves the fernet creation to a helper method in order to make it easier to either encrypt or decrypt values in the config/cache file. The motivation for this change is to make it easier to debug issues or to re-use OAuth tokens obtained outside the proxy.

terencehonles · 2023-10-07T22:11:46Z

emailproxy.py

+            return ('%s: Invalid `token_salt` value found in config file entry for account %s - this value is not '
+                    'intended to be manually created; please remove and retry' % (APP_NAME, username))


This could also be rewritten to stomp on previous values and just show a warning instead.

This is the post in question that for whatever reason gave that incorrect advice and led to a few invalid issues being opened (and this code being added). Traffic seems to have subsided from that source recently, so yes, replacing the value with a new salt and just showing a warning would probably be fine. So, something like:

# we hash locally-stored tokens with the given password token_salt = config.get(username, 'token_salt', fallback=None) # generate encrypter/decrypter based on password and random salt try: decoded_salt = base64.b64decode(token_salt.encode('utf-8')) # catch incorrect third-party proxy guide except binascii.Error: Log.info('%s: Invalid `token_salt` value found in config file entry for account %s - this value is not ' 'intended to be manually created; please remove and retry' % (APP_NAME, username)) token_salt = None if not token_salt: token_salt = base64.b64encode(os.urandom(16)).decode('utf-8') config.set(username, 'token_salt', token_salt)

Ok, updating the PR to generate the salt on error

simonrob

Thanks for the PR. I don't have strong feelings either way about this, so it'd be useful if you could give a bit more detail about how it makes things easier. Is that because you'd rewrite or monkey-patch this function to change its behaviour? And how would it help with debugging issues or reusing externally-obtained OAuth tokens?

emailproxy.py

simonrob · 2023-10-11T19:58:35Z

emailproxy.py

+        # check if there was an error getting the encryption info
+        if not isinstance(fernet, Fernet):
+            return (False, fernet)


Can this ever actually happen?

This was just to catch the old invalid salt and continue exiting like the previous behavior

simonrob · 2023-10-11T20:07:10Z

emailproxy.py

+            return ('%s: Invalid `token_salt` value found in config file entry for account %s - this value is not '
+                    'intended to be manually created; please remove and retry' % (APP_NAME, username))


This is the post in question that for whatever reason gave that incorrect advice and led to a few invalid issues being opened (and this code being added). Traffic seems to have subsided from that source recently, so yes, replacing the value with a new salt and just showing a warning would probably be fine. So, something like:

# we hash locally-stored tokens with the given password token_salt = config.get(username, 'token_salt', fallback=None) # generate encrypter/decrypter based on password and random salt try: decoded_salt = base64.b64decode(token_salt.encode('utf-8')) # catch incorrect third-party proxy guide except binascii.Error: Log.info('%s: Invalid `token_salt` value found in config file entry for account %s - this value is not ' 'intended to be manually created; please remove and retry' % (APP_NAME, username)) token_salt = None if not token_salt: token_salt = base64.b64encode(os.urandom(16)).decode('utf-8') config.set(username, 'token_salt', token_salt)

This change moves the fernet creation to a helper method in order to make it easier to either encrypt or decrypt values in the config/cache file. The motivation for this change is to make it easier to debug issues or to re-use OAuth tokens obtained outside the proxy.

terencehonles

Thanks for the PR. I don't have strong feelings either way about this, so it'd be useful if you could give a bit more detail about how it makes things easier. Is that because you'd rewrite or monkey-patch this function to change its behaviour? And how would it help with debugging issues or reusing externally-obtained OAuth tokens?

It just makes it easier since the logic is encapsulated in one place and it makes it easier (possible) to run something like:

>>> import emailproxy, getpass
>>> config = emailproxy.AppConfig.get()
>>> username = input('Username: ')
Username: example@example.com
>>> cryptographer = emailproxy.OAuth2Helper.get_encryption_info(config, username, getpass.getpass())[0]
Password: 
>>> print("Refresh token: ", emailproxy.OAuth2Helper.decrypt(cryptographer, config.get(username, 'refresh_token')))
...

It's also possible to write new tokens following the same pattern as ^^^.

I wouldn't expect someone to monkey patch the function, but it's true that it does allow a way to potentially change the encryption method, though that would mean that OAuth2Helper.encrypt and OAuth2Helper.decrypt would also need to be updated and if making the encryption method be configurable is a goal then it probably makes sense to move everything to a class and create an instance that has a single interface.

terencehonles · 2023-10-15T08:24:57Z

emailproxy.py

+            return ('%s: Invalid `token_salt` value found in config file entry for account %s - this value is not '
+                    'intended to be manually created; please remove and retry' % (APP_NAME, username))


Ok, updating the PR to generate the salt on error

terencehonles · 2023-10-15T08:25:43Z

emailproxy.py

+        # check if there was an error getting the encryption info
+        if not isinstance(fernet, Fernet):
+            return (False, fernet)


This was just to catch the old invalid salt and continue exiting like the previous behavior

emailproxy.py

terencehonles · 2023-10-15T10:03:32Z

emailproxy.py

+            token_salt = base64.b64encode(decoded_salt).decode('ascii')
+
+        # generate encrypter/decrypter based on password and random salt
+        key_derivation_function = PBKDF2HMAC(algorithm=hashes.SHA256(), length=32, salt=decoded_salt, iterations=100000,


It looks like this is a bit low based on current recommendations. I believe this is not something that can be just updated and two different fernets probably need to be created and the old one can be used as a fallback when decrypting. A change like this is out of scope of this PR, and would require confirming a migration would be necessary.

Yes – this is something I looked at improving a while ago, but it was hard to find a good way to migrate. Since it's a locally-stored encrypted value I felt the risk was quite low, but if you can suggest a way to do this that isn't onerous from the user side I'm happy to look at it.

It looks like it might be relatively straightforward to do with a https://cryptography.io/en/latest/fernet/#cryptography.fernet.MultiFernet but you'd want to probably store the number of iterations used so that way you could run the migration and re-save the config when needed.

terencehonles · 2023-10-16T11:15:51Z

The added commit should address #198 (comment), but I left it as a separate commit in case the change should be backed out from this PR.

requirements-no-gui.txt

simonrob

I was just about to take a look at iteration upgrading via MultiFernet, but saw this. Thanks for the great work!

simonrob · 2023-10-16T19:42:45Z

emailproxy.py

@@ -570,6 +570,79 @@ def _save_cache(cache_store_identifier, output_config_parser):
            Log.error('Error saving state to cache store file at', cache_store_identifier, '- is the file writable?')


+class Cryptographer:
+    ITERATIONS = 870000  # taken from cryptography's suggestion of using Django's defaults
+    LEGACY_ITERATIONS = 100000


What do you think about keeping a list of previous iteration counts given that the recommendation changes fairly often? (e.g., from 480,000 in December 2022 to 870,000 under a year later)

Well this change stores the number of iterations in the config, so right now the options it tries are: the initial default 100_000, if there's a value in the config, and the new default 870_000, going forward if the config is updated it will store the value in the config, so it should really never go past that and the legacy value is just for historical reasons.

Ideally it's just trying [ITERATIONS, configured_iterations] and if the config is up to date that's actually just [ITERATIONS] when uniqued.

I guess I was thinking about a case somewhat related to your other comment in which, for example, someone upgrades the script to a version that adjusts the primary iterations value, then changes the cached iterations value for an account before they've logged in, perhaps thinking this will increase security (when in actual fact that would cause a decryption failure, and they should instead just log in to the account to trigger rotation). But that's a pretty long shot, and they could also just re-authorise the account regardless. So on balance I think the current approach is probably fine, and ITERATIONS can just be updated as and when the recommendation changes.

As an aside, since the proxy requires Python 3.6 or later, and underscore notation was added in that version (via PEP 515) I think we might as well use the clearer values of 870_000 and 100_000 directly here.

emailproxy.py

simonrob · 2023-10-16T19:45:38Z

emailproxy.py

+        token_salt = config.get(username, 'token_salt', fallback=None)
+        if token_salt:
+            try:
+                self._salt = base64.b64decode(token_salt.encode('ascii'))  # catch incorrect third-party proxy guide


What's the reason for switching to ascii here?

The base64 alphabet is always going to be ascii so if it doesn't encode then the decoding is not going to work. If for some reason (unlikely) the config returns unicode that can't be encoded as 'utf-8' this would have still broken before.

simonrob · 2023-10-16T19:45:48Z

emailproxy.py

+
+    @property
+    def salt(self):
+        return base64.b64encode(self._salt).decode('ascii')


As above – what's the reason for switching to ascii here?

This one is guaranteed to always work since the base 64 alphabet is ascii

simonrob · 2023-10-16T19:48:37Z

emailproxy.py

+            iterations = self.LEGACY_ITERATIONS
+
+        # the first fernet is the primary fernet so sort the iterations count descending
+        self._iterations_options = sorted({self.ITERATIONS, iterations, self.LEGACY_ITERATIONS}, reverse=True)


I'm curious – I'd always do self.ITERATIONS + iterations + self.LEGACY_ITERATIONS here. What's the reason for {self.ITERATIONS, iterations, self.LEGACY_ITERATIONS}?

since it's a set it will drop out any duplicates since iterations should be self.ITERATIONS

Sorry, I had in my head that these values were lists after making the first comment above (and hence this would not work). Ignore this one!

emailproxy.py

terencehonles · 2023-10-17T10:41:42Z

One thing that may not be obvious is that you can actually set the token_iteration in the config to a value larger than what the script provides. As is, as long as the original iterations value is either Cryptographer.ITERATIONS or Cryptographer.LEGACY_ITERATIONS then they will be tried as fallbacks and the value in the config would be re-written since it would be the primary fernet.

This could support a previous or future iterations value by allowing the Cryptographer to have a fallback_iterations/new_iterations passed to its constructor, and one could rotate the config manually but even that could be moved to its own function to allow a user to easily force higher values and move between them. This change seems a bit further out of scope of this original PR, but shows some future opportunities for improvement.

simonrob · 2023-10-17T16:11:47Z

I edited a few comments and removed an unnecessary ValueError check (if fallback is specified this doesn't happen). I think this is good to go?

#198)" This reverts commit 1c067dd.

terencehonles commented Oct 7, 2023

View reviewed changes

simonrob requested changes Oct 11, 2023

View reviewed changes

terencehonles force-pushed the add-cryptographer-helper-method branch from 2fdde19 to 4ed058e Compare October 15, 2023 09:24

terencehonles commented Oct 15, 2023

View reviewed changes

terencehonles force-pushed the add-cryptographer-helper-method branch 2 times, most recently from f39767e to f582366 Compare October 16, 2023 11:20

update cryptographer to support rotating secrets

2dc9715

terencehonles force-pushed the add-cryptographer-helper-method branch from f582366 to 2dc9715 Compare October 16, 2023 11:21

terencehonles commented Oct 16, 2023

View reviewed changes

requirements-no-gui.txt Show resolved Hide resolved

simonrob reviewed Oct 16, 2023

View reviewed changes

suggestions

490a355

terencehonles commented Oct 17, 2023

View reviewed changes

emailproxy.py Outdated Show resolved Hide resolved

Update emailproxy.py

c3f8b5b

Minor comment edits; version bump and formatting

c241e35

simonrob approved these changes Oct 17, 2023

View reviewed changes

Merge branch 'main' into add-cryptographer-helper-method

7fb24e6

simonrob merged commit 1c067dd into simonrob:main Oct 22, 2023

simonrob added a commit that referenced this pull request Oct 22, 2023

Revert "Move fernet creation to helper method; support secret rotation (

742f909

#198)" This reverts commit 1c067dd.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move fernet creation to helper method #198

move fernet creation to helper method #198

terencehonles commented Oct 7, 2023

terencehonles Oct 7, 2023

simonrob Oct 11, 2023

terencehonles Oct 15, 2023

simonrob left a comment

simonrob Oct 11, 2023

terencehonles Oct 15, 2023

simonrob Oct 11, 2023

terencehonles left a comment •

edited

Loading

terencehonles Oct 15, 2023

terencehonles Oct 15, 2023

terencehonles Oct 15, 2023

simonrob Oct 15, 2023

terencehonles Oct 16, 2023

terencehonles commented Oct 16, 2023

simonrob left a comment

simonrob Oct 16, 2023

terencehonles Oct 17, 2023

simonrob Oct 17, 2023

simonrob Oct 16, 2023

terencehonles Oct 17, 2023

simonrob Oct 16, 2023

terencehonles Oct 17, 2023

simonrob Oct 16, 2023

terencehonles Oct 17, 2023

simonrob Oct 17, 2023

terencehonles commented Oct 17, 2023 •

edited

Loading

simonrob commented Oct 17, 2023

		return ('%s: Invalid `token_salt` value found in config file entry for account %s - this value is not '
		'intended to be manually created; please remove and retry' % (APP_NAME, username))

move fernet creation to helper method #198

move fernet creation to helper method #198

Conversation

terencehonles commented Oct 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonrob left a 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

terencehonles left a comment • edited Loading

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

terencehonles commented Oct 16, 2023

simonrob left a 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

terencehonles commented Oct 17, 2023 • edited Loading

simonrob commented Oct 17, 2023

terencehonles left a comment •

edited

Loading

terencehonles commented Oct 17, 2023 •

edited

Loading