WIP: Add DSSE Envelope #1
Conversation
DSSE Envelope is a specification for signing arbitrary data. Envelope contains from_dict and to_dict methods to convert the object to json/dict and vice versa. Signed-off-by: Pradyumna Krishna <[email protected]>
|
This looks great! Is there anything you want to add, before I take a look? Otherwise, I suggest to un-draft the PR. Also, in our conversation earlier today, you mentioned an alternative design that uses data classes. Do you want comments on that too? |
helper functions related to base64 is now moved to util.py Signed-off-by: Pradyumna Krishna <[email protected]>
PradyumnaKrishna
force-pushed
the
dsse_envelope
branch
from
ee0ccb6 to
bdb70d2
Compare
Actually I think |
There was a problem hiding this comment.
Impeccable work, @PradyumnaKrishna! 🎉 I only left a few very minor style comments inline, but otherwise this looks great to me.
Regarding style, would you mind running black, isort and pylint locally over any new modules you add until we fix secure-systems-lab#243?
see secure-systems-lab#243 (comment) for details
Also some test would be great. But we can do that in a follow-up PR.
|
|
||
| <Return> | ||
| base64 string | ||
| """ |
There was a problem hiding this comment.
I appreciate that you adopt the existing code style here (wrt indentation), but I'd say it's okay to deviate from the rest of the file when it comes to docs. Could you please use the same docstring style as above in metadata.py?
There was a problem hiding this comment.
Actually I was following the docstring used by signer.py. Do I have to add all the fields like Purpose, Arguments, Exceptions, Side Effects and Returns in class methods because many functions don't have any arguments or exceptions. or have to use <> for specifying field in docs.
There was a problem hiding this comment.
Our code style guide has some notes about docstrings.
TLDR: In new functions/methods/classes, we should use the Google docstring-style, even if the new code is added to an existing module that otherwise uses the old docstring style, which you used here.
Context: The advantage of the Google docstring-style is that there is a Sphinx-parser that can convert it for Readthedocs. And that it is generally more compact, e.g. the <Purpose> header is omitted because it is implicit and empty sections are also omitted (often times this is the <Side Effects> section).
Please take a quick look at the style guide linked above and let me know if you have more questions.
No, it's okay to use 4 in |
Run linters like black, isort, pylint and fix all errors. Update some doc strings. Change attribute "payload_type" to snake case. Signed-off-by: Pradyumna Krishna <[email protected]>
Pre-Auth-Encoding method returns a byte sequence required to sign the payload and create DSSE signatures. Signed-off-by: Pradyumna Krishna <[email protected]>
|
By the end of this week I will try to add tests for the implemented methods and then it will be good to merge. Afterwards I will look into sign and verification of |
DSSE envelope tests are added which included test cases like * test for from and to dict methods of envelope * test to check envelope equality * and test to check Pre-Auth-Encoding of envelope Signed-off-by: Pradyumna Krishna <[email protected]>
tests/test_metadata.py
Outdated
| """Test envelope to_dict and from_dict methods""" | ||
|
|
||
| # create envelope object from its dict. | ||
| envelope_obj = Envelope.from_dict(copy.deepcopy(self.envelope_dict)) |
There was a problem hiding this comment.
envelope_dict contains a nested dict and these tests changes the signature, so I have to use deepcopy.
I just enabled them for the fork. Not sure how to trigger the workflow for the existing PR. Maybe pushing a new commit does it? |
I think reopening this PR would work, let me try. |
There was a problem hiding this comment.
Excellent PR and I really appreciate the commit discipline! I'm inclined to just merge it, but maybe you can still change the docstring style for the base64 function (see inline comment)?
Regrading pre-authentication encoding, you will likely also need a deserialization function. You can add it in a follow-up PR. But maybe it makes sense to already spend a thought on its name and whether pae will then still make sense for the serialization function implemented here.
|
|
||
| <Return> | ||
| base64 string | ||
| """ |
There was a problem hiding this comment.
Our code style guide has some notes about docstrings.
TLDR: In new functions/methods/classes, we should use the Google docstring-style, even if the new code is added to an existing module that otherwise uses the old docstring style, which you used here.
Context: The advantage of the Google docstring-style is that there is a Sphinx-parser that can convert it for Readthedocs. And that it is generally more compact, e.g. the <Purpose> header is omitted because it is implicit and empty sections are also omitted (often times this is the <Side Effects> section).
Please take a quick look at the style guide linked above and let me know if you have more questions.
Minor changes includes * Improve test case of from_dict. * Convert PreAuthEncoding to property. * Use of TypeError instead of FormatError. Signed-off-by: Pradyumna Krishna <[email protected]>
Signed-off-by: Pradyumna Krishna <[email protected]>
Fixes: secure-systems-lab#370
Description of the changes being introduced by the pull request:
Pull Request for DSSE Envelope.
Envelopehaving attributespayload,payloadType, andsignatures.metadata.pyorEnvelopePlease verify and check that the pull request fulfils the following requirements: