Skip to content

Add VaultSigner and tests #800

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

Merged
merged 1 commit into from
May 2, 2024
Merged

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Apr 26, 2024

Adds fully functional VaultSigner and basic "import_ -> from_priv_key_uri
-> sign -> verify" test loop using a local vault server.

Tests run on CI and can be run locally with tox, if vault is installed.

Note also that vault supports all key types and signing schemes supported by
SSlibKey. The here added VaultSigner, however, only supports ed25519
keys.

Other key types and schemes may be added in follow-up PRs

Fixes #307

@jku
Copy link
Collaborator

jku commented Apr 26, 2024

  • fix ci failure (the test actually passes, but the vault tear down script seems to exit with non-zero and thus fails the job).

143 is what docker does when killed I think...

My guess is the initial vault server command has a signal handler that does exit 143

@lukpueh
Copy link
Member Author

lukpueh commented Apr 29, 2024

143 is what docker does when killed I think...

AFAIU not only on docker, but generally on linux. If a command receives SIGTERM it should exit with 143.

My guess is the initial vault server command has a signal handler that does exit 143

I think so too. But I cannot reproduce. I don't currently have a linux box at hand, and on my Mac the vault command does not exit with 143 when killed. (Interestingly, a different test command (tail -f /dev/null) indeed exits with 143 when killed).

I tried two independent workarounds to no avail:

# Start vault in background. `|| true` eats the non-zero exit code, when the
# background process is killed, so that it does not fail the Github Action.
{ vault server -dev -dev-root-token-id="${VAULT_TOKEN}" & } || true

# make sure to exit with 0
while kill -0 $pid
do
sleep 1
done
exit 0

Any ideas, @jku?

@lukpueh
Copy link
Member Author

lukpueh commented Apr 29, 2024

My guess is the initial vault server command has a signal handler that does exit 143

I think so too. But I cannot reproduce. I don't currently have a linux box at hand, and on my Mac the vault command does not exit with 143 when killed. (Interestingly, a different test command (tail -f /dev/null) indeed exits with 143 when killed).

I tried to debug this inside the GHA runner (with tmate) and vault exits with 0 there too when killed. This at least explains why my workarounds did not work.

@jku
Copy link
Collaborator

jku commented Apr 29, 2024

Now that I think about it, I was probably wrong and this actually kills tox instead of "vault server":

pid=$(pgrep -f vault)
kill $pid

EDIT: yes, I tested this with a slightly modified branch: this does indeed kill tox.

maybe pgrep -f "vault server"?

@lukpueh
Copy link
Member Author

lukpueh commented Apr 29, 2024

... actually kills tox

🤦 or the stop-vault.sh

Adds fully functional VaultSigner and basic "import_ -> from_priv_key_uri
-> sign -> verify" test loop using a local vault server.

Tests run on CI and can be run locally with tox, if vault is installed.

Note also that vault supports all key types and signing schemes supported by
SSlibKey. The here added VaultSigner, however, only supports `ed25519`
keys.

Other key types and schemes may be added in follow-up PRs.

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh lukpueh changed the title Add VaultSigner and tests (WIP) Add VaultSigner and tests May 2, 2024
@lukpueh lukpueh marked this pull request as ready for review May 2, 2024 10:01
@lukpueh
Copy link
Member Author

lukpueh commented May 2, 2024

@jku, this is ready for review. But we don't need to include it in 1.0.

Copy link
Collaborator

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


# Test sign and verify
signature = signer.sign(b"DATA")
self.assertIsNone(public_key.verify_signature(signature, b"DATA"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: assert seems unnecessary

Suggested change
self.assertIsNone(public_key.verify_signature(signature, b"DATA"))
public_key.verify_signature(signature, b"DATA")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷 I wanted to make it clear that this is a test scenario, and not some random function call that could return e.g. True or False and pass the test either way.

@jku
Copy link
Collaborator

jku commented May 2, 2024

we don't need to include it in 1.0.

Up to you, no strong feelings here.

@lukpueh
Copy link
Member Author

lukpueh commented May 2, 2024

we don't need to include it in 1.0.

Up to you, no strong feelings here.

In that case, let's ship it.

@lukpueh lukpueh merged commit 5789578 into secure-systems-lab:main May 2, 2024
13 checks passed
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.

Enable signing and verifying using keys stored in Hashicorp Vault
2 participants