Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hashicorp vault support #439

Merged

Conversation

denisandreenko
Copy link
Contributor

🚀 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:

  • The integration utilizes HashiCorp Vault's Transit Secrets Engine to handle cryptographic operations.
  • Each key management system is supported through a distinct plugin, ensuring compatibility and flexibility.
  • Proper error handling and logging mechanisms are in place to facilitate troubleshooting and maintenance.

🔒 Security Considerations:

  • Access to Vault and its policies is strictly controlled, limiting who can perform cryptographic operations.
  • The integration leverages AppRole authentication with fine-grained policies to ensure least privilege access.
  • All secrets and sensitive configuration parameters are securely stored using Vault's capabilities.

📑 Documentation Updates:

  • I've updated service documentation to include detailed guides on how to set up and configure the integration with Hashicorp Vault

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 stephengaudet requested a review from e-asphyx August 21, 2023 23:45
@stephengaudet
Copy link
Contributor

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

@stephengaudet
Copy link
Contributor

stephengaudet commented Aug 30, 2023

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
Copy link
Contributor Author

Hi @stephengaudet

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

Added HCP init for Signatory CLI

@stephengaudet
Copy link
Contributor

stephengaudet commented Sep 1, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

stephengaudet commented Sep 1, 2023

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
Copy link
Contributor Author

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

@stephengaudet stephengaudet self-requested a review September 8, 2023 19:06
@denisandreenko
Copy link
Contributor Author

Hi @stephengaudet 👋

Are there any other comments?

Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
@denisandreenko denisandreenko force-pushed the feature/hashicorp_vault_support branch from ac1a924 to 8a03867 Compare September 12, 2023 19:42
@denisandreenko
Copy link
Contributor Author

Thanks @stephengaudet 👍

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

@stephengaudet stephengaudet merged commit ddf37fa into ecadlabs:main Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants