[group key addrs 2/?]: internal/ecies: add encrypt/decrypt with ECIES #1512
Conversation
Pull Request Test Coverage Report for Build 15299877521Details
💛 - Coveralls |
|
@Roasbeef: review reminder |
internal/ecies/ecies.go
Outdated
| var result bytes.Buffer | ||
| result.Write(nonce) | ||
|
|
||
| gcm, err := cipher.NewGCMWithNonceSize(block, GCMNonceSize) |
There was a problem hiding this comment.
Any particular reason to use this over NewGCM? 12-byte nonces are standardized, it's also what the underlying Go packages uses with the default constructor.
There was a problem hiding this comment.
See section 5.2.1 of this PDF: https://nvlpubs.nist.gov/nistpubs/legacy/sp/nistspecialpublication800-38d.pdf
For IVs, it is recommended that implementations restrict support to the length of 96 bits, to
promote interoperability, efficiency, and simplicity of design.
IV here is basically a nonce.
There was a problem hiding this comment.
One alternative crypto system to consider is chacha20poly1305: https://pkg.go.dev/golang.org/x/crypto/chacha20poly1305#New. This is what we use for encryption in LN. It also natively supports adding associative data (plaintext data included in the MAC).
There was a problem hiding this comment.
Hmm, I just used an example I found somewhere.
But I like ChaCha20Poly1305 better, changed the code to use that. We'll use the sender's ephemeral public key as the additional data, to authenticate the ciphertext. It will be part of the outer (authmailbox message) envelope, not the ciphertext itself. Does that make sense?
There was a problem hiding this comment.
We'll use the sender's ephemeral public key as the additional data, to authenticate the ciphertext
Makes sense, this is exactly what I had in mind!
When it comes to the question of if we should have the key be a "part" of the cipher text (sent as a single blob), or split out into a single key, I lean towards making it part of the cipher text. That way it's a logical unit during transport.
internal/ecies/ecies.go
Outdated
| // SHA256. | ||
| func HkdfSha256(secret []byte) ([32]byte, error) { | ||
| var key [32]byte | ||
| kdf := hkdf.New(sha256.New, secret, nil, nil) |
There was a problem hiding this comment.
Maybe add a comment here explaining why it's ok to not use a salt here.
There was a problem hiding this comment.
Actually, adding some salt here wouldn't be too bad of an idea. Added the protocol name to make the derived key unique to this application.
internal/ecies/ecies.go
Outdated
| func EncryptSha256ChaCha20Poly1305(sharedSecret [32]byte, msg []byte, | ||
| additionalData []byte) ([]byte, error) { | ||
|
|
||
| // We begin by stretching the shared secret using HKDF with SHA256. |
There was a problem hiding this comment.
Not a lot of stretching involved if it goes from 32 bytes to 32 bytes. 😄
There was a problem hiding this comment.
Yeah, you're right. Re-formulated to "hardening against brute forcing".
| } | ||
|
|
||
| // Select a random nonce, and leave capacity for the ciphertext. | ||
| nonce := make( |
There was a problem hiding this comment.
As we're using a random nonce here, we should use NewX, which is meant for cases where a counter-like nonce isn't used. Basically some extra security margin for when nonces are generated randomly.
There was a problem hiding this comment.
Ah, yes, makes sense. Changed to X.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @guggero)
This commit creates a simple encryption and decryption function that uses ChaCha20Poly1305 for tne symmetric encryption and HKDF with SHA256 for the key derivation. The shared key generation is not part of these functions, because we'll need to use lnd RPCs in some cases to be able to derive it.
No, that looks like a flake. Re-triggered it. |
Implements the ECIES encryption scheme as mentioned here: #874 (comment)
This commit creates a simple encryption and decryption function that uses ChaCha20Poly1305 for tne symmetric encryption and HKDF with SHA256 for the key derivation.
The shared key generation is not part of these functions, because we'll need to use lnd RPCs in some cases to be able to derive it.
The idea is that we'd transport the sender's ephemeral public key outside of the cipher text and use it as the additional data to authenticate the ciphertext.
This change is