VAULT KMS signing and verification broken after rotating a key in vault

[BerndFarkaDyna]

The Vault transit engine has a fundamental feature for rotating keys (https://www.vaultproject.io/docs/secrets/transit), therefore each digest returned from vault has a prefix identifying the version of the used key. For example vault:v3:XXXXXXX links version 3 of the key. The version of course is important for the verification.

Detailed analysis:

https://github.com/sigstore/sigstore/blob/245ddc285f5a25d3b2e1e87dd5f19b0bf7e942f3/pkg/signature/kms/hashivault/client.go

func vaultDecode(data interface{}) ([]byte, error)

removes the "vault:vN" prefix from the digest and

func (h hashivaultClient) verify(sig, digest []byte, alg crypto.Hash) error

Adds a predefined prefix to the secret.

Possible solution: Do not remove and add the prefix again, simply store the whole secret returned from vault.

[hectorj2f]

Hello @BerndFarkaDyna

Yes, it is correct that the version is removed from the signature. Have you tried to use VAULT_KEY_PREFIX ? You could replace the hardcoded/default value vault:v1 by vault:v3 as you defined when verifying it.

[BerndFarkaDyna]

@hectorj2f THX for you response!

I see various problems in defining it with VAULT_KEY_PREFIX:

• me as the verifier of an signature i most probably do not know the version it got signed
• older versions can still be valid for verification
• there can be mixed versions in an image, first n layers got signed with v1 and a new layer gets signed with v2 for example

AFAIK this could all be valid cases (from the vault point of view)

[hectorj2f]

@BerndFarkaDyna Indeed, it opens some valid use cases here.

[BerndFarkaDyna]

@hectorj2f: I would be open for helping implementing the fix, since i have a valid test-setup up and running.

The question for me is: what should be the solution? would adding the vault and version prefix to the digest a valid solution from your point of view?

[hectorj2f]

@BerndFarkaDyna That is a good question. Is there a way to get the last version ? Why would i want to use a rotated version of my key ?

[BerndFarkaDyna]

From the Vault Documentation (https://learn.hashicorp.com/tutorials/vault/eaas-transit#rotate-the-encryption-key)

One of the benefits of using the Vault transit secrets engine is its ability to easily rotate encryption keys. Keys can be rotated manually by a human or by an automated process which invokes the key rotation API endpoint through cron, a CI pipeline, a periodic Nomad batch job, Kubernetes Job, etc.

Vault maintains the versioned keyring and the admin can decide the minimum version allowed for decryption operations. When data is encrypted using Vault, the resulting ciphertext is prepended by the version of the key used to encrypt it.

The More general Answer would be:

The primary purpose of rotating encryption keys is not to decrease the probability of a key being broken, but to reduce the amount of content encrypted with that key so that the amount of material leaked by a single key compromise is less.

So basically it simply is a very good preventive measure if a key/secret/whatever gets leaked.

In my use-case it is simply part of "Best Practice" to not have very long living keys.

[hectorj2f]

@dlorenc I believe we need your opinion about this ☝🏻 ? wdyt ?

[dlorenc]

I don't know that we have a great answer here. The approach of storing the entire result from vault will work for rotation but it will break verification against an exported public key.

Could the prefix stuff be stored separately, and then used if present?

[BerndFarkaDyna]

From my point of view this could be a very valid solution.

After having a deeper look in the implementation this would need some bigger code change but feels like the most cleanest way.

But it might break the current API.

As addition we still could add the vault:v1 prefix if somebody tries to verify against vault and no prefix is stored (which would keep the compatibility to the current situation)

[dlorenc]

But it might break the current API.

If you have a sketch on what it would look like we can evaluate!

[BerndFarkaDyna]

I might have to rework mine a bit but that should not be a problem within the next few days.

but the changes will go to the sigtool and cosign repo -> do we need 2 issues there?

[dlorenc]

Nah - one tracking issue is fine!

[BerndFarkaDyna]

@dlorenc, @hectorj2f

Some thoughts from my side where you (hopefully) can share some thoughts from your side...

"github.com/sigstore/sigstore/pkg/cryptoutils"

type SignerVerifier interface {
    Signer
    Verifier
}

type Signer interface {
    PublicKeyProvider
    SignMessage(message io.Reader, opts ...SignOption) ([]byte, error)
}

type Verifier interface {
    PublicKeyProvider
    VerifySignature(signature, message io.Reader, opts ...VerifyOption) error
}

My Suggestion would be to change the return values of SignMessage to an struct which contains the byte value and a map[string]string which can save optional values (like the vault prefix in our case). this map can later on be used to write additional values to the image. Further the VerifySignature Function should be extended to accept this map for making the values accessible on verifying.

Overall this will make the whole API more complicated but i so no other way to archive the goal of storing the prefix in a dedicated field. IMHO it maybe should be reconsidered if this is a option you want to go and pay the price of this change vs storing the prefix in the signature.

Please feel free to take this comment as an open for further discussions...

[dlorenc]

Just thinking out loud here, this might not be a great idea (@mattmoor and @znewman01 and @bobcallaway might have better ones):

The generic pattern for that "prefix" is sometimes referred to as a KeyID. What if we make an optional extension to the SignerVerifier interface that looks something like:

SIgnerVerifierWithKeyID

It could include extensions to those methods:

SignMessageWithKeyID(message io.Reader, opts ...SignOption, keyID string) ([]byte, error

VerifySignatureWithKeyID(signature, message io.Reader, opts ...VerifyOption, keyID string) error

Then our code could do a type assertion to test whether or not the signer implementation supports key IDs, and use that one if present. We could augment the CLI to support setting a KeyID (and storing it as an annotation), that would require the use of a Signer that implements the KeyID extensions.

WDYT?

[BerndFarkaDyna]

Fine with me too of course!

[znewman01]

I'm not terribly familiar with Vault, but here are my initial thoughts.

We want to enable three use cases (both API and CLI):

1. Sign with latest version, check against "valid" versions.

   This could mean any version is okay, or we check that the key is >= some version.

    # Sign with latest key:
    cosign sign --kms hashivault://my-key <IMAGE>
    # Verify: <IMAGE> was signed with *any* key:
    cosign verify --kms hashivault://my-key <IMAGE>
    # Verify: <IMAGE> was signed with key v3 or later:
    cosign verify --kms hashivault://my-key@>=3 <IMAGE> # syntax needs work

2. Sign/verify with specific version.

   Here, we can just think of the version as part of the key URI:

    cosign sign --kms hashivault://my-key@3 <IMAGE>
    cosign verify --kms hashivault://my-key@3 <IMAGE>

   This is for a case where the user will manage key versions.

3. Sign/verify with exported public key.

   Here, we can just think of the version as part of the key URI:

    cosign sign --kms hashivault://my-key@3 <IMAGE>
    vault export my-key@3 > my-key.pub  # or however the `vault` CLI works
    cosign verify --key my-key.pub <IMAGE>

   This is for a case where the user will manage key versions.

It seems like the vault:v1 prefix is part of the ciphertext in this case so I'm inclined to say it should just be part of the bytes. It's a little ugly but without that I'm not sure how we could enable use case (1). And overall seems cleaner than extending the API.

I can't tell from a quick skim if that would break everything else in cosign, though. sounds like that breaks use case (3)

Edit: ETA use case (3) which @dlorenc brings up in the following comment

[dlorenc]

It seems like the vault:v1 prefix is part of the ciphertext in this case so I'm inclined to say it should just be part of the bytes.

It's returned at part of the ciphertext, but vault also supports exporting the plain public key for standard verification. If you leave the prefix in then, it breaks :(

[znewman01]

Ahh, that's what I was missing. Updated my comment above.

That said, it's a terrible hack but could we just make VerifySignature work as follows?

• if ciphertext starts with "vault:XXX:"
  • if key is a vault KMS key, check for "compatibility"
  • otherwise, strip "vault..." and proceed
• otherwise, if key is a vault KMS key: check signature with latest key in the vault
• otherwise, verify as usual

That might be a lot of magic for VerifySignature, which is supposed to be relatively straightforward. So if we're going to expand the API something like @dlorenc's proposal makes sense. My only quibble is that perhaps the keyID belongs in the return type of SignMessageWithKeyID, rather than the parameter list (the Transit docs suggest that you don't learn the key ID until you do the encryption).

[BerndFarkaDyna]

@znewman01:

I really doubt that

Sign/verify with specific version.

is a use-case at all.

For me as a user of a KMS, one of the reasons for choosing Vault is that i do not have to care about those versions on the client side at all. I can specify the the versions (valid for signing and verification) on vault itself. I would not think of any valid reason why doing this on the client.

Even on the Vaults Rest API for verification there is no parameter for setting the version since the version is part of the payload (cypher).

[BerndFarkaDyna]

Verifying the signature with an exported key:

1. Is this a use-case which really should be supported? The default of Vault is to forbidden the export and you have to enable this for certain keys.
2. If somebody enables that the user will have to keep on tracking the versions and distribute the public key.
3. AFAIK it could be the case that some layers are signed with different versions, how to handle that?

If this still is a valid use-case:

A possible solution could be: just store the KMS Type which was used to sign and add functionality to an interface (SignerVerifier?) to normalize the cypher.

Basically the suggestion would be, if somebody decides to verify with public key to check if was signed with an KMS and call the normalization before doing the verification.

[dlorenc]

Verifying the signature with an exported key:

1. Is this a use-case which really should be supported? The default of Vault is to forbid the export and you have to enable this for certain keys.

Yup, this is very common. These are public keys, not private ones, so exporting is common and is actually a good pattern to allow verification without access to vault or by a third party.

[bobcallaway]

Just thinking out loud here, this might not be a great idea (@mattmoor and @znewman01 and @bobcallaway might have better ones):

The generic pattern for that "prefix" is sometimes referred to as a KeyID. What if we make an optional extension to the SignerVerifier interface that looks something like:

SIgnerVerifierWithKeyID

It could include extensions to those methods:

SignMessageWithKeyID(message io.Reader, opts ...SignOption, keyID string) ([]byte, error

VerifySignatureWithKeyID(signature, message io.Reader, opts ...VerifyOption, keyID string) error

Then our code could do a type assertion to test whether or not the signer implementation supports key IDs, and use that one if present. We could augment the CLI to support setting a KeyID (and storing it as an annotation), that would require the use of a Signer that implements the KeyID extensions.

WDYT?

I'm thinking through a longer response, but wanted to mention this idea - rather than adding new methods, could we not just leverage the functional options pattern already in place:

• add an RPCOption like options.WithVaultKeyID(keyID string) to request using a specific keyID for signing and/or verification
• add an SignerOption like options.RecordVaultKeyID(keyID *string) to request that the Hashicorp signer would extract the prefix value and store it in keyID for later use (e.g. store in an annotation, logging, etc).

This has the added benefit of keeping these options available if someone wants to use the Vault implementation through the golang crypto.Signer interface.

[dlorenc]

Ah yeah, this is much better!

[BerndFarkaDyna]

@bobcallaway: THX this is the best approach so far...

Just one minor little thing: shouldn't it be considered calling it

options.WithKeyID(keyID string)
options.RecordKeyID(keyID *string)

so it would be much more general and could be reused in future for further KMS if needed?

[bobcallaway]

@bobcallaway: THX this is the best approach so far...

Just one minor little thing: shouldn't it be considered calling it

options.WithKeyID(keyID string)
options.RecordKeyID(keyID *string)

so it would be much more general and could be reused in future for further KMS if needed?

Is there another KMS that would immediately benefit from making this more generic? "KeyID" is a rather vague term so from a readability perspective I'd lean towards keeping the name specific to HV if it's the only one we know of right now (we could always delegate later to a common implementation and more generic name)

[BerndFarkaDyna]

I just had a quick look on the AWS-KMS documentation...

the response of an sign operation looks like this: https://docs.aws.amazon.com/kms/latest/APIReference/API_Sign.html

{
   "KeyId": "string",
   "Signature": blob,
   "SigningAlgorithm": "string"
}

and it looks like the verification request also takes a keyId (and looks even more complex than a vault request does)

https://docs.aws.amazon.com/kms/latest/APIReference/API_Verify.html

{
   "GrantTokens": [ "string" ],
   "KeyId": "string",
   "Message": blob,
   "MessageType": "string",
   "Signature": blob,
   "SigningAlgorithm": "string"
}

[bobcallaway]

According to https://docs.aws.amazon.com/kms/latest/developerguide/rotate-keys.html

Automatic key rotation has the following benefits:

The properties of the KMS key, including its key ID, key ARN, region, policies, and permissions, do not change when the key is rotated.

You do not need to change applications or aliases that refer to the key ID or key ARN of the KMS key.

I think that means that there's no way for AWS KMS callers to refer to multiple versions of a key in AWS KMS since the values don't change when the key is rotated.

[bobcallaway]

According to https://docs.aws.amazon.com/kms/latest/developerguide/rotate-keys.html

Automatic key rotation has the following benefits: The properties of the KMS key, including its key ID, key ARN, region, policies, and permissions, do not change when the key is rotated. You do not need to change applications or aliases that refer to the key ID or key ARN of the KMS key.

I think that means that there's no way for AWS KMS callers to refer to multiple versions of a key in AWS KMS since the values don't change when the key is rotated.

On second look, I think we could make this generic for GCP KMS so I just pushed up the first patch of several that should address the original concern - PTAL :)

[kent007]

Hi, I'd like to see about getting some more attention on this issue. I'm looking at using cosign via Connoisseur in a k8s environment for signature validation and we've run into this same issue....we want to have a rolling keyring in Vault of several valid keys, and we want cosign to understand which one to use for validation (or try all of them?).

Right now the default behavior of Cosign is to sign using the latest key and always validate using version 1, even if that version is no longer valid in Vault. Leaving aside the above discussion for a minute, my "bandaid" suggestion would be that if the key version hasn't been overridden anywhere, cosign attempts to query the key details from Vault. This would return the range of valid keys, and it could try all of them in order. Return early on the first success, otherwise return a failure. This requires that the VAULT_TOKEN grants access to read the key details, but I don't think that's entirely unreasonable.

I have a related suggestion about improving how Connoisseur authenticates to Vault to keep a token current, but I think that probably belongs in a separate issue

[znewman01]

There are two ways we could get Cosign to know which key to use for validation:

1. Try all of them (as you point out/suggest)
2. Give Cosign a "hint." There's ongoing work on a new "bundle format" which will have support for "key IDs". So when you use Cosign, the signature would mention which version of the key was used to sign.

I strongly prefer (2), but I can see the argument for (1), especially because it might be a while before (2) is ready.

I would be willing to merge a PR to do (1). I can't speak for everyone, thought—would love to hear others chime in.

[bradbeck]

I have run across this issue while using Vault transit to do the signing in Tekton Chains. The signing part seems to work as expected; using the most recent key to perform signing.

Verification on the other hand appears to only attempt to match against the original version of the key.

Since I believe Vault retains the history of all keys, would it be possible to attempt to match any key during verification and succeed if a match is found?

My steps for reproducing the issue are as follows:

# start vault server in separate shell
vault server -dev -dev-root-token-id root -dev-listen-address 0.0.0.0:8200

# login to vault and enable transit
export VAULT_ADDR='http://0.0.0.0:8200'
export VAULT_TOKEN=root
vault secrets enable transit

# create the transit key
cosign generate-key-pair --kms hashivault://cosign-keys
vault read transit/keys/cosign-keys

# create a blob
cat >blob.txt <<EOF
Some random stuff.
EOF

# sign the blob with the original transit key
cosign sign-blob --tlog-upload=false --key hashivault://cosign-keys --output-signature blob.sig.1 blob.txt

# verify the signature using the transit key URI
cosign verify-blob --insecure-ignore-tlog --key hashivault://cosign-keys --signature blob.sig.1 blob.txt

# verify the signature using the current public key
cosign public-key --key hashivault://cosign-keys > cosign.pub.1
cosign verify-blob --insecure-ignore-tlog  --key cosign.pub.1 --signature blob.sig.1 blob.txt

# rotate the transit key
vault write -f transit/keys/cosign-keys/rotate

# sign the blob with the rotated transit key
cosign sign-blob --tlog-upload=false --key hashivault://cosign-keys --output-signature blob.sig.2 blob.txt

# attempt to verify the signature using the transit key URI (fails unexpectedly)
cosign verify-blob --insecure-ignore-tlog --key hashivault://cosign-keys --signature blob.sig.2 blob.txt

# attempt to verify the first signature using the transit key URI (succeeds)
cosign verify-blob --insecure-ignore-tlog --key hashivault://cosign-keys --signature blob.sig.1 blob.txt

# attempt to verify the signature using the current public key (succeeds)
cosign public-key --key hashivault://cosign-keys > cosign.pub.2
cosign verify-blob --insecure-ignore-tlog --key cosign.pub.2 --signature blob.sig.2 blob.txt

# attempt to verify sig 2 using public key 1 (fails as expected)
cosign verify-blob --insecure-ignore-tlog --key cosign.pub.1 --signature blob.sig.2 blob.txt

# attempt to verify sig 1 using public key 2 (fails as expected)
cosign verify-blob --insecure-ignore-tlog --key cosign.pub.2 --signature blob.sig.1 blob.txt

[bradbeck]

@bobcallaway Made the following suggestion in Slack: https://sigstore.slack.com/archives/C01PZKDL4DP/p1692713079764339?thread_ts=1692711742.547209&cid=C01PZKDL4DP

I dont know that anyone is actively working on it, but I think we need to update the logic to verify for any enabled key version that is >= min_decryption_version from https://developer.hashicorp.com/vault/api-docs/secret/transit#update-key-configuration:~:text=Parameters-,min_decryption_version,-(int%3A%200)

[developer-guy]

Today we (w/@dentrax) also faced the same issue, basically to reproduce the issue:

cosign version: v2.2.0 vault version: v1.13.2

1. vault server -dev
2. vault secrets enable transit
3. vault kv put transit/keys/cosign type="ecdsa-p256"
4. cosign sign --key hashivault://cosign <image>
5. cosign verify --key hashivault://cosign <image> # it will work
6. vault write -f transit/keys/cosign/rotate
7. vault kv get -format=json transit/keys/cosign | jq '.data.latest_version' # you should see '2'
8. cosign verify --key hashivault://cosign <image> # it won't work which is what we expected
9. cosign sign --key hashivault://cosign <image> # resign the image with same key
10. cosign verify --key hashivault://cosign <image> # this won't work unless you specify VAULT_KEY_RPEFIX as 'vault:v2:'.

so here is the deal, wouldn't it solve the problem of finding the latest version of the key and use it as the value of VAULT_KEY_PREFIX or send it to the sigstore library as a key value version by default always?

P.S. I found this line in Sigstore which uses default vaultV1DataPrefix so instead of doing that we are going to fetch the latest version of the key and use it, does that sound good @bobcallaway?

client := h.client.Logical()

path := fmt.Sprintf("/%s/keys/%s", h.transitSecretEnginePath, h.keyPath)

keyResult, err := client.Read(path)
if err != nil {
  return fmt.Errorf("public key: %w", err)
}

if keyResult == nil {
  return fmt.Errorf("could not read data from transit key path: %s", path)
}

latestVersion, hasVersion := keyResult.Data["latest_version"]
if !hasVersion {
  return errors.New("failed to read transit key keys: corrupted response")
}

vaultDataPrefix = fmt.Sprintf("vault:v%d:", latestVersion)