Improve cookie crypto for CookieDealer #373
Conversation
|
Changes Unknown when pulling d0a2e58 on schlenk:cookie_crypto into ** on OpenIDC:master**. |
|
LGTM apart from the import sorting :-/ |
1 similar comment
|
Seems isort messed up the line endings..., need to fix that again. |
Added per cookie IV's to the CookieDealers encryption handling. This fixes CZ-NIC#363. Also restyled the encrypt and MAC construction for cookie security to use a more modern AEAD approach. In this case it is AES-SIV (RFC 5297), which has the nice property to be a bit resistant to IV reuse.
2 similar comments
src/oic/utils/aes.py
Outdated
| """ | ||
| Authenticated Encryption with Associated Data Wrapper | ||
|
|
||
| This does encrypts and does an integrity check in one |
There was a problem hiding this comment.
%s/does encrypts and does an/does encryption and an/g
|
|
||
| """ | ||
| def __init__(self, key, iv, mode=AES.MODE_SIV): | ||
| assert isinstance(key, binary_type) |
There was a problem hiding this comment.
Should we not raise an ImproperlyConfigured error here for consistency?
There was a problem hiding this comment.
This assert is mostly there to help with PY2/PY3 issues so keys/ivs etc stay 'byte strings'.
The configuration mistakes possible should be handled higher up, down here it is a programming error.
src/oic/utils/http_util.py
Outdated
| bytes_load = load.encode("utf-8") | ||
| bytes_timestamp = timestamp.encode("utf-8") | ||
|
|
||
| # If we have an encryption key, we use an AEAD cipher instead of |
There was a problem hiding this comment.
I would be worried that the comments will not match the implementation if changes come. Can you make a more higher level description of the function in the docstring?
tests/test_aes.py
Outdated
|
|
||
|
|
||
| def test_AEAD_good(): | ||
| key = os.urandom(32) |
There was a problem hiding this comment.
Do you want to take advantage of our new and shiny conftest.py and add what you can (seed, iv, cleartext) as fixtures?
There was a problem hiding this comment.
Will have a look if it fits.
There was a problem hiding this comment.
I don't think its useful in the main conftest.py file, but i moved them to local fixtures in the test file.
There was a problem hiding this comment.
Sure, fine for now. If we need them, they can move.
2 similar comments
|
@lwm I fixed the docstrings a bit, so maybe its good to go now. |
👍 |
Added per cookie IV's to the CookieDealers encryption handling.
This fixes #363.
Also restyled the encrypt and MAC construction for cookie security to
use a more modern AEAD approach.
In this case it is AES-SIV (RFC 5297), which has the nice property to
be a bit resistant to IV reuse.