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

Add encrypt-then-MAC using HMAC and the same encryption key #69

Merged
merged 14 commits into from
May 8, 2016

Conversation

fdemmer
Copy link
Contributor

@fdemmer fdemmer commented Apr 21, 2016

Maybe a step towards key rotation (#22) is detecting valid key before decrypting.

In any case, i was annoyed by the cryptic (little pun intended :)) DjangoUnicodeDecodeError that resulted from having the wrong key and was looking to actually detect valid credentials before attempting to decrypt. So i added a HMAC (default, md5) signature between $AES$ and the cyphertext. Now invalid key raises a SignatureException.

This still works with unsigned database values, as it simply checks for the number of items when splitting the value at the $. I also ripped out the encryption stuff into its own class, which has a sign attribute to get back the old behavior (incl the DjangoUnicodeDecodeError).

from hmac import compare_digest
return compare_digest(a, b)
except ImportError:
return a == b
Copy link
Owner

Choose a reason for hiding this comment

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

django.utils.crypto has a constant_time_compare for this same backport.

@mlavin
Copy link
Owner

mlavin commented Apr 21, 2016

I like the direction of this change though it has some issues on Python 3. I think #22 will most likely be address (if I can ever find the time) by moving to https://github.com/orcasgit/django-fernet-fields which uses the MultiFernet to handle the rotation. This is a nice intermediate step.

@codecov-io
Copy link

Current coverage is 92.21%

Merging #69 into master will decrease coverage by -1.33% as of d6906c7

@@            master     #69   diff @@
======================================
  Files           11      11       
  Stmts          403     437    +34
  Branches        32      35     +3
  Methods          0       0       
======================================
+ Hit            377     403    +26
- Partial         10      13     +3
- Missed          16      21     +5

Review entire Coverage Diff as of d6906c7

Powered by Codecov. Updated on successful CI builds.

@mlavin
Copy link
Owner

mlavin commented May 8, 2016

Sorry it took me awhile to get back to this. Thank you for contributing! ❤️

@mlavin mlavin merged commit 299a1f7 into mlavin:master May 8, 2016
@fdemmer fdemmer deleted the sign-db-values branch May 10, 2016 21:18
@fdemmer
Copy link
Contributor Author

fdemmer commented Oct 31, 2016

@mlavin thanks for merging. do you have a new release planned with this in current master?

@mlavin
Copy link
Owner

mlavin commented Nov 12, 2016

I pushed out a release today. Thank you again for this contribution and your patience!

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.

3 participants