Encrypt decrypt #2130

yemmyharry · 2024-03-04T16:54:51Z

btcec Encrypt / Decrypt used to be available in btcec but wasn't present in btcec/v2. This PR adds it in btcec/v2.
https://pkg.go.dev/github.com/btcsuite/btcd/btcec/v2
Closes #1880

coveralls · 2024-03-05T08:11:31Z

Pull Request Test Coverage Report for Build 8143981554

Details

50 of 68 (73.53%) changed or added relevant lines in 1 file are covered.
7 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.01%) to 56.787%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
btcec/ciphering.go	50	68	73.53%

Files with Coverage Reduction	New Missed Lines	%
connmgr/connmanager.go	2	86.62%
peer/peer.go	5	74.16%

Totals
Change from base Build 8117173129:	0.01%
Covered Lines:	29340
Relevant Lines:	51667

💛 - Coveralls

Chinwendu20 · 2024-03-06T17:35:26Z

Nice this is the commit that removes it:
87e8fe9#diff-c24bf040ebe7e026d5ae1d5f3c14771574b4d2b87f5245d71f6b211ff784160c

I think we might get more context as to why it was removed. The function that was used to type alias the functions removed here does not have it either:
https://pkg.go.dev/github.com/decred/dcrd/dcrec/secp256k1/v4#pkg-functions

yemmyharry · 2024-03-07T10:13:12Z

@Chinwendu20 The context as to why it was removed is found here: #1880 (comment).
The dcrd package doesn't have it but they gave an example as to how it can be done here: https://pkg.go.dev/github.com/decred/dcrd/dcrec/secp256k1/v4#example-package-EncryptDecryptMessage. Having the functions to make it easy for users seems better don't you think?

lazynina · 2024-04-17T13:40:43Z

@guggero - is it possible to get a review of this PR please? I have a project that we recently upgraded to the latest version of the library and ended up copying over this code. Would be great to have this functionality available in the library again.

guggero · 2024-04-17T14:49:41Z

@guggero - is it possible to get a review of this PR please? I have a project that we recently upgraded to the latest version of the library and ended up copying over this code. Would be great to have this functionality available in the library again.

I'm super busy right now. You're welcome to add a review yourself, since you seem to need the code.

Chinwendu20

Thanks a lot for the PR and sharing the reference doc, it helped me review. Left a question and other comments. Also is there a reason why this b9f98b6 is a separate commit? Can it not be merged with previous?

Commits are usually in the form: package name: commit message. Here is the guideline:
https://github.com/lightningnetwork/lnd/blob/master/docs/code_contribution_guidelines.md#ideal-git-commit-structure

I think maybe these guidelines should be included in this repo as well.
Also I am not entirely sure why this should be here (same withe the sharedSecrets function), I do not think we use them any where, maybe there is a reason it was left for users to create themselves, it is not even in the decred
secp256k1 package itself. Maybe it would be more appropriate to have that there? then we can have encrypt functions for AES-256, AES-128, etc.

btcec/ciphering.go

Chinwendu20 · 2024-04-18T14:46:13Z

Also I am not entirely sure why this should be here (same withe the sharedSecrets function), I do not think we use them any where, maybe there is a reason it was left for users to create themselves, it is not even in the decred secp256k1 package itself. Maybe it would be more appropriate to have that there? then we can have encrypt functions for AES-256, AES-128, etc.

Ohh I see btcec was created as a standalone golang bitcoin as opposed to decred that is for a different ecosystem.

Chinwendu20

I could be wrong but I think the point of the issue that birthed this PR is to bring back the Encrypt and Decrypt function here back here:

https://github.com/Roasbeef/btcd/blob/e8e2167a1af6c20f1f32abb76bd387ee87763bc9/btcec/ciphering.go#L51-L118

Left comments in the Resolved thread as well. Maybe hold on, on resolving comment for now.

btcec/ciphering.go

Chinwendu20 · 2024-04-19T12:17:29Z

btcec/ciphering.go

+	ecdhKey := GenerateSharedSecret(ephemeral, pubKey)
+	hashedSecret := sha256.Sum256(ecdhKey)
+	encryptionKey := hashedSecret[:16]
+	block, err := aes.NewCipher(encryptionKey)


Seems like you sliced the shared secret to 16 bytes here

yes for compatibility with AES-128-GCM encryption

Ohh thought you said AES-256-GCM here: a2245a6#r1570762508 but looks like the comment has been updated.

Chinwendu20 · 2024-04-19T14:16:06Z

btcec/ciphering.go

+	}
+
+	gcm, err := cipher.NewGCMWithNonceSize(block, 16)
+	if err != nil {


I would like to understand why it has to be a nonce size of 16 instead of the default 12

Chinwendu20 · 2024-04-19T15:33:59Z

btcec/ciphering.go

@@ -22,14 +23,19 @@ func GenerateSharedSecret(privkey *PrivateKey, pubkey *PublicKey) []byte {

 // Encrypt encrypts data for the target public key using AES-128-GCM
 func Encrypt(pubKey *PublicKey, msg []byte) ([]byte, error) {
+	var pt bytes.Buffer
+


You want to copy the cipher text into a different variable?

var pt bytes.Buffer is used to accumulate the encrypted data in a buffer, which is then returned as a single byte slice at the end of the function

why not return the cipher text itself?

i didnt return the ciphertext itself because i need some details in the decrypt function. the tag (and other parameters like nonce etc) are appended to the ciphertext in the encrypt function and in the decrypt function the tag is separated from the ciphertext to verify authenticity.

I think the tag is in the cipher text already

Is there a reason that you did not just do this:

nonce := make([]byte, aead.NonceSize()) ciphertext := make([]byte, 4+len(ephemeralPubKey)) binary.LittleEndian.PutUint32(ciphertext, uint32(len(ephemeralPubKey))) copy(ciphertext[4:], ephemeralPubKey) ciphertext = aead.Seal(ciphertext, nonce, plaintext, ephemeralPubKey)

Gotten from here: https://pkg.go.dev/github.com/decred/dcrd/dcrec/secp256k1/v4#section-readme

No need for the intermediary pt bytes in this case.

Both approaches are valid. I used a buffer as it automatically grows and shrinks as needed, reducing manual memory management.

IMO, I do not think the buffer is needed at all when the cipher text can just be returned directly unless there is need to copy the cipher text into a different variable. if not maybe we should not use extra space.

i didnt return the ciphertext itself because i need some details in the decrypt function. the tag (and other parameters like nonce etc) are appended to the ciphertext in the encrypt function and in the decrypt function the tag is separated from the ciphertext to verify authenticity.

nonce := make([]byte, aead.NonceSize()) ciphertext := make([]byte, 4+len(ephemeralPubKey)) binary.LittleEndian.PutUint32(ciphertext, uint32(len(ephemeralPubKey))) copy(ciphertext[4:], ephemeralPubKey) ciphertext = aead.Seal(ciphertext, nonce, plaintext, ephemeralPubKey)

If you write it this way would you still need the intermediary buffer?

https://github.com/Chinwendu20/btcd/blob/317cdc7a2a2e25f5d0251decaebccc9da2b2fb66/btcec/ciphering.go#L26-L55
This is what I mean, no need for the buffer or any intermediate?

Chinwendu20 · 2024-04-19T15:38:19Z

btcec/ciphering.go

@@ -62,3 +63,44 @@ func Encrypt(pubKey *PublicKey, msg []byte) ([]byte, error) {

 	return pt.Bytes(), nil
 }
+
+// Decrypt decrypts a passed message with a receiver private key, returns plaintext or decryption error


format: Line length
Edit:
80 cols limit

Chinwendu20 · 2024-04-19T15:42:32Z

btcec/ciphering.go

+// Decrypt decrypts a passed message with a receiver private key, returns plaintext or decryption error
+func Decrypt(privkey *PrivateKey, msg []byte) ([]byte, error) {
+	// Message cannot be less than length of public key (65) + nonce (16) + tag (16)
+	if len(msg) <= (1 + 32 + 32 + 16 + 16) {


Isn't the length of public key compressed 33 bytes? Maybe make comments clearer on how the number came about, what does 1 represent here?

An uncompressed public key in Elliptic Curve Cryptography (ECC) consists of:
1 byte for the curve type
32 bytes for the X coordinate
32 bytes for the Y coordinate

An uncompressed public key in Elliptic Curve Cryptography (ECC) consists of: 1 byte for the curve type 32 bytes for the X coordinate 32 bytes for the Y coordinate

Was actually referring to serialized compressed public key which is what was written into the bytes during the encryption.

its actually an uncompressed public key.

Oh that is true, I can see that an uncompressed public key was used in the encrypt function. Maybe this code would be clearer this way:

Suggested change

if len(msg) <= (1 + 32 + 32 + 16 + 16) {

// Message cannot be less than the size of an

// uncompressed public key (65 bytes) + nonce (16 bytes)

// + tag (16 bytes)

if len(msg) <= (65+ 16 + 16) {

Or something better
If 1+ 32 + 32 is to be included maybe include where the numbers are from in the comment as well?

Chinwendu20 · 2024-04-20T00:05:32Z

I could be wrong but I think the point of the issue that birthed this PR is to bring back the Encrypt and Decrypt function here back here:

https://github.com/Roasbeef/btcd/blob/e8e2167a1af6c20f1f32abb76bd387ee87763bc9/btcec/ciphering.go#L51-L118

Left comments in the Resolved thread as well. Maybe hold on, on resolving comment for now.

I think in general the point of this PR is to bring back the encrypt and decrypt function that was previously in btcec as opposed to writing a new one. Though maybe new encryption and decryption functions could be included as well but not the point of the issue.

Others can give their opinion.

yemmyharry added 4 commits March 3, 2024 23:42

add encrypt function

a2245a6

made changes to encrypt function

b9f98b6

add decrypt function

9bbd1a4

add test for encrypt and decrypt functions

1b9b22f

yemmyharry mentioned this pull request Mar 4, 2024

btcec Encrypt / Decrypt #1880

Open

Chinwendu20 reviewed Apr 18, 2024

View reviewed changes

btcec/ciphering.go Show resolved Hide resolved

btcec/ciphering.go Outdated Show resolved Hide resolved

btcec: update comment [skip ci]

902f456

yemmyharry requested a review from Chinwendu20 April 18, 2024 14:40

Chinwendu20 reviewed Apr 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encrypt decrypt #2130

Encrypt decrypt #2130

yemmyharry commented Mar 4, 2024 •

edited

coveralls commented Mar 5, 2024

Chinwendu20 commented Mar 6, 2024

yemmyharry commented Mar 7, 2024

lazynina commented Apr 17, 2024

guggero commented Apr 17, 2024

Chinwendu20 left a comment •

edited

Chinwendu20 commented Apr 18, 2024

Chinwendu20 left a comment •

edited

Chinwendu20 Apr 19, 2024 •

edited

yemmyharry Apr 19, 2024

Chinwendu20 Apr 19, 2024

Chinwendu20 Apr 19, 2024

Chinwendu20 Apr 19, 2024

yemmyharry Apr 19, 2024 •

edited

Chinwendu20 Apr 19, 2024

yemmyharry Apr 19, 2024

Chinwendu20 Apr 20, 2024

yemmyharry Apr 20, 2024

Chinwendu20 Apr 20, 2024

yemmyharry Apr 20, 2024

Chinwendu20 Apr 20, 2024

Chinwendu20 Apr 20, 2024

Chinwendu20 Apr 19, 2024 •

edited

Chinwendu20 Apr 19, 2024

yemmyharry Apr 19, 2024 •

edited

Chinwendu20 Apr 20, 2024

yemmyharry Apr 20, 2024

Chinwendu20 Apr 20, 2024

Chinwendu20 commented Apr 20, 2024 •

edited

-	if len(msg) <= (1 + 32 + 32 + 16 + 16) {
+	// Message cannot be less than the size  of an
+	// uncompressed public  key (65 bytes) + nonce (16 bytes)
+	// + tag (16 bytes)
+	if len(msg) <= (65+ 16 + 16) {

Encrypt decrypt #2130

Are you sure you want to change the base?

Encrypt decrypt #2130

Conversation

yemmyharry commented Mar 4, 2024 • edited

coveralls commented Mar 5, 2024

Pull Request Test Coverage Report for Build 8143981554

Details

💛 - Coveralls

Chinwendu20 commented Mar 6, 2024

yemmyharry commented Mar 7, 2024

lazynina commented Apr 17, 2024

guggero commented Apr 17, 2024

Chinwendu20 left a comment • edited

Choose a reason for hiding this comment

Chinwendu20 commented Apr 18, 2024

Chinwendu20 left a comment • edited

Choose a reason for hiding this comment

Chinwendu20 Apr 19, 2024 • edited

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

yemmyharry Apr 19, 2024 • edited

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

Chinwendu20 Apr 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yemmyharry Apr 19, 2024 • edited

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

Chinwendu20 commented Apr 20, 2024 • edited

yemmyharry commented Mar 4, 2024 •

edited

Chinwendu20 left a comment •

edited

Chinwendu20 left a comment •

edited

Chinwendu20 Apr 19, 2024 •

edited

yemmyharry Apr 19, 2024 •

edited

Chinwendu20 Apr 19, 2024 •

edited

yemmyharry Apr 19, 2024 •

edited

Chinwendu20 commented Apr 20, 2024 •

edited