feat: Legacy encrypt (current aries RFC 19), compatible with ACA-Py #292
Conversation
Codecov Report
@@ Coverage Diff @@
## master #292 +/- ##
==========================================
- Coverage 88.75% 88.71% -0.05%
==========================================
Files 39 42 +3
Lines 1975 2126 +151
==========================================
+ Hits 1753 1886 +133
- Misses 122 131 +9
- Partials 100 109 +9
Continue to review full report at Codecov.
|
troyronda
requested review from
sudeshrshetty,
rolsonquadras,
llorllale,
baha-ai,
fqutishat and
bstasyszyn
|
code coverage is low |
| return encodedRecipients, nil | ||
| } | ||
|
|
||
| // Note: the spec is incorrect with the parameters for encrypted_key: the CEK is the message to encrypt |
There was a problem hiding this comment.
Did you raise an issue on aries-rfcs?
There was a problem hiding this comment.
The spec in question is being amended to a JWE format (that @Baha-sk is working on) anyway - perhaps we should request it be split to keep a spec for the legacy format for backwards-compatibility, and amend that.
| recipient1Key, err := randCurveKeyPair(rand.Reader) | ||
| require.NoError(t, err) | ||
|
|
||
| t.Run("Generate a box_seal message to compare to ACA-Py:", func(t *testing.T) { |
There was a problem hiding this comment.
where is the comparison to ACA-Py's output?
There was a problem hiding this comment.
Randomness (for nonces, CEKs) keeps me from generating encrypted envelopes in ACA-Py to compare against my own output. Right now I generate test data which can be pasted into a python test script to verify using ACA-Py decrypt, but once I have decrypt implemented I can create test data in ACA-Py that can be part of test cases here, testing decrypt, and I can also have test cases for my encrypt against my decrypt.
In essence, verification of correctness needs to happen on the decrypt side. I could work on making encrypt take a Reader as parameter for the randomness source, with the wallet being relied on to submit a CSPRNG.
There was a problem hiding this comment.
My above concern should be solved now. You should soon see more tests like what you want to see.
| TODO: use Ed25519 keys, convert to curve25519 internally, encode the Ed25519 form for message packing | ||
| */ | ||
|
|
||
| t.Run("Success test case: given keys, generate envelope", func(t *testing.T) { |
There was a problem hiding this comment.
it would be good to compare the actual result to an expected value
There was a problem hiding this comment.
Now that I can run the crypter using a deterministic rand source, I have a few tests which do this. Got some ideas on more I can do though.
Moopli
force-pushed
the
issue-139
branch
2 times, most recently
from
8e7eca5 to
9522538
Compare
I just pushed fixes to most of what's been brought up, some improvements to test coverage, and I made the crypter deterministic, taking an RNG as a parameter - |
Moopli
changed the title
Moopli
force-pushed
the
issue-139
branch
4 times, most recently
from
d6d121f to
3417002
Compare
Moopli
force-pushed
the
issue-139
branch
2 times, most recently
from
7cb4451 to
31a15a1
Compare
Moopli
force-pushed
the
issue-139
branch
3 times, most recently
from
a50249a to
fadbb35
Compare
Closes hyperledger-archives#139 Signed-off-by: Filip Burlacu <[email protected]>
|
@fqutishat every live code path is now exercised - only future regressions or cosmic rays will make errors happen in the few error cases that aren't tested. |
| return c, nil | ||
| } | ||
|
|
||
| func (c *Crypter) setRandSource(source io.Reader) { |
There was a problem hiding this comment.
It's used in tests, but I can replace it with direct access to the Crypter.
Implements legacy encrypter for compatibility with ACA-Py.
Closes #139
Signed-off-by: Filip Burlacu [email protected]