search

tendermint / tmkms

Key Management service for Tendermint Validator nodes
Apache License 2.0
140 stars 43 forks source link

Ledger backend is not on par with tendermint votes/proposals #171

Closed adrianbrink closed 5 years ago

adrianbrink commented 5 years ago

This seems to be a larger problem with the KMS implementation. Currently the sign_bytes that are returned in session.rs/sign() change depending on whether the KMS is compiled with the YubiHSM backend or the Ledger backend. This should never be the case since the KMS should be agnostic to the backend being used.

So I've added logging after the sign_bytes method in called in session.rs/sign(). With the YubiHSM it results in Proposal: 119 Prevote: 109 or 110 Precommit: 109 or 110

With the ledger it results in: Proposal: 120 Prevote: 37

However the sign() method is called before it ever reaches any specific backend HSM. One reason for these errors could be that due to the conditional compilation with "--features" the KMS generates different data depending on which features are turned on.

@tarcieri @Liamsi I have no idea why this happens. With the Yubi HSM it works fine and sign_bytes returns the correct values, whereas if you don't compile with yubihsm the returned sign_bytes are wrong.

The second issue here is that the Ledger validator application does not expect these sign_bytes, since they are divergent from the specification. The reason why this only appears with the ledger is that the YubiHSM will just sign whatever it is given, but the ledger actually decodes the bytes. @jleni

adrianbrink commented 5 years ago

I've managed to get the same sign bytes for SignProposal if I add ledger and yubihsm to the default features. The ones for SignVote are still different.

13:18:53 [DEBUG] tmkms::session: started handling request ...
13:18:54 [DEBUG] tmkms::session: got sign proposal request
13:18:54 [DEBUG] tmkms::session: got sign request
13:18:54 [DEBUG] tmkms::session: sign_bytes for request: [119, 8, 32, 17, 1, 0, 0, 0, 0, 0, 0, 0, 33, 255, 255, 255, 255, 255, 255, 255, 255, 42, 72, 10, 32, 191, 103, 198, 145, 194, 126, 71, 202, 43, 138, 157, 77, 130, 186, 161, 188, 97,
 53, 37, 92, 124, 110, 78, 222, 47, 18, 148, 103, 202, 210, 142, 122, 18, 36, 10, 32, 229, 245, 202, 169, 32, 97, 200, 158, 211, 240, 106, 182, 197, 35, 14, 157, 114, 199, 37, 118, 2, 22, 36, 232, 216, 110, 41, 153, 215, 79, 219, 39, 16,
1, 50, 12, 8, 190, 213, 149, 227, 5, 16, 248, 190, 156, 140, 3, 58, 9, 116, 101, 115, 116, 45, 121, 117, 98, 105]
13:18:54 [DEBUG] tmkms::keyring: Successfully got signer and now trying to sign message
Ledger is trying to sign: [119, 8, 32, 17, 1, 0, 0, 0, 0, 0, 0, 0, 33, 255, 255, 255, 255, 255, 255, 255, 255, 42, 72, 10, 32, 191, 103, 198, 145, 194, 126, 71, 202, 43, 138, 157, 77, 130, 186, 161, 188, 97, 53, 37, 92, 124, 110, 78, 222,
 47, 18, 148, 103, 202, 210, 142, 122, 18, 36, 10, 32, 229, 245, 202, 169, 32, 97, 200, 158, 211, 240, 106, 182, 197, 35, 14, 157, 114, 199, 37, 118, 2, 22, 36, 232, 216, 110, 41, 153, 215, 79, 219, 39, 16, 1, 50, 12, 8, 190, 213, 149, 22
7, 5, 16, 248, 190, 156, 140, 3, 58, 9, 116, 101, 115, 116, 45, 121, 117, 98, 105]
Received an error
13:18:54 [ERROR] [test-yubi@tcp://127.0.0.1:26658] signing operation failed: received an invalid signature: received an invalid signature
13:18:55 [INFO] KMS node ID: 3DD79F0F25B3F71423DABEF47F43BAF3ABEEC68B
13:18:55 [DEBUG] tmkms::session: test-yubi: Connecting to 127.0.0.1:26658...
13:18:55 [INFO] [test-yubi@tcp://127.0.0.1:26658] connected to validator successfully

sign_bytes for SignProposal with the Yubi:

[119, 8, 32, 17, 1, 0, 0, 0, 0, 0, 0, 0, 33, 255, 255, 255, 255, 255, 255, 255, 255, 42, 72, 10, 32, 109, 184, 47, 185, 69, 249, 99, 187, 134, 206, 136, 198, 23, 145, 171, 194, 52,
8, 17, 118, 206, 143, 191, 25, 150, 215, 173, 178, 131, 254, 28, 94, 18, 36, 10, 32, 148, 8, 254, 116, 222, 40, 160, 178, 114, 15, 62, 50, 10, 187, 33, 66, 206, 190, 39, 140, 25, 50, 56, 61, 158, 0, 54, 81, 122, 146, 19, 15, 16, 1, 50, 12
, 8, 230, 185, 149, 227, 5, 16, 240, 150, 158, 195, 3, 58, 9, 116, 101, 115, 116, 45, 121, 117, 98, 105]

sign_bytes for SignProposal with the Ledger given that all yubihsm stuff is compiled as well:

[119, 8, 32, 17, 1, 0, 0, 0, 0, 0, 0, 0, 33, 255, 255, 255, 255, 255, 255, 255, 255, 42, 72, 10, 32, 191, 103, 198, 145, 194, 126, 71, 202, 43, 138, 157, 77, 130, 186, 161, 188, 97,
 53, 37, 92, 124, 110, 78, 222, 47, 18, 148, 103, 202, 210, 142, 122, 18, 36, 10, 32, 229, 245, 202, 169, 32, 97, 200, 158, 211, 240, 106, 182, 197, 35, 14, 157, 114, 199, 37, 118, 2, 22, 36, 232, 216, 110, 41, 153, 215, 79, 219, 39, 16,
1, 50, 12, 8, 190, 213, 149, 227, 5, 16, 248, 190, 156, 140, 3, 58, 9, 116, 101, 115, 116, 45, 121, 117, 98, 105]

However on the above sign_bytes the ledger fails as you can see from the first log from tmkms.

adrianbrink commented 5 years ago

However for SignVote with compiled ledger and yubihsm features the sign_bytes are wrong.

13:18:57 [DEBUG] tmkms::session: got sign vote request
13:18:57 [DEBUG] tmkms::session: got sign request
13:18:57 [DEBUG] tmkms::session: sign_bytes for request: [36, 8, 1, 17, 1, 0, 0, 0, 0, 0, 0, 0, 42, 12, 8, 193, 213, 149, 227, 5, 16, 176, 252, 185, 140, 3, 50, 9, 116, 101, 115, 116, 45, 121, 117, 98, 105]
13:18:57 [DEBUG] tmkms::keyring: Successfully got signer and now trying to sign message
Ledger is trying to sign: [36, 8, 1, 17, 1, 0, 0, 0, 0, 0, 0, 0, 42, 12, 8, 193, 213, 149, 227, 5, 16, 176, 252, 185, 140, 3, 50, 9, 116, 101, 115, 116, 45, 121, 117, 98, 105]
Received an error
13:18:57 [ERROR] [test-yubi@tcp://127.0.0.1:26658] signing operation failed: received an invalid signature: received an invalid signature
13:18:58 [INFO] KMS node ID: 3DD79F0F25B3F71423DABEF47F43BAF3ABEEC68B

I can't explain why the conditional compilation of the yubihsm or the ledger would cause different sign_bytes to appear.

jleni commented 5 years ago

Yes, but still.. that first byte is unexpected and out of spec. I am looking at tendermint-rs right now.

adrianbrink commented 5 years ago

Oh 119 is not in the spec?

jleni commented 5 years ago

well.. at least the spec for how votes/proposals where going to be serialized. https://github.com/cosmos/ledger-cosmos/blob/aaae52d2692a586eae171577152378c3b756593f/tests/val/vote_parser.cpp#L83

The ledger will expect field_number and later content

liamsi commented 5 years ago

It seems like the ledger test-vectors are not up to date. That is easy to fix. The other problem (sign bytes seem to differ depending on if compiled with yubihsm or not) is rather mysterious tho.

jleni commented 5 years ago

Ok, the issue is now clear :) When serializing, the code is using cp.encode_length_delimited(sign_bytes)?; so the first "magic" byte is the number of bytes. we need to drop that when sending to the HSMs

jleni commented 5 years ago

The issue is in this line https://github.com/tendermint/kms/blob/752184b5bfa139cde4c9013d836e0f3cda897110/tendermint-rs/src/amino_types/proposal.rs#L118

jleni commented 5 years ago

I will write a fix + additional test and check other types too.

liamsi commented 5 years ago

OK, wait: In the test-vectors you've linked MarshalBinaryBare is used (see https://github.com/tendermint/tendermint/blob/28d75ec8016b7fb043735cbe4c4d92ec73355de7/types/vote_test.go#L99-L138), the actual sign-bytes use

func (vote *Vote) SignBytes(chainID string) []byte {
    bz, err := cdc.MarshalBinaryLengthPrefixed(CanonicalizeVote(chainID, vote))
    if err != nil {
        panic(err)
    }
    return bz
}

which corresponds to encode_length_delimited (see: https://github.com/tendermint/tendermint/blob/28d75ec8016b7fb043735cbe4c4d92ec73355de7/types/vote.go#L72-L78).

jleni commented 5 years ago

ok, so should I then update the test vectors and the spec for the ledger app?

liamsi commented 5 years ago

Yes, sorry that the test vectors skip the length. I think initially they were length encoded. Must have been changed in PR (in tm).

adrianbrink commented 5 years ago

This is amazing. Thank you very much for looking into this.

@Liamsi What is the best way to investigate why the YubiHSM feature is needed for correct compilation?

liamsi commented 5 years ago

@jleni: I'll submit a PR to tendermint to match the actual sign bytes.

@Liamsi What is the best way to investigate why the YubiHSM feature is needed for correct compilation?

No idea. I'll look into it after the PR.

liamsi commented 5 years ago

https://github.com/tendermint/tendermint/pull/3312

liamsi commented 5 years ago

I can't reproduce why the bytes differ depending on how you compile (tried with all features available here). Can you elaborate a bit how you've compiled it?

adrianbrink commented 5 years ago

Cargo.toml

signatory = { version = "0.11", features = ["ed25519"] }
signatory-dalek = "0.11"
signatory-yubihsm = { version = "0.11", optional = true }
signatory-ledger-cosval = { version = "0.11", optional = true }
subtle-encoding = "0.3"
tendermint = { version = "0.2", path = "tendermint-rs" }

[patch.crates-io]
signatory = { path = "/Users/adrian/code/tendermint/signatory" }
signatory-dalek = { path = "/Users/adrian/code/tendermint/signatory/signatory-dalek" }
signatory-yubihsm = { path = "/Users/adrian/code/tendermint/signatory/signatory-yubihsm" }
signatory-ledger-cosval = { path = "/Users/adrian/code/tendermint/signatory/signatory-ledger-cosval" }

[dev-dependencies]
tempfile = "3"
rand = "0.6"

[features]
default = ["softsign", "yubihsm"]
ledger = ["signatory-ledger-cosval"]
softsign = []
yubihsm = ["signatory-yubihsm/usb"] # USB only for now
yubihsm-mock = ["yubihsm", "signatory-yubihsm/mockhsm"]

# Enable integer overflow checks in release builds for security reasons
[profile.release]
overflow-checks = true

I added logging output to session.rs/sign().

    /// Perform a digital signature operation
    fn sign<T: TendermintRequest + Debug>(&mut self, mut request: T) -> Result<Response, KmsError> {
        debug!("got sign request");
        request.validate()?;

        let mut to_sign = vec![];
        request.sign_bytes(self.chain_id, &mut to_sign)?;
        debug!("sign_bytes for request: {:?}", to_sign);

        // TODO(ismail): figure out which key to use here instead of taking the only key
        // from keyring here:
        let sig = KeyRing::sign(None, &to_sign)?;

        request.set_signature(&sig);
        debug!("successfully signed request:\n {:?}", request);
        Ok(request.build_response())
    }

I'm logging the sign_bytes and below I'll paste the sign_bytes.

Compiling with YubiHSM enabled

  1. Compile tmkms with cargo build, which by default builds the default features including the yubihsm.
  2. Setup a local 1 node testnet.
  3. Start tmkms with ./target/debug/tmkms start -c tmkms.toml -v
  4. Start gaiad

tmkms.toml

# Example KMS configuration file
#
# Copy this to 'tmkms.toml' and edit for your own purposes

## Yubi ##
[[validator]]
addr = "tcp://127.0.0.1:26658" # or "unix:///path/to/socket"
chain_id = "test-yubi"
reconnect = true # true is the default
secret_key = "/Users/adrian/.tmkms/secret_connection.key"

[[providers.yubihsm]]
adapter = { type = "usb" }
auth = { key = 1, password = "password" } # Default YubiHSM admin credentials. Change ASAP!
keys = [{ id = "test-yubi", key = 42 }]
serial_number = "0007550214" # identify serial number of a specific YubiHSM to connect to

Truncated output from tmkms:

Compiling with Ledger enabled but without YubiHSM

  1. Compile tmkms with cargo build --features ledger
  2. Setup a local 1 node testnet.
  3. Start tmkms with ./target/debug/tmkms start -c tmkms.toml -v
  4. Start gaiad

tmkms.toml

# Example KMS configuration file
#
# Copy this to 'tmkms.toml' and edit for your own purposes

# ## Ledger ##
[[validator]]
addr = "tcp://127.0.0.1:26658" # or "unix:///path/to/socket"
chain_id = "test-ledger"
reconnect = true # true is the default
secret_key = "/Users/adrian/.tmkms/secret_connection.key"

[[providers.ledger]]
active = true

Truncated output from tmkms

The reason why the Ledger doesn't return a proper signature have been discussed above and will hopefully be fixed soon.

However, I don't understand why the first byte is different in the SignProposal Request is different and why the bytes for the SignVote Request are completely different. As far as I can tell nothing in the code leading up until the point of calling 

        let mut to_sign = vec![];
        request.sign_bytes(self.chain_id, &mut to_sign)?;
        debug!("sign_bytes for request: {:?}", to_sign);

is branching on whether the YubiHSM or Ledger feature is enabled. The only branching behaviour depending on the backend should happen in.

        // TODO(ismail): figure out which key to use here instead of taking the only key
        // from keyring here:
        let sig = KeyRing::sign(None, &to_sign)?;
jleni commented 5 years ago

Quick update:

adrianbrink commented 5 years ago

Fantastic :-) I can't wait to test the entire pipeline.

I also created #172 which integrates the current ledger version into it. It needs updating with your changes though or we can discard it and pull in yours.

jleni commented 5 years ago

I am happy to take it and apply my changes on top. I will do it tomorrow morning then.

liamsi commented 5 years ago

I think this can be closed. This was fixed here: https://github.com/tendermint/signatory/pull/141/. And as far as I can see, the original concerns raised by Adrian (that this does depend on with which feature you compile with) do not hold.

Feel free to reopen if there is still an issue.