Add support for HSM

[gmilescu]

Support for:

• YubiHSM
• Ledger
• Cloud HSM on AWS

For this, we need new type of Signer and configuration to specify it.

ND-136 created by None

[gmilescu]

ilblackdragon commented:

Implementation details:

• Implement a `RESTSigner` that implements `Signer` trait and uses sync client to fetch signature via REST API.
• Investigate the exact APIs that YubiHSM/Ledger/Cloud HSM supports.
• Extra: investigate changing `Signer` to async.

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

ailisp commented:

I assume no one start on this yet, so I'm taking this start working on this Friday. If someone already start work on this I'll step down.

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

teodor-pripoae commented:

The signer trait does not know which data it previously signed. If you are going to implement external signer I think you may need to change the trait to include information such as block height, data, etc.

By using an external signer it can be exploited by an attacker to trigger double signing by calling the REST API. The external signer should keep track of what blocks it signed and avoid signing the same block again.

pub trait Signer: Sync + Send {

    fn public_key(&self) -> PublicKey;

    fn sign(&self, data: &[u8]) -> Signature;

    fn verify(&self, data: &[u8], signature: &Signature) -> bool 
{
signature.verify(data, &self.public_key())
}
    /// Used by test infrastructure, only implement if make sense for testing otherwise raise `unimplemented`.

    fn write_to_file(&self, path: &Path);

}

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

ilblackdragon commented:

@teodor-pripoae I see the value for the signer to understand the height of the block. What are the risk factors you are trying to address?

Because if node is compromised, they will just send REST call with invalid block height in that parameter but with data that does double signing. Same with issues in the code.

Vs I would expect to just read out the height from `data` on the signer side. For a specific version of protocol, height will be at a fixed offset in the data so don't even need to parse the whole thing.

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

teodor-pripoae commented:

Isn't the block id also in the payload that is signed ? If the signer keeps track of what height it previously signed it cannot sign 2 payloads at the same height.

For tendermint for example the signer checks the last signed payload and it refuses to sign a height less than previously signed height

func (pv *FilePV) signVote(chainID string, vote *types.Vote) error {

    height, round, step := vote.Height, vote.Round, voteToStep(vote)

 lss := pv.LastSignState
 sameHRS, err := lss.CheckHRS(height, round, step)

    if err != nil 
{
return err
}



signBytes := vote.SignBytes(chainID)



// We might crash before writing to the wal,

// causing us to try to re-sign for the same HRS.

// If signbytes are the same, use the last signature.

// If they only differ by timestamp, use last timestamp and signature

// Otherwise, return error

if sameHRS {

if bytes.Equal(signBytes, lss.SignBytes) {
vote.Signature = lss.Signature
} else if timestamp, ok := checkVotesOnlyDifferByTimestamp(lss.SignBytes, signBytes); ok {
vote.Timestamp = timestamp
vote.Signature = lss.Signature
} else {
err = fmt.Errorf("conflicting data")
}

return err

}



// It passed the checks. Sign the vote

sig, err := pv.Key.PrivKey.Sign(signBytes)

if err != nil {     return err  }
 pv.saveSigned(height, round, step, signBytes, sig)

    vote.Signature = sig

    return nil

}

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

teodor-pripoae commented:

I'm currently working on an external signer for tendermint which permits active-active high availability setup. Multiple validators send payloads to the signer raft cluster and only one of the requests for the same block gets signed, the other are refused.

Using this setup even if the validator node gets compromised the attacker can only DoS the system, it cannot double sign.

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

teodor-pripoae commented:

I'm was curious if I can adapt it to also work for near protocol.

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

ilblackdragon commented:

So on our side, we need to do next things:

1. Add ValidatorSigner:

trait ValidatorSigner

{ fn sign_block_header(&self, header: BlockHeader) -> Signature; fn sign_approval(&self, approval: Approval) -> Signature; }

and change Client to use it instead of current signer.

1. Add InMemory implementation for `ValidatorSigner` that Borsh-ifys and hashes the data and signs.

1. Add RPC version, that would Borsh-ify the data and then call the required end point on given RPC.

1. Connect with @teodor-pripoae 's implementation on HSM for example.

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

ailisp commented:

starting to work on this today

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

ailisp commented:

Now chunk is there, so something more need to be signed:

BlockHeader
Approval
ChallengeBody
Transaction
ShardChunkHeader

However do we really need sign these structs? In fact everything above we sign is a hash of the struct or hash or partial struct. So I thought it's okay to have only one API: `sign(hash: &[u8], prev_block_hash: CryptoHash)` prev block isn't always available in above context, but prev_block_hash is and i guess it's possible for REST signer to keep a block_hash<>block_index mapping?

In further, since the thing we sign is always a hash, is it really necessary to avoid double sign with additional prev_block_hash info? Can RESTSigner keep a sizedcache/timedcache of hashes it signs before? @ilblackdragon

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

ailisp commented:

@ilblackdragon Still need confirmation above. I believe a trait of sign(hash: &[u8], prev_block_hash: CryptoHash) is enough, or even current sign(hash: &[u8]) is enough since hash is trivial, for signer it doesn't hurt to sign this hash again even it's signed before

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

janewang commented:

Requested by current validators the physical / YubiHSM. Bringing into the NodeX board: https://github.com/orgs/near/projects/18

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

stale[bot] commented:

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

stale[bot] commented:

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

[gmilescu]

Closing as duplicate of #1026.