refactor: Crypter/Wallet crypto operations #459

baha-ai · 2019-10-09T19:37:56Z

The current wallet is calling Pack/Unpack
This behavior has now changed and delegated to a
newer Packager interface

In addition, operations involving private keys have been
moved to the wallet. Namely, DerviveKEK() is the only function
in the crypto functionality that deals with private keys

Aries framework has been updated with new context functions
to support the new structure. In addtion to WithWallet(),
2 new function are added to the context provider:
WithCrypter() and WithPackager().

JWE Crypto functionality depends on these providers in
the following order:
1. WithWallet()
2. WithCrypter()
3. WithPackager()

Finally the recipient key is no longer needed in Crypter.Decrypt()
as it will be matched with the recipients's list in the envelope
and the wallet's first key found.

closes #385
Signed-off-by: Baha Shaaban [email protected]

codecov · 2019-10-09T19:38:32Z

Codecov Report

Merging #459 into master will decrease coverage by 0.3%.
The diff coverage is 88.62%.

@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
- Coverage   90.24%   89.93%   -0.31%     
==========================================
  Files          65       66       +1     
  Lines        3445     3489      +44     
==========================================
+ Hits         3109     3138      +29     
- Misses        180      193      +13     
- Partials      156      158       +2

Impacted Files	Coverage Δ
pkg/didcomm/crypto/jwe/authcrypt/common.go	`100% <ø> (+6.89%)`	⬆️
pkg/wallet/api.go	`100% <ø> (ø)`	⬆️
pkg/framework/aries/default.go	`92.59% <100%> (+0.92%)`	⬆️
pkg/didcomm/crypto/jwe/authcrypt/encrypt_jwk.go	`80.32% <100%> (ø)`	⬆️
pkg/didcomm/crypto/jwe/authcrypt/decrypt.go	`87.5% <100%> (+2.36%)`	⬆️
pkg/didcomm/crypto/legacy/authcrypt/decrypt.go	`94.05% <100%> (ø)`	⬆️
pkg/didcomm/crypto/jwe/authcrypt/authcrypt.go	`100% <100%> (ø)`	⬆️
pkg/didcomm/crypto/legacy/authcrypt/encrypt.go	`92.85% <100%> (ø)`	⬆️
pkg/didcomm/transport/http/inbound.go	`80.64% <100%> (ø)`	⬆️
pkg/didcomm/crypto/jwe/authcrypt/decrypt_jwk.go	`75.75% <100%> (-13.14%)`	⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d0e99a...eac8a67. Read the comment docs.

sandrask · 2019-10-09T20:13:32Z

pkg/didcomm/crypto/jwe/authcrypt/encrypt.go

-		return nil, fmt.Errorf("failed to encrypt message: %w", err)
+func (c *BaseCrypter) Encrypt(payload, senderKey []byte, recipients [][]byte) ([]byte, error) { //nolint:funlen
+	if len(senderKey) == 0 {
+		return nil, fmt.Errorf("failed to encrypt message")


error message should include something like empty sender key

will update...

sandrask · 2019-10-09T20:30:42Z

pkg/wallet/api.go

+	// returns:
+	// 		kek []byte
+	//		error in case of errors
+	DeriveKEK(alg, apu []byte, fromKey, toPubKey *[chacha.KeySize]byte, isFromKeyPriv bool) ([]byte, error)


Is there a case where private key would be provided here? I am not clear why we need to expose isFromKeyPriv flag here.

yes, when deriving a KEK from an ephemeral key created in the crypter, in this case fromKey will be a private key.. this is why we need isFromKeyPriv flag

but deriving a KEK for a CEK doesn't require an ephemeral key, and therefore fromKey will be public to tell the function to load its corresponding private key from the wallet

I can probably hide this function and expose 2 functions in the interface:
DeriveKEKUsingPrivKeyFromWallet() which will call DeriveKEK with isFromKeyPriv= false
DeriveKEKUsingEphemeralPrivKey() which will call DeriveKEK with isFromKeyPriv= true

this way isFromKeyPriv flag is hidden from the interface user.

Agree?

Does it make more sense to create utility function DeriveKEK that will work with private key? In that case we can have have only one method in the wallet that will retrieve private key and call utility function DeriveKEK. And for ephemeral keys since you already have private key you just call utility function DeriveKEK. I am not sure where to put this utility function though.

I'll move the util function to util.go under wallet package.. will push soon

member DeriveKEK function takes fromKey as public key and calls a util exported function with the same name that does the real derivation using a private key.

done

pkg/wallet/api.go

pkg/didcomm/crypto/jwe/authcrypt/authcrypt.go

llorllale · 2019-10-11T12:07:04Z

pkg/didcomm/envelope/api.go

+// Envelope contain msg, FromVerKey and ToVerKeys
+type Envelope struct {
+	Message    []byte
+	FromVerKey string


"From" and "To" are relative to the direction the envelope is going (inbound vs outbound) and might get confusing. I suggest we keep it simple:

FromVerKey -> MyVerKey

ToVerKeys -> TheirVerKeys

I don't see how MyVerKey is clearer than FromVerKey?

This will be more confusing from the agent's perspective since MyVerKey for a receiving agent is going to be different from the sending agent's while FromVerKey is clear to be the sender's agent key, no matter who is using the Envelope (From will always be source agent).

Again, this code was moved from wallet into envelope, it's not new code.

Envelope is exported for reasons I don't understand.

If all the complexity of handling Envelope structs is contained within the packing/unpacking functionality then it's okay. But if Envelope starts percolating into the application layer where code would have to first determine their role (sender vs receiver) before picking the right attribute ("from" vs "to") - that would be an unnecessary burden imposed on the application layer.

pkg/didcomm/envelope/packager.go

llorllale · 2019-10-11T12:23:02Z

Separating packaging concerns (envelope.Packager) from encryption (crypto.Crypter) seems like a good idea. However, this PR leaves Crypter with knowledge that, I think, belongs to the Packager. The Crypter still knows stuff about envelopes. It's doing the actual packaging of the envelopes.

Moopli · 2019-10-11T13:59:53Z

@llorllale We've pulled the secret-touching out of the crypter and into the wallet - my suggestion to @Baha-sk was to rename the Crypters to Packagers, now that they're the envelope builders. They still do a bit of crypto, but that's always going to be integral to envelope building/parsing so I think it's reasonable.

llorllale · 2019-10-11T14:28:31Z

@Moopli as discussed in today's morning call, it's probably better to separate the concerns of packaging vs. encryption:

Support at least two envelope formats (legacy and the new one)
Support different alg and enc

baha-ai · 2019-10-11T15:58:40Z

@Moopli as discussed in today's morning call, it's probably better to separate the concerns of packaging vs. encryption:

Support at least two envelope formats (legacy and the new one)

Support different alg and enc

this will be done as part of a separate PR

Moopli · 2019-10-11T15:59:55Z

@llorllale @Baha-sk I opened #475

Moopli

👍

pkg/wallet/api.go

troyronda · 2019-10-11T19:08:14Z

pkg/wallet/utils.go

+}
+
+// Derive25519KEK is a utility function that will derive an ephemeral symmetric key (kek) using fromPrivKey and toPubKey
+func Derive25519KEK(alg, apu []byte, fromPrivKey, toPubKey *[chacha.KeySize]byte) ([]byte, error) { // nolint:lll


Is this used outside the package?

It's used in the Crypter - DeriveKEK was split into a util function because the crypter needs to be able to pass an ephemeral private key in at one point.

It still seems odd that the generic wallet package has a special 25519 method.

I will move utils.go to pkg/internal/cryptoutil to separate the utility logic from the wallet.. in the end, they are internal to our go framework and should not be exposed in the wallet.

done... moved utils.go to pkg/internal/cryptoutil package and updated all its references.

The current wallet is calling Pack/Unpack This behavior has now changed and delegated to a newer Packager interface In addition, operations involving private keys have been moved to the wallet. Namely, DerviveKEK() is the only function in the crypto functionality that deals with private keys Aries framework has been updated with new context functions to support the new structure. In addtion to WithWallet(), 2 new functions are added to the context provider: WithCrypter() and WithPackager(). JWE Crypto functionality depends on these providers in the following order: 1. WithWallet() 2. WithCrypter() 3. WithPackager() Also, minor refactoring of function names in the crypter was done for better readability. Finally the recipient key is no longer needed in Crypter.Decrypt() as it will be matched with the recipients's list in the envelope and the wallet's first key found. Signed-off-by: Baha Shaaban <[email protected]>

Moopli

👍

baha-ai changed the title ~~feat: refactor Crypter/Wallet crypto operations~~ refactor: Crypter/Wallet crypto operations Oct 9, 2019

troyronda requested review from Moopli, sandrask and llorllale October 9, 2019 19:42

sandrask reviewed Oct 9, 2019

View reviewed changes

baha-ai force-pushed the issue-385 branch from 10d37af to 393589c Compare October 10, 2019 22:11

troyronda reviewed Oct 10, 2019

View reviewed changes

pkg/wallet/api.go Outdated Show resolved Hide resolved

troyronda reviewed Oct 10, 2019

View reviewed changes

pkg/didcomm/crypto/jwe/authcrypt/authcrypt.go Outdated Show resolved Hide resolved

llorllale reviewed Oct 11, 2019

View reviewed changes

pkg/didcomm/envelope/packager.go Show resolved Hide resolved

Moopli mentioned this pull request Oct 11, 2019

Base Wallet Using only first recipient key for unpack #468

Closed

Moopli approved these changes Oct 11, 2019

View reviewed changes

pkg/wallet/api.go Outdated Show resolved Hide resolved

sandrask approved these changes Oct 11, 2019

View reviewed changes

baha-ai force-pushed the issue-385 branch from 393589c to cbe209a Compare October 11, 2019 18:56

Moopli approved these changes Oct 11, 2019

View reviewed changes

baha-ai force-pushed the issue-385 branch from cbe209a to 7d90089 Compare October 11, 2019 19:04

troyronda reviewed Oct 11, 2019

View reviewed changes

baha-ai force-pushed the issue-385 branch from 7d90089 to eac8a67 Compare October 11, 2019 19:37

Moopli approved these changes Oct 11, 2019

View reviewed changes

troyronda approved these changes Oct 11, 2019

View reviewed changes

troyronda merged commit 1cca97f into hyperledger-archives:master Oct 11, 2019

Moopli mentioned this pull request Oct 11, 2019

Move Pack/Unpack out of the Wallet #351

Closed

baha-ai deleted the issue-385 branch October 15, 2019 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Crypter/Wallet crypto operations #459

refactor: Crypter/Wallet crypto operations #459

baha-ai commented Oct 9, 2019 •

edited

Loading

codecov bot commented Oct 9, 2019 •

edited

Loading

sandrask Oct 9, 2019

baha-ai Oct 9, 2019

baha-ai Oct 10, 2019

sandrask Oct 9, 2019

baha-ai Oct 9, 2019

baha-ai Oct 10, 2019

sandrask Oct 10, 2019

baha-ai Oct 10, 2019

baha-ai Oct 10, 2019

llorllale Oct 11, 2019

baha-ai Oct 11, 2019

llorllale Oct 11, 2019

llorllale commented Oct 11, 2019

Moopli commented Oct 11, 2019

llorllale commented Oct 11, 2019

baha-ai commented Oct 11, 2019

Moopli commented Oct 11, 2019 •

edited

Loading

Moopli left a comment

troyronda Oct 11, 2019

baha-ai Oct 11, 2019

Moopli Oct 11, 2019

troyronda Oct 11, 2019 •

edited

Loading

baha-ai Oct 11, 2019

baha-ai Oct 11, 2019

Moopli left a comment

refactor: Crypter/Wallet crypto operations #459

refactor: Crypter/Wallet crypto operations #459

Conversation

baha-ai commented Oct 9, 2019 • edited Loading

codecov bot commented Oct 9, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale commented Oct 11, 2019

Moopli commented Oct 11, 2019

llorllale commented Oct 11, 2019

baha-ai commented Oct 11, 2019

Moopli commented Oct 11, 2019 • edited Loading

Moopli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troyronda Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Moopli left a comment

Choose a reason for hiding this comment

baha-ai commented Oct 9, 2019 •

edited

Loading

codecov bot commented Oct 9, 2019 •

edited

Loading

Moopli commented Oct 11, 2019 •

edited

Loading

troyronda Oct 11, 2019 •

edited

Loading