ecadlabs / signatory

Signatory - A Tezos Remote Signer for signing block-chain operations with private keys using YubiHSM, AWS, GCP, Ledger's or Azure Key Vault
https://signatory.io
Apache License 2.0
61 stars 18 forks source link

hashicorp vault support #439

Closed denisandreenko closed 1 year ago

denisandreenko commented 1 year ago

πŸš€ Pull Request: New Integration with HashiCorp Vault Transit for Tezos Transaction Signing

πŸ“ Description:

This pull request introduces an exciting new feature that enhances service's flexibility. I've integrated HashiCorp Vault's Transit Secrets Engine to manage the signing of Tezos transactions.

πŸ› οΈ Implementation Details:

πŸ”’ Security Considerations:

πŸ“‘ Documentation Updates:

Please review the code changes, test results, and documentation updates. Your feedback and suggestions are greatly appreciated!

πŸ™Œ Thank you for your attention and support!

stephengaudet commented 1 year ago

thank you for your contribution @denisandreenko I'll have this PR tested shortly

stephengaudet commented 1 year ago

hi @denisandreenko

Ideally, test results would be communicated in an Issue, and not in a PR. Please give a read of the contributions section of the readme https://github.com/ecadlabs/signatory#contributions

currently the signatory-cli binary does not recognize the new vault driver. the signatory binary starts and authenticates with the configured vault, but, the signatory-cli fails to execute using the same signatory.yaml config file

% docker exec signatory signatory-cli list time="2023-08-30T21:30:55Z" level=info msg="Initializing vault" vault=hashicorpvault vault_name=hashicorp Error: unknown vault driver: hashicorpvault

with the same file, signatory is fine: time="2023-08-30T21:25:44Z" level=info msg="Initializing vault" vault=hashicorpvault vault_name=hashicorp time="2023-08-30T21:25:44Z" level=info msg="Public Key Hash: tz1Nt4qojbMELqTmQHtvpQJN9y53kuadrVTd" time="2023-08-30T21:25:44Z" level=info msg="Vault: HASHICORP_VAULT" time="2023-08-30T21:25:44Z" level=info msg="ID: tz1key" time="2023-08-30T21:25:44Z" level=info msg="Active: false"

denisandreenko commented 1 year ago

Hi @stephengaudet βœ‹

Thanks for the hint, I'll do the following PR through issues.

Added HCP init for Signatory CLI

stephengaudet commented 1 year ago

hi @denisandreenko πŸ‘‹ the integration test I've developed gets some decent coverage of the new vault with a statement coverage of 69.4%. the other 30% is mostly error handling. with 2 exceptions.

my tests are not reaching the following 2 functions: vault_transit.go GetKey vault_transit.go Verify

Are these unreachable code?

github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault.go:44: PublicKey 100.0% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault.go:49: ID 100.0% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault.go:60: init 66.7% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault.go:79: New 73.3% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault.go:120: Name 100.0% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault.go:124: login 66.7% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault.go:142: ListPublicKeys 100.0% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault.go:149: Next 81.8% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault.go:171: GetPublicKey 69.2% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault.go:201: SignMessage 75.0% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault_transit.go:27: Transit 100.0% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault_transit.go:32: ListKeys 75.0% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault_transit.go:60: GetKey 0.0% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault_transit.go:64: GetKeyWithContext 100.0% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault_transit.go:68: getKey 68.8% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault_transit.go:98: Sign 70.0% github.com/ecadlabs/signatory/pkg/vault/hashicorp/vault_transit.go:120: Verify 0.0% total: (statements) 69.4%

denisandreenko commented 1 year ago

Hi @stephengaudet πŸ‘‹

Thank you so much for your work!

That's right, these methods are not used at the moment, however they can be very useful for users who want to check the signed message for validity. So we can remove them so as not to mislead, or we can add an additional API which will allow to verify if messages were signed by a particular key.

stephengaudet commented 1 year ago

thanks @denisandreenko ! My preference would be to remove the unused functions. that said, I'm not here to stifle innovation. :) If your preference is to keep them, then I would ask you to write unit tests for them. noteworthy, the octez-client verifies signatures it gets from Signatory, so, the use case for Signatory providing a verification is unclear

denisandreenko commented 1 year ago

thanks @denisandreenko ! My preference would be to remove the unused functions. that said, I'm not here to stifle innovation. :) If your preference is to keep them, then I would ask you to write unit tests for them. noteworthy, the octez-client verifies signatures it gets from Signatory, so, the use case for Signatory providing a verification is unclear

For cases when octez-client is not used or if we want to check it before sending a transaction to the blockchain. But still, at the moment I have removed these methods, as they may be redundant for the current pull request

denisandreenko commented 1 year ago

Hi @stephengaudet πŸ‘‹

Are there any other comments?

denisandreenko commented 1 year ago

Thanks @stephengaudet πŸ‘

Did a rebase and merged conflicts. @e-asphyx could you please review PR