Fix certificate verification

[plebhash]

close stratum-mining#717

[Sjors]

I think we use 3VANfft6ei6jQq1At7d8nmiZzVhBFS4CiQujdgim1ign as a placeholder key everywhere.

[plebhash]

3VANfft6ei6jQq1At7d8nmiZzVhBFS4CiQujdgim1ign is the default value of authority_public_key on the config file.

https://github.com/stratum-mining/stratum/blob/a0cc7913559412b6b9a5d891dc344368b4879e2d/roles/pool/config-examples/pool-config-hosted-tp-example.toml#L2

I thought we wanted tp_authority_public_key to hold a value that's coming from Bitcoin Core logs?
I got this value from:

2024-02-09T16:40:46Z Template Provider authority key: EguTM8URcZDQVeEBsM4B5vg9weqEUnufA8pm85fG4bZd

[Sjors]

It's randomly generated the first time you run the template provider. So it's going to be different for everyone.

[Sjors]

I plan to add support for importing a certificate later, so you'll be able to use an existing authority key. I.e. a pool might want that. I also have to write a tool that lets you generate and sign an arbitrary static key using the authority key.

[plebhash]

ok but we shouldn't really use 3VANfft6ei6jQq1At7d8nmiZzVhBFS4CiQujdgim1ign for tp_authority_public_key, should we?

[plebhash]

question for @Fi3 :

I'm adapting Initiator so that responder_authority_pk is Option<XOnlyPublicKey>

but step_2() returns Result<NoiseCodec, Error>, and I'm not sure how to return a NoiseCodec without a pubkey

[Fi3]

You have Initiator::new(authority_public_key). So you should change the type of initiator.authority_public_key¹ to Option<XOnlyPublicKey> and pass an option also to the new function. So that when authority_public_key is Some(authority_public_key) you do the check otherwise you don't.

Footnotes

1. I would call the field name authority_public_key rather responder_authority_pk cause authority_public_key is what we use in the spec, and for this cases stick impl_name = identity(spec_name) can be helpful. ↩

[plebhash]

yeah that's the direction I was going for

but when authority_public_key is None, what do we return for step_2()?

[Sjors]

I still think that adding support for None can be done in a followup PR.

[plebhash]

@Sjors it will, but I asked this question to @Fi3 before you had made this suggestion so I'm just using the momentum

[Sjors]

This now works for the pool role, but now the Job Declarator client can't connect to the pool, presumably because the pool needs to update the way it signs its certificate.

While you're at it, you may want to rename tp_authority_pub_key in the JD client config to tp_authority_public_key for consistency with the pool.

[Sjors]

This did the trick for me:

diff --git a/protocols/v2/noise-sv2/src/responder.rs b/protocols/v2/noise-sv2/src/responder.rs
index 15ebe190..f5a3ddcc 100644
--- a/protocols/v2/noise-sv2/src/responder.rs
+++ b/protocols/v2/noise-sv2/src/responder.rs
@@ -27,10 +27,12 @@ pub struct Responder {
     h: [u8; 32],
     // ephemeral keypair
     e: Keypair,
     // Static pub keypair
     s: Keypair,
+    // Authority pub keypair
+    a: Keypair,
     c1: Option<GenericCipher>,
     c2: Option<GenericCipher>,
     cert_validity: u32,
 }
 
@@ -105,19 +107,20 @@ impl Responder {
         } else {
             Err(Error::InvalidRawPublicKey)
         }
     }
 
-    pub fn new(s: Keypair, cert_validity: u32) -> Box<Self> {
+    pub fn new(a: Keypair, cert_validity: u32) -> Box<Self> {
         let mut self_ = Self {
             handshake_cipher: None,
             k: None,
             n: 0,
             ck: [0; 32],
             h: [0; 32],
             e: Self::generate_key(),
-            s,
+            s: Self::generate_key(),
+            a,
             c1: None,
             c2: None,
             cert_validity,
         };
         Self::initialize_self(&mut self_);
@@ -268,34 +271,35 @@ impl Responder {
         ret[5] = valid_from[3];
         ret[6] = not_valid_after[0];
         ret[7] = not_valid_after[1];
         ret[8] = not_valid_after[2];
         ret[9] = not_valid_after[3];
-        SignatureNoiseMessage::sign(&mut ret, &self.s);
+        SignatureNoiseMessage::sign(&mut ret, &self.s.x_only_public_key().0, &self.a);
         ret
     }
 
     fn erase(&mut self) {
         if let Some(k) = self.k.as_mut() {
             for b in k {
                 unsafe { ptr::write_volatile(b, 0) };
             }
         }
         for mut b in self.ck {
             unsafe { ptr::write_volatile(&mut b, 0) };
         }
         for mut b in self.h {
             unsafe { ptr::write_volatile(&mut b, 0) };
         }
         if let Some(c1) = self.c1.as_mut() {
             c1.erase_k()
         }
         if let Some(c2) = self.c2.as_mut() {
             c2.erase_k()
         }
         self.e.non_secure_erase();
         self.s.non_secure_erase();
+        self.a.non_secure_erase();
     }
 }
 
 impl Drop for Responder {
     fn drop(&mut self) {
diff --git a/protocols/v2/noise-sv2/src/signature_message.rs b/protocols/v2/noise-sv2/src/signature_message.rs
index d58083ba..48bfeace 100644
--- a/protocols/v2/noise-sv2/src/signature_message.rs
+++ b/protocols/v2/noise-sv2/src/signature_message.rs
@@ -42,13 +42,15 @@ impl SignatureNoiseMessage {
             secp.verify_schnorr(&s, &m, authority_pk).is_ok()
         } else {
             false
         }
     }
-    pub fn sign(msg: &mut [u8; 74], kp: &Keypair) {
+    pub fn sign(msg: &mut [u8; 74], static_pk: &XOnlyPublicKey, kp: &Keypair) {
         let secp = Secp256k1::signing_only();
-        let m = Message::from_hashed_data::<sha256::Hash>(&msg[0..10]);
+        let m = [&msg[0..10], &static_pk.serialize()].concat();
+        let m = Message::from_hashed_data::<sha256::Hash>(&m);
+
         let signature = secp.sign_schnorr(&m, kp);
         for (i, b) in signature.as_ref().iter().enumerate() {
             msg[10 + i] = *b;
         }
     }

[Sjors]

@jakubtrnka this PR - with my diff above - will break any WIP hardware support (assuming it checks the certificate). So you may want to check if this change is correct (I think it is).

[plebhash]

another open question for my end is:

so far we are only fixing the pool and JDC roles, but which other roles will also need fixing?
it might be a good idea for me to also include them on this PR

[plebhash]

@Sjors 78ff124 is addressing the JDC issues you pointed out

[Sjors]

Nice, tested that this works (I rebased your 7 commits on top of of the latest lorbax/ElligatorSwift branch. The pool role can connect to the template provider and the job declarator client role can connect to the pool.

[Sjors]

so far we are only fixing the pool and JDC roles, but which other roles will also need fixing

As long as all roles use the noise-sv2 responder / initiator they should work. I'm not sure how complete automated test coverage is, so it's probably worth testing different configurations.

My testing setup is:

• a pool role connected to the TP
• a job declarator server connected to the TP
• a job declarator client connected to the TP, and to the pool
• a v1-v2 translator connected to the pool

Those all work. I haven't tested a proxy and I haven't tested v2 aware miner firmware.

[Sjors]

I'll work on an update to the Template Provider to make this work since stratum-mining#722 was merged. Right now the Bitcoin Core TP will use the old base58 encoding.

stratum-mining#746 is potentially a workaround in the mean time; it you don't set the TP authority key, you can simply merge this without having to wait for me.

Of course EllSwift has to be merged first.

[Sjors]

I pushed a commit to 2024/02/sv2-poll-ellswift that should fix the base58 encoding. I wasn't able to test it yet. In order to test this, stratum-mining#737 needs to be rebased and then your PR needs to be rebased on top of that.

I got a merge conflict when trying to do that quickly.

[Sjors]

stratum-mining#737 was just rebased.

I was able to resolve the merge conflicts and test. Works!

[Sjors]

Here's my rebase: https://github.com/Sjors/stratum/tree/2024/02/cert-fix-rebase

[Sjors]

@plebhash can you open a PR against the main repo?

[Sjors]

Maybe close this since it's a duplicate?