Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal for the use Bitcoin-Core native cryptographic primitives #10

Merged
merged 16 commits into from Nov 23, 2022

Conversation

jakubtrnka
Copy link
Collaborator

This PR:

  1. proposes to use cryptographic primitives used in Bitcoin Core
    • It deprecates using curve25519 in favor of secp256k1
    • proposes using SHA256 instead of Blake2s
    • proposes using Schnorr/secp256k1 for Signature Noise Message for server authentication
    • it implies changing CA-authority public keys for existing pools and introducing new format for CA public keys in order to avoid mistaking secp256k1 pubkeys for ed25519 pubkeys on new and old systems
    • requires mandatory support for Noise_NX_secp256k1_ChaChaPoly_SHA256 handshake and optional support for Noise_NX_secp256k1_AESGCM_SHA256
  2. proposes to change message formats during handshake
    • Previously handshake messages used to be wrapped in length-delimited frames with 2-Byte length prefix. The length prefix is not necessary, because the size of messages in each handshake step is known
  3. Each handshake step thoroughly described, mainly extracted from https://noiseprotocol.org/noise.html for the specific NX protocol
  4. PR moves the description of noise-secure layer into a separate file

Secp256k1 curve points, which includes Public Keys and ECDH results, are points with of X- and Y-coordinate, 32-bytes each. There are several possibilities how to serialize them:
1. 64-byte full X- and Y-coordinate serialization for public keys (and ECDH results) and 96 Bytes for signatures.
2. 33-byte X-coordinate with 1 parity bit serialization for public keys and similarly 65-byte for signatures.
3. 32-byte X-coordinate only with implicit Y-coordinate for public keys and 64-byte for signatures.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe list 3 as 1 since this is the one we are choosing to implement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep it on the 3rd position. Because it's the least obvious approach.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to consider using option 1 given the secp256k1 C-lib used in core expects 64 bytes for ecdh operations. Not sure if it accepts 32 byte x-coordinate plus padding, would need to be investigated. Also the Rust secp256k1 library is an ffi wrapper that calls into the same C-lib secp256k1 and it seems like the PublicKey wrapper is also 64 bytes when calling ecdh operation:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about representation on the wire
The full public key as EC point is always reconstructed from the X coordinate assuming it has even parity.

04-Protocol-Security.md Outdated Show resolved Hide resolved

The key can be embedded into the mining URL as part of the path.

Authority Public key is encoded as a 32-byte secp256k1 public key (with implicit Y coordinate), prefixed with `[0x4b, 0x69]`, in [base58-check](https://en.bitcoin.it/wiki/Base58Check_encoding) encoding.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we don't have to use exactly this prefix and leave it undefined. Individual users can use these bytes to make the base58 encoded public key start with string prefix of their choice.

@QunL
Copy link

QunL commented Oct 19, 2022

In my opion, to protect privacy and lower detection possibility, the protocol could remove negotiation phrase and fix the Noise_NX_secp256k1_ChaChaPoly_SHA256.

@jakubtrnka
Copy link
Collaborator Author

jakubtrnka commented Oct 20, 2022

@QunL You have a good point. Let me summarize:
Currently proposed handshake begins with non-standard cipher negotiation. This makes it very easy to an eavesdropper to recognize Stratum-V2 connection. It would be preferable to initiate a connection with standard Noise_NX_secp256k1_ChaChaPoly_SHA256 handshake.

Removing the negotiation step entirely, however, removes the possibility to choose suitable aead-cipher that might have better performance characteristics.

I therefore suggest following:

  1. Initialize encrypted session using Noise_NX_secp256k1_ChaChaPoly_SHA256 protocol:
-> e
<- e, ee, s, es, SIGNATURE_NOISE_MESSAGE
  1. Once the encrypted session is established cipher upgrade can be negotiated:
// In the first message in transport state, client provides list of their available aead-ciphers other than
// chachapoly-1305
-> AEAD_CIPHERS

// Server responds/acks with Option<CipherChoice>. Option is non-empty if server requires to switch
// to a new cipher. It provides a new symmetric key alongside its choice - it can because the connection
// has been authenticated and is encrypted. Key exchange is not necessary here
<- CIPHER_CHOICE

// Both client and server now restart an encrypted session using negotiated aead-cipher and key provided by server.

where

struct CipherChoice {
    cipher_code: u32,
    symmetric_key: [u8; 32],
}
  1. Based on previous steps either continue using chacha-poly1305 or with a new algorithm

Advantages:

  1. Standard canonical initialization of a noise-encrypted connection
  2. We retain the ability to choose a cipher for the stratum-session
  3. It is simpler than previous proposal of using pre-handshake negotiation and working with prologue

@jakubtrnka
Copy link
Collaborator Author

Update:
During post-handshake cipher negotiation server no-longer decides about the symmetric key to use:
instead keys are derived from the original transport keys.

Another update:
Authority public keys are encoded with version-prefix [1, 0]. Not with some fancy byte prefix [0x4b, 0x69].

04-Protocol-Security.md Outdated Show resolved Hide resolved
04-Protocol-Security.md Outdated Show resolved Hide resolved
04-Protocol-Security.md Outdated Show resolved Hide resolved
04-Protocol-Security.md Outdated Show resolved Hide resolved
04-Protocol-Security.md Outdated Show resolved Hide resolved
9. return pair of CipherState objects, the first for encrypting transport messages from initiator to responder, and the second for messages in the other direction:
1. sets `temp_k1, temp_k2 = HKDF(ck, zerolen, 2)`
2. creates two new CipherState objects `c1` and `c2`
3. calls `c1.InitializeKey(temp_k1)` and `c2.InitializeKey(temp_k2)`
Copy link

@lorbax lorbax Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CAREFUL! It seems that initiator and responder have the same encryption key enc_k and the same decryption key dec_k. So, each party would use dec_k for decrypting a message encrypted with enc_k, a different key. I didn't look deeply at ChaCha20-Poly1305, but in a symmetric key algorithm, a message encrypted with a key k can be decrypted only with the key k. Consider switching the keys for one partcipant; for example, at line 263, for the initiator:
c1.InitializeKey(temp_k2) and c2.InitializeKey(temp_k1)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what's the issue here.
If the handshake completes successfully without any active attacker, it means both Initiator and Responder have identical value of ck, therefore (temp_k1, temp_k2) would be identical.
the first is used as a symmetric key for messages sent from initiator to responder, as described above in the text, and the other for stream in the opposite direction.
So:
All messages that initiator sends are encrypted with temp_k1. All messages that responder receives are decrypted with temp_k1.
And correspondingly, all messages that initiator receives are decrypted with temp_k2 and all messages that responder sends are encrypted with temp_k2.

Initiator never decrypts with temp_k1 nor does he encrypt with temp_k2. Also Responder never decrypts with temp_k2 and never encrypts with temp_k1, but that's just technical detail. It's not worth mentioning.

Copy link

@lorbax lorbax Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All messages that initiator sends are encrypted with temp_k1. All messages that responder receives are decrypted with temp_k1.
And correspondingly, all messages that initiator receives are decrypted with temp_k2 and all messages that responder sends are encrypted with temp_k2.

Ok it was specified for both initiator and responder, but I understood incorrectly, sorry.

3. calls `c1.InitializeKey(temp_k1)` and `c2.InitializeKey(temp_k2)`
4. returns the pair `(c1, c2)`

### 4.5.3 Server authentication
Copy link

@lorbax lorbax Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe is a good idea to spend few words (like a compressed form of our chat on discord
discord) about backward compatibility for miners/pools that use curve 25519 cryptography. Consider also a new paragraph about backward compatibility

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't make any references to earlier stages where different primitives were used. This would make the specification unnecessarily complex. And implementing support for previous versions is not desired. We can handle it in our own software and new independent implementations doesn't need to care about our previous attempts.

04-Protocol-Security.md Outdated Show resolved Hide resolved
@Fi3 Fi3 merged commit d0b496f into stratum-mining:main Nov 23, 2022
@jakubtrnka jakubtrnka deleted the noise/secp256k1 branch November 25, 2022 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants