feat: Add int "aes128gcm" content-type handling to python. by jrconlin · Pull Request #29 · web-push-libs/encrypted-content-encoding

jrconlin · 2016-11-01T23:55:25Z

adds new content-type verison handling
fixes flake8 issues
adds documentation and type descriptions for parameters
moves tests to unit testing
adds ability to do cross library validation (note: node.js does not
use valid ec256p curve points)
prep work for travis integration

martinthomson

The big open question here is to what extent we need to maintain backward compatibility in the API. A version bump should ensure that the new version isn't picked up automatically, but it could be surprising for some. Maybe the right answer is to maintain "aesgcm" as a default and change the default when we go to publication for the RFCs and have that coincide with a 1.0 release.

martinthomson · 2016-11-02T00:02:48Z

-def deriveKey(mode, salt, key=None, dh=None, keyid=None, authSecret=None, padSize=2):
-    def buildInfo(base, context):
-        return b"Content-Encoding: " + base + b"\0" + context
+MAX_BUFFER_SIZE = pow(2, 36) - 31


You can trim this down to 2^32-1. I doubt that anyone using the old stuff will care.

martinthomson · 2016-11-02T00:05:09Z

        nonceinfo = b"Content-Encoding: nonce"
+    elif content_type == "aes128gcm":
+        keyinfo = b"Content-Encoding: aes128gcm\x00\x01"
+        nonceinfo = b"Content-Encoding: nonce\x00\x01"


HKDF should be adding the 0x01 to the end, so you should remove it from here.

martinthomson · 2016-11-02T00:11:15Z

+            "salt": content[:16],  # 16 octets
+            "rs": struct.unpack("!l", content[16:20])[0],  # 4 octets
+            "id_len": id_len,  # 1 octet
+            "key_id": content[21:21 + id_len],  # n octets


nit: # id_len octets

martinthomson · 2016-11-02T00:15:33Z

+        id_len = struct.unpack("!B", content[20])[0]
+        reply = {
+            "salt": content[:16],  # 16 octets
+            "rs": struct.unpack("!l", content[16:20])[0],  # 4 octets


Important: "!L" (and above)

martinthomson · 2016-11-02T00:21:12Z

-    def decryptRecord(key, nonce, counter, buffer):
+
+def decrypt(content, salt, key=None, keyid=None, dh=None, rs=4096,
+            auth_secret=None, pad_size=2, content_type="aesgcm", **kwargs):


content_type might be a bad name

the default for content_type is unfortunate. I would prefer to have this default to the new thing. (My js code does.) Is it too much to ask that existing users include content_type="aesgcm" in their code?

since pad_size is a legacy argument, I would remove it from here and fall back to your code with kwargs

martinthomson · 2016-11-02T00:24:21Z

-        raise Exception(u"Message truncated")
+    # handle old, malformed args
+    pad_size = kwargs.get('padSize', pad_size)
+    auth_secret = kwargs.get('authSecret', auth_secret)


I don't think that you need to include this. I would prefer to leave code that hasn't been updated using the old code. To that end, pad_size can be removed entirely.

martinthomson · 2016-11-02T00:26:08Z

-        "salt": os.urandom(16),
+        "salt": salt,
        "rs": rlen() + 1,
        "authSecret": os.urandom(10)


might pay to underscore this one too

martinthomson · 2016-11-02T00:28:32Z

-                ):
-            log("Test: " + f.__name__)
-            f()
+    for c_type in [ "aes128gcm", "aesgcm", "aesgcm128"]:


I think that I'm going to steal your new argument for my JS code.

martinthomson · 2016-11-02T00:31:59Z

@jrconlin, thanks for picking this up. I think that you have one fairly big error in your code. I will make some changes to the JS code based on this though, it's a definite improvement.

jrconlin · 2016-11-02T02:57:52Z

Well, the guiding principle is "Minor library versions DO NOT BREAK existing code". Since I don't think you're ready for this to go to 1.0 quite yet, I'm setting the default to be "aesgcm", which is the current standard. Once 1.0 is ready to roll, we can switch the default, provide proper docs about what the change is, and give folks a good heads up. That's one of the reasons that I have code to accept both the camel case as well as proper underscore vars, since just changing them will break existing code. (More fun for the CHANGELOG, I suppose.) I'm also probably going to add some static blocks to encrypt/decrypt to the units so that we can verify that we're not stomping on stuff that once worked. If you like, I'll see about putting them in a file so that we can run them on the node side as well.

Thanks for the early review. I'm going to move the stuff in tests to proper unit tests and probably add a few scripts to setup.py. Once things are nailed down, I'll collapse the commits, clean up the comment and assign the PR to you so you can give it a proper yay/nay.

martinthomson · 2016-11-02T03:18:43Z

Awesome, thanks for doing the work. You've much better python than I do, so I appreciate the help.

BTW, you should check that your implementation agrees with mine. I've updated the node.js stuff (in #28) and the command line utilities to match.

Incidentally, I settled on an argument called version instead of content_type.

jrconlin · 2016-11-02T03:25:56Z

👍 You're protocol guy. I'll pound on the code to do and say what you want it to.

martinthomson

This is unconditionally better than what I have. I could quibble about things, but I'm lazy enough to accept this without too much more scrutiny. I will hold this until I get better confirmation that the direction of this stuff is good.

martinthomson · 2016-11-07T09:40:11Z

+
+    (key_, nonce_) = derive_key(mode="decrypt", salt=salt,
+                                key=key, keyid=keyid, dh=dh,
+                                authSecret=auth_secret,


s/authSecret/auth_secret/

Again, thanks!

martinthomson · 2016-11-07T09:42:41Z

    name='http_ece',
-    version='0.5.0',
+    version='0.6.0',
    author='Martin Thomson',


@jrconlin, feel free to add yourself as a contributor here

Thanks. PyPi can get a bit squirrelly if you have multiple authors or emails, so I'll just stick with the participants list on github. Plus, this way you look even better, so bonus on that front.

martinthomson · 2016-11-07T09:44:33Z

-    if salt is None or len(salt) != 16:
-        raise Exception(u"'salt' must be a 16 octet value")
+    if version not in versions:
+        raise ECEException(u"invalid version specified")


I think that you need to move this below the block 97-100

Ah, right. Good catch

martinthomson · 2016-11-07T09:45:10Z

+            version = "aesgcm"
+
+    if salt is None or len(salt) != 16:
+        raise ECEException(u"'salt' must be a 16 octet value")


In the JS code, I just generate a random salt. That's probably nicer API ergonomics.

This is a bit complicated, sadly. Since we have to respect prior versions and function, I can't just gen up a new salt, (We used to throw errors, so we should still do so for older versions.)

Fortunately, in the encrypt() function we do check if salt has not been set, create one, and also force the content type to aes128gcm.

martinthomson · 2016-11-07T09:46:03Z

+    :param keyid: Internal key identifier for private key info
+    :type keyid: str
+    :param dh: Remote Diffie Hellman sequence
+    :type dh: str


In the JS code I read the DH key from the keyid in the header when I'm using DH.

Yep, but we've got to preserve old function when possible. (else we'll have to bump up the major version prematurely)

* adds new content-type verison handling * fixes flake8 issues * adds documentation and type descriptions for parameters * moves tests to unit testing * adds ability to do cross library validation (note: node.js does not use valid ec256p curve points) * prep work for travis integration

jrconlin · 2016-11-07T17:39:39Z

One general "TODO" would be to get travis integration so PRs get tested. I'm not fully versed enough in Travis arcana to know how you get it to test multiple languages of a given project, but I'll see what I can dig up. I need to do the same for one of my own projects.

martinthomson reviewed Nov 2, 2016

View reviewed changes

jrconlin force-pushed the draft/python branch 3 times, most recently from 1fb58c0 to df08f3f Compare November 4, 2016 18:23

jrconlin changed the title ~~wip: Add int "aes128gcm" content-type handling to python.~~ feat: Add int "aes128gcm" content-type handling to python. Nov 4, 2016

martinthomson approved these changes Nov 7, 2016

View reviewed changes

jrconlin force-pushed the draft/python branch from df08f3f to 5483765 Compare November 7, 2016 17:33

Merge branch 'master' into draft/python

fc8cc61

martinthomson merged commit dd5c246 into web-push-libs:master Dec 22, 2016

Conversation

jrconlin commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinthomson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinthomson commented Nov 2, 2016

Uh oh!

jrconlin commented Nov 2, 2016

Uh oh!

martinthomson commented Nov 2, 2016

Uh oh!

jrconlin commented Nov 2, 2016

Uh oh!

martinthomson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrconlin commented Nov 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jrconlin commented Nov 1, 2016 •

edited

Loading