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 feature tmkms #840

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

mkaczanowski
Copy link
Contributor

TL;DR

This is a rebased version of #613 with some changes:

  1. integration tests works now
  2. VAULT_CACERT and VAULT_SKIP_VERFIY are now configurable
  3. config changed slightly

Test plan

I've tested this live and also via unittests and integration test:

$ ./tests/support/start_vault.sh

$ cargo test --features hashicorp,softsign
...
test result: ok. 12 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.20s

@mkaczanowski
Copy link
Contributor Author

mkaczanowski commented Jan 13, 2024

Huh, there is no fix for: https://rustsec.org/advisories/RUSTSEC-2023-0071 yet

(but it is optional dependency anyway)

EDIT: I need to fix the CI tests (integration test requires running vault)

@mkaczanowski
Copy link
Contributor Author

@tony-iqlusion it's ready for review 🙏

@helder-moreira
Copy link

@mkaczanowski may I ask if you considered vault disconnection issues? Will it reconnect? I noticed that even if I have multiple vault instances, when I restart one of them, tmkms wont try to connect to another. But this is an old version in my fork, maybe its addressed already.

@mkaczanowski
Copy link
Contributor Author

I was unable to reproduce the connection issues

@helder-moreira
Copy link

@tony-iqlusion any ETA on merging this?

@tony-iqlusion
Copy link
Member

I should have some time to review it soon. Please be patient.

@tony-iqlusion
Copy link
Member

Note: I would still like to get this into the v0.14 release but my time on TMKMS has been taken up by vote extension signing support. I hope to be able to review it soon when other TMKMS-related work is done.

@helder-moreira
Copy link

@mkaczanowski I am currently testing this PR, and it seems that CA certificate does not work:

Message:  Unable to connect to Vault at https://vault.vault.svc.cluster.local:8200
Location: src/commands/hashicorp/upload.rs:145

Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.

Works fine if I set vault_skip_verify = true. Can you double check this?

@mkaczanowski
Copy link
Contributor Author

@helder-moreira you can try now. I did rework bunch of things (as u can see in the commit history).

I'll be testing it live soon, but code wise I think it is pretty much done

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.

4 participants