Questions about noise_sv2 crate

[plebhash]

this discussion should guide the documentation of noise_sv2, as described on #845

[plebhash]

the aes-gcm crate provides an implementation of AES-GCM cipher, which is an alternative to ChaCha20Poly1305 cipher (provided by chacha20poly1305 crate)

both crates are used as dependencies for noise_sv2

inside impl AeadCipher for ChaCha20Poly1305 we have:

stratum/protocols/v2/noise-sv2/src/aed_cipher.rs

Lines 27 to 34 in 24b8b1d

	fn encrypt<T: Buffer>(
	&mut self,
	nonce: &[u8; 12],
	ad: &[u8],
	data: &mut T,
	) -> Result<(), aes_gcm::Error> {
	self.encrypt_in_place(nonce.into(), ad, data)
	}

I'm confused: how can ChaCha20Poly1305::encrypt_in_place return aes_gcm::Error, if they come from different crates?

[plebhash]

ah I see: both aes_gcm::Error and chacha20poly1305::Error are actually re-exports of aead::Error:

https://github.com/RustCrypto/AEADs/blob/f4e39243d2d3ca9ceea4c6e1782359ff9400bbb9/chacha20poly1305/src/lib.rs#L146
https://github.com/RustCrypto/AEADs/blob/f4e39243d2d3ca9ceea4c6e1782359ff9400bbb9/aes-gcm/src/lib.rs#L111

note for the documentation efforts: it is probably be a good idea to replace aes_gcm::Error with aead::Error to avoid this kind of confusion to other readers

[Shourya742]

@Fi3, In noise_sv2, we're implementing AEAD ciphers for two algorithms: 1. ChaCha20Poly1305 and 2. AES256-GCM. However, we're only using ChaCha20Poly1305 for Noise. Is there a specific reason for including AES256-GCM?

[jakubtrnka]

I'd suggest that we remove everything related to AES-GCM from the code. I think there is no mention of it in the spec anymore.
that multi-cipher feature turned out to be unnecessary.

[Fi3]

we still have it in the spec so we should also remove it also from there

[GitGab19]

we still have it in the spec so we should also remove it also from there

Yeah AES-GCM is still there: https://github.com/stratum-mining/sv2-spec/blob/main/04-Protocol-Security.md#441-cipherstate-object
Opened an issue to remove it from specs: stratum-mining/sv2-spec#93

[Sjors]

Where in the handshake could we even negotiate the choice between ChaCha20Poly1305 and AES-GCM? I think that was removed in #61.

On the Bitcoin Core side I haven't implemented the latter at all.

[Fi3]

yep we need to remove every reference to AES in the spec and also remove it from the impl

[Shourya742]

@Fi3, Can you provide more insights on this code snippet? It makes sense for Cipher to be non-Sync and non-copyable so that we don't have multiple copies of it and to prevent concurrent access by multiple threads. However, I see that the implementation to enforce this is commented out. Why wasn't it implemented?

// Make sure that Cipher is not sync so we do not need to worry about what other memory accessor see
// after that we zeroize k is send cause if we send it the original thread can not access
// anymore it
//impl<C: AeadCipher> !Sync for Cipher<C> {}
//impl<C: AeadCipher> !Copy for Cipher<C> {}
impl<C: AeadCipher> Cipher<C> {
    /// Internal use only, we need k for handshake
    pub fn from_key_and_cipher(k: [u8; 32], c: C) -> Self {
        Self {
            k: Some(k),
            n: 0,
            cipher: Some(c),
        }
    }

    /// At the end of the handshake we return a cipher with hidden key
    #[allow(dead_code)]
    pub fn from_cipher(c: C) -> Self {
        Self {
            k: None,
            n: 0,
            cipher: Some(c),
        }
    }
}

cc: @rrybarczyk @plebhash

[Fi3]

ok maybe it work only on nightly and I just commented it so when will be stabilized we can use it

[rrybarczyk]

Are there many features that are only supported on nightly? How can we tell which features are nightly only?

[Shourya742]

Given the current structure of the Cipher struct, it is highly unlikely that it is Sync or Copy. Both the encryption key k and nonce n are primitives, making them Sync and Copy. However, the traits of the entire struct depend on all of its fields. The primary concern is the cipher field, which implements the AeadCipher trait. Since this trait includes methods that require a mutable reference to self, the cipher field is neither Copy nor Sync, which means the Cipher struct as a whole is also neither Sync nor Copy.

[Fi3]

@Shourya742 yep is correct, having it enforced by typesystem can useful if we modify the code. @rrybarczyk if it compile on stable we are not using features available only on nighlty

[rrybarczyk]

Sounds good, thank you.

[Shourya742]

@Fi3, the generatekey method in handshake.rs, in this method we are generating key pairs and checking if the parity is odd or not, and if it is then we are generating a new pair. It is a recursive call. Why are we not just negating the key, in case when the parity is not what we expect?

fn generate_key() -> Keypair {
        let secp = Secp256k1::new();
        let (secret_key, ) = secp.generate_keypair(&mut rand::thread_rng());
        let kp = Keypair::from_secret_key(&secp, &secret_key);
        if kp.x_only_public_key().1 == crate::PARITY {
            kp
        } else {
            Self::generate_key()
        }
    }

cc: @rrybarczyk

[Fi3]

negating the key is a far better solution

[plebhash]

what happens if we instantiate a noise_sv2::Initiator without a pk?

@Fi3

[Fi3]

double check it, but I think that in this case we do not check the certificate. So you still have an encrypted connection but you don't authenticate the upstream

[Fi3]

For example when you connect to a TP that you run locally