Authcrypt Decrypt Using (X)Chacha20Poly1035 #230
Conversation
Codecov Report
@@ Coverage Diff @@
## master #230 +/- ##
==========================================
+ Coverage 87.34% 87.36% +0.02%
==========================================
Files 36 36
Lines 1643 1726 +83
==========================================
+ Hits 1435 1508 +73
- Misses 115 120 +5
- Partials 93 98 +5
Continue to review full report at Codecov.
|
|
@Baha-sk Test coverage looks low? |
baha-ai
force-pushed
the
issue-122
branch
2 times, most recently
from
4461412 to
3358c03
Compare
baha-ai
force-pushed
the
issue-122
branch
3 times, most recently
from
f2e2f5d to
884e84e
Compare
@troyronda coverage is now increased |
| require.NoError(t, err) | ||
| require.NotEmpty(t, dec) | ||
|
|
||
| // TODO fix try to decrypt the reference payload |
There was a problem hiding this comment.
How do we know this PR is working?
There was a problem hiding this comment.
working on php interoperable decryption/encryption..
There was a problem hiding this comment.
Why is this line commented out then?
There was a problem hiding this comment.
encryption/decryption fully works in Go.. testing with a JWE generated in another language (PHP) is still failing, but doesn't block Pack/Unpack for now.. so we can go ahead and merge this PR and a new PR will deal with interoperability updates.
|
As discussed, please remove senderKey and recipient public keys from New(). You don't need to specify sender key if you are only using Decrypt() function. |
I will refactor to extract keys out of New() and move them to Encrypt()/Decrypt() accordingly. |
| pld := []byte("lorem ipsum dolor sit amet") | ||
| enc, e := crypter.Encrypt(pld) | ||
| require.NoError(t, e) | ||
| require.NotEmpty(t, enc) |
There was a problem hiding this comment.
I assume the correctness check isn't being done due to a non-deterministic nonce?
Seems like the testing should be setting deterministic values and comparing actual results. !Empty checks seem insufficient.
There was a problem hiding this comment.
yes, probably by hardcoding values into RandReader for tests .. I'll see how this can be done..
baha-ai
force-pushed
the
issue-122
branch
2 times, most recently
from
098e039 to
335fe65
Compare
| @@ -31,17 +30,27 @@ const XC20P = ContentEncryption("XC20P") // XChacha20 encryption + Poly1035 auth | |||
| //nolint:gochecknoglobals | |||
| var randReader = rand.Reader | |||
|
|
|||
| // ErrEmptyRecipients is used when recipients list is empty | |||
| var ErrEmptyRecipients = errors.New("empty recipients") | |||
There was a problem hiding this comment.
I think you should only create exported Error values for things you expect the consumer of the library to check. i.e., expected errors.
Are you suggesting implementations need to check for these specific errors? Most don't seem like runtime issues... If they aren't expected to be checked, then don't export the errors.
Even more explicitly, if you don't expect someone to call errors.Is then there is no need for this error value to show up in the API.
There was a problem hiding this comment.
The user of the Crypter can miss adding recipients when calling Encrypt/Decrypt... but this will probably be internal (used by the wallet API).. I can un export these error types
Using RawURLEncoding for Base64 encoding to allow interoperability with non Go agents Signed-off-by: Baha Shaaban <[email protected]>
Closes #122