Skip to content

Commit

Permalink
fix certificate validation
Browse files Browse the repository at this point in the history
  • Loading branch information
@plebhash
plebhash committed Feb 8, 2024
1 parent 12356bd commit bdc3880
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 3 deletions.
2 changes: 1 addition & 1 deletion protocols/v2/noise-sv2/src/initiator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ impl Initiator {
.0
.serialize();
let rs_pk_xonly = XOnlyPublicKey::from_slice(&rs_pub_key).unwrap();
if signature_message.verify(&rs_pk_xonly) {
if signature_message.verify(&rs_pk_xonly, &self.pk) {

This comment has been minimized.

Sign in to view

Copy link
@Sjors

Sjors Feb 9, 2024

Is self.pk the expected responder authority key? If so, that's a confusing variable name.

This comment has been minimized.

Sign in to view

Copy link
@Sjors

Sjors Feb 9, 2024

I would probably rename pub_key to authority_pubkey in the above, and then self.pk to self.(expected_)authority_pubkey.

This comment has been minimized.

Sign in to view

Copy link
@Sjors

Sjors Feb 9, 2024

The line is also very confusing (maybe wrong?):

let pub_key: Secp256k1PublicKey = authority_public_key;
let initiator = Initiator::from_raw_k(pub_key.into_bytes())

The authority pubkey belongs to the responder, not the initiator.

This comment has been minimized.

Sign in to view

Copy link
@Sjors

Sjors Feb 9, 2024

It would be useful if someone added a code doc to explain what pub struct TemplateRx is. cc @Fi3

This comment has been minimized.

Sign in to view

Copy link
@Sjors

Sjors Feb 9, 2024

I'm guessing it's the role that's "receiving" templates from the Template Provider.

So here I would rename authority_public_key to expected_authority_public_key.

So this creates a pub struct Initiator instance, for which the constructor pub fn new(pk: XOnlyPublicKey) -> Box<Self> only needs the authority public key. The code seems correct, but I would rename pk to expected_authority_public_key for clarity.

I assume that when you tested this, you copied the authority key Bitcoin Core spits out (in base58 format) and put in the config file as tp_authority_pub_key?

So then your verification function takes two arguments: verify(self, pk: &XOnlyPublicKey, authority_pk: &XOnlyPublicKey)

The first is the static key that the Template Provider sent over during the handshake. The second is the authority pubkey we got from the config file.

It then adds the serialized static key to the certificate hash (&pk.serialize()). That seems correct. It then verifies that the signature was set by the authority key: secp.verify_schnorr(&s, &m, authority_pk).is_ok()

That also seems correct.

This comment has been minimized.

Sign in to view

Copy link
@lorbax

lorbax Feb 9, 2024

The line is also very confusing (maybe wrong?):

let pub_key: Secp256k1PublicKey = authority_public_key;
let initiator = Initiator::from_raw_k(pub_key.into_bytes())

The authority pubkey belongs to the responder, not the initiator.

Hi @Sjors if I remember correctly, the authority is taken from the config of any role that starts an handshake as the "initiator". Also, the initiator, at a certain point of the handshake, receives the certificate, which includes the responder's static pubkey. The certificate is signed by the authority, this means that the authority guarantees that the responder can be trusted. Then the initiator checks the certificate against the authority pubkey and can finally trust the responder.

So, it should be
let rs_pk_xonly: XOnlyPublicKey = self.pk
or something like that.
I agree with @Sjors that the variables' name is not the best.

This comment has been minimized.

Sign in to view

Copy link
@plebhash

plebhash Feb 9, 2024 •
edited
Loading

Author Owner

I assume that when you tested this, you copied the authority key Bitcoin Core spits out (in base58 format) and put in the config file as tp_authority_pub_key?

No I didn't. I'm not sure how to extract the authority key from Bitcoin Core.

This comment has been minimized.

Sign in to view

Copy link
@lorbax

lorbax Feb 9, 2024 •
edited
Loading

By the way, if we follow the code, the pk field of the Initiator comes from the config file, which is the correct behaviour.
Also, the pool doesn't need authority because, as I understood, most of the cases the pool is the authority.

This comment has been minimized.

Sign in to view

Copy link
@Sjors

Sjors Feb 9, 2024

Regarding the config, I commented about that here: stratum-mining#717 (comment)
let (temp_k1, temp_k2) = Self::hkdf_2(self.get_ck(), &[]);
let c1 = ChaCha20Poly1305::new(&temp_k1.into());
let c2 = ChaCha20Poly1305::new(&temp_k2.into());
Expand Down
6 changes: 4 additions & 2 deletions protocols/v2/noise-sv2/src/signature_message.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,21 @@ impl From<[u8; 74]> for SignatureNoiseMessage {
}

impl SignatureNoiseMessage {
pub fn verify(self, pk: &XOnlyPublicKey) -> bool {
pub fn verify(self, pk: &XOnlyPublicKey, authority_pk: &XOnlyPublicKey) -> bool {
let now = SystemTime::now()
.duration_since(SystemTime::UNIX_EPOCH)
.unwrap()
.as_secs() as u32;
if self.valid_from <= now && self.not_valid_after >= now {
let secp = Secp256k1::verification_only();
let (m, s) = self.split();
let m = Message::from_hashed_data::<sha256::Hash>(&m[0..10]);
let m = [&m[0..10], &pk.serialize()].concat();

This comment has been minimized.

Sign in to view

Copy link
@Sjors

Sjors Feb 9, 2024

Maybe add a comment here that explains which fields are concatenated and refers to the spec.
let m = Message::from_hashed_data::<sha256::Hash>(&m);
let s = match Signature::from_slice(&s) {
Ok(s) => s,
_ => return false,
};
// secp.verify_schnorr(&s, &m, authority_pk).is_ok()

This comment has been minimized.

Sign in to view

Copy link
@Sjors

Sjors Feb 9, 2024

Hopefully this works after my fix from today...
secp.verify_schnorr(&s, &m, pk).is_ok()
} else {
false
Expand Down

0 comments on commit bdc3880

Please sign in to comment.