-
Notifications
You must be signed in to change notification settings - Fork 124
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
feat: add vault transit kms engine #44
Conversation
b96c1f0
to
ce672ce
Compare
This is great! |
Signed-off-by: Richard Simpson <[email protected]>
924b480
to
913c4af
Compare
Verified. Edit: I did slightly modify registration some more. E2E Testspackage main
import (
"context"
"crypto/ecdsa"
"crypto/sha256"
"fmt"
"os"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
"github.com/sigstore/sigstore/pkg/kms"
vault "github.com/hashicorp/vault/api"
)
type VaultSuite struct {
suite.Suite
}
func (suite *VaultSuite) GetProvider(key string) kms.KMS {
provider, err := kms.Get(context.Background(), fmt.Sprintf("hashivault://%s", key))
require.NoError(suite.T(), err)
require.NotNil(suite.T(), provider)
return provider
}
func (suite *VaultSuite) SetupSuite() {
client, err := vault.NewClient(&vault.Config{
Address: os.Getenv("VAULT_ADDR"),
})
require.Nil(suite.T(), err)
require.NotNil(suite.T(), client)
err = client.Sys().Mount("transit", &vault.MountInput{
Type: "transit",
})
require.Nil(suite.T(), err)
}
func (suite *VaultSuite) TearDownSuite() {
client, err := vault.NewClient(&vault.Config{
Address: os.Getenv("VAULT_ADDR"),
})
require.Nil(suite.T(), err)
require.NotNil(suite.T(), client)
err = client.Sys().Unmount("transit")
require.Nil(suite.T(), err)
}
func (suite *VaultSuite) TestProviders() {
providers := kms.ProvidersMux().Providers()
assert.Len(suite.T(), providers, 2)
}
func (suite *VaultSuite) TestProvider() {
suite.GetProvider("provider")
}
func (suite *VaultSuite) TestCreateKey() {
provider := suite.GetProvider("createkey")
key, err := provider.CreateKey(context.Background())
assert.Nil(suite.T(), err)
assert.NotNil(suite.T(), key)
}
func (suite *VaultSuite) TestSign() {
provider := suite.GetProvider("testsign")
key, err := provider.CreateKey(context.Background())
assert.Nil(suite.T(), err)
assert.NotNil(suite.T(), key)
data := []byte("mydata")
sig, signed, err := provider.Sign(context.Background(), data)
assert.Nil(suite.T(), err)
assert.NotNil(suite.T(), sig)
assert.NotNil(suite.T(), signed)
hash := sha256.Sum256(signed)
ok := ecdsa.VerifyASN1(key, hash[:], sig)
assert.True(suite.T(), ok)
}
func (suite *VaultSuite) TestVerify() {
provider := suite.GetProvider("testverify")
key, err := provider.CreateKey(context.Background())
assert.Nil(suite.T(), err)
assert.NotNil(suite.T(), key)
data := []byte("mydata")
sig, signed, err := provider.Sign(context.Background(), data)
assert.Nil(suite.T(), err)
assert.NotNil(suite.T(), sig)
assert.NotNil(suite.T(), signed)
err = provider.Verify(context.Background(), data, sig)
assert.Nil(suite.T(), err)
}
func (suite *VaultSuite) TestNoProvider() {
provider, err := kms.Get(context.Background(), "hashi://nonsense")
require.Error(suite.T(), err)
require.Nil(suite.T(), provider)
}
func TestVault(t *testing.T) {
suite.Run(t, new(VaultSuite))
} |
Do you want to add the e2e tests here as well? We might even be able to get the vault docker container running automatically in github actions, but we can handle that as a follow on. |
I'd def be game to contribute them, I just wasn't sure how ya'll wanted to handle E2E (especially since vault might be a one off case where E2E is "easy"). I'm actually using the vanilla Vault docker image locally, so that'd def work. |
Signed-off-by: Richard Simpson <[email protected]>
I've done e2e before with vault (https://github.com/hashicorp/vault-action/blob/master/.github/workflows/build.yml#L62) so luckily I already know all the component parts we need :D. Edit: AWS KMS e2e might be possible when I add that via https://hub.docker.com/r/localstack/localstack so I could be game to go ahead and setup the infra for it here |
Signed-off-by: Richard Simpson <[email protected]>
Signed-off-by: Richard Simpson <[email protected]>
Signed-off-by: Richard Simpson <[email protected]>
Signed-off-by: Richard Simpson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! A few nits that I wouldn't normally block on, but since you have to cleanup the golangci-lint issues anyway, might as well :)
Makefile
Outdated
go test ./pkg/... | ||
|
||
test-e2e: | ||
go test ./test/e2e/... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: in other places here we use a build tag for this so you can still run "go test ./..." at the root: https://github.com/sigstore/cosign/blob/main/test/e2e_test.go#L16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh, noted! I'm still a bit of a golang novice, I was wondering how I could do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random addendum: This still scopes the e2e test run to this subpackage. I figured it was somewhat wasteful to run the whole test suite again just to run e2e
pkg/kms/hashivault/hashivault.go
Outdated
func (g *KMS) CreateKey(ctx context.Context) (*ecdsa.PublicKey, error) { | ||
client := g.client.Logical() | ||
|
||
_, err := client.Write(fmt.Sprintf("/transit/keys/%s", g.keyPath), map[string]interface{}{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: inline the if statement?
pkg/kms/hashivault/hashivault.go
Outdated
func (g *KMS) PublicKey(ctx context.Context) (crypto.PublicKey, error) { | ||
client := g.client.Logical() | ||
|
||
_, err := client.Write(fmt.Sprintf("/transit/keys/%s", g.keyPath), map[string]interface{}{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: inline?
pkg/kms/hashivault/hashivault.go
Outdated
return nil | ||
} | ||
|
||
var vaultV1DataPrefix = "vault:v1:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: const and put at the top of the file?
Signed-off-by: Richard Simpson <[email protected]>
b8cd68c
to
7e079f4
Compare
Signed-off-by: Richard Simpson <[email protected]>
Signed-off-by: Richard Simpson <[email protected]>
0aa0488
to
75f1a56
Compare
Addressed feedback (thanks!) and when ahead and added a local run script for e2e like cosign. |
Signed-off-by: Richard Simpson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the awesome PR!
* Start adding support for uploading to transparency log after sign Signed-off-by: Priya Wadhwa <[email protected]> * Verify log exists in transparency log when TLOG=1 Signed-off-by: Priya Wadhwa <[email protected]> * Change func name to Upload to match rekor Signed-off-by: Priya Wadhwa <[email protected]> * fix int test Signed-off-by: Priya Wadhwa <[email protected]> * fix lint, use not deprecated hasher Signed-off-by: Priya Wadhwa <[email protected]> * use public key from private key for 'cosign sign', use REKOR_SERVER env var to match rekor project Signed-off-by: Priya Wadhwa <[email protected]> * revert test Signed-off-by: Priya Wadhwa <[email protected]> * Add integration test for tlog support Sets up a local rekor server via the provided docker-compose.yaml file in the rekor repo. I tested to make sure this test fails if the local server isn't running, which is the case. Signed-off-by: Priya Wadhwa <[email protected]>
Adds a KMS option based off Vault Transit. Also slightly reorganized how providers are registered, inspired in part by go-cloud.
Related: sigstore/cosign#131