Add HashiCorp Vault key manager plugin to SPIRE server

[InverseIntegral]

Pull Request check list

• Commit conforms to CONTRIBUTING.md?
• Proper tests/regressions included?
• Documentation updated?

Affected functionality
This MR introduces a new SPIRE server key manager plugin that uses HashiCorp Vault to manage keys.

Description of change
This change adds a new key manager plugin to SPIRE server named hashicorp_vault. It uses a transit secrets engine within HashiCorp Vault to manage keys and sign data. The client is largely based on the existing upstream authority plugin for HashiCorp Vault which was introduced in #1611 by @hiyosi.

Which issue this PR fixes
This PR fixes #5058

Note that this is my first time implementing such a plugin and I expect there to be a lot of open questions and expected changes. I'm really looking forward to any feedback! ❤️

[amartinezfayo]

Hi @InverseIntegral, thank you very much for this contribution.
I'm still going over this PR, but the first high-level comment that I wanted to provide is that we will need integration tests that exercise this plugin. It should be a new suite added to our integration test framework: https://github.com/spiffe/spire/tree/main/test/integration

I'll be adding more comments as I make progress with the review :) Thank you again for this contribution!

[amartinezfayo]

Suggested change

	UpstreamAuthority "vault" {
	KeyManager "hashicorp_vault" {

[amartinezfayo]

Suggested change

	UpstreamAuthority "vault" {
	KeyManager "hashicorp_vault" {

[amartinezfayo]

Suggested change

	UpstreamAuthority "vault" {
	KeyManager "hashicorp_vault" {

[amartinezfayo]

Suggested change

	UpstreamAuthority "vault" {
	KeyManager "hashicorp_vault" {

[InverseIntegral]

Thank you @amartinezfayo for the initial review and sorry for the delay on my end. I will add integration tests to test the plugin more thoroughly 👍

[amartinezfayo]

We should have a note here, that this is only for test environments.

[amartinezfayo]

I think that we should log a warning if InsecureSkipVerify is set to true.

[InverseIntegral]

Added a warning and adjusted the docs

[amartinezfayo]

Thank you @InverseIntegral for adding the integration tests, and sorry for the delay in the review.
I've added some comments. My main concern is around the handling of the creation of new keys (and the management of the key lifecycle in general), noticing that the path for a given SPIRE Key ID (e.g.: x509-CA-A) is always the same. It seems that the keys may not be rotated?

[amartinezfayo]

The variable c shadows the receiver c of the method. Consider renaming the variable to avoid confusion.

[InverseIntegral]

Good idea, fixed it

[amartinezfayo]

I believe that this refers to others parameters not being relevant. We may rephrase the comment for better understanding.

Suggested change

	// don't care any parameters
	// Other parameters are not relevant for token renewal

[InverseIntegral]

Correct, I fixed the comment 👍

[amartinezfayo]

This type assertion will panic if the type assertion ever fail. It would be desirable to add a check to ensure the type assertion is valid.

[InverseIntegral]

Done 👍

[amartinezfayo]

What happens if the key already exists in the specified path?
I'm worried about only the possibility of creating a new key the only the first time, and then always using the same key.

[amartinezfayo]

I don't think that we would want to make SPIRE keys exportable.

[InverseIntegral]

In that case I remove the TODO.

[amartinezfayo]

I think that SPIRE will always manage a single version of the key, creating a new key when a rotation happens. We need to make sure that keys that have been rotated are being deleted.

[InverseIntegral]

I see, in that case I will remove this TODO. Thanks for the clarification 🙂

[amartinezfayo]

It seems that all keys with the same SPIRE Key ID have the same path.
I think that the logic here should be that if we already have this SPIRE Key ID cached, a new key is created and the old one should be deleted.
I've added a comment about my concern about the current implementation that doesn't seem to be creating a new key, since the same path is always used for a given SPIRE Key ID.

[InverseIntegral]

Yes, they indeed all map to the same key which will cause issues during rotations. I saw that other plugins actually generate a unique ID for each key. For instance, the Azure Key Vault plugin does this:

// generateKeyName returns a new identifier to be used as a key name.
// The returned name has the form: spire-key-<UUID>-<SPIRE-KEY-ID>,
// where UUID is a new randomly generated UUID and SPIRE-KEY-ID is provided
// through the spireKeyID parameter.
func (p *Plugin) generateKeyName(spireKeyID string) (keyName string, err error) {
	uniqueID, err := generateUniqueID()
	if err != nil {
		return "", err
	}

	return fmt.Sprintf("%s-%s-%s", keyNamePrefix, uniqueID, spireKeyID), nil
}

Maybe it would make sense to do something similar here?

I think that the logic here should be that if we already have this SPIRE Key ID cached, a new key is created and the old one should be deleted.

Yes, that is the behaviour what we want. I see two options how we can achieve this:

Option 1: Use the random key name approach as outlined above such that all keys have a unique name within the HashiCorp Vault. When a new key is requested, we need to clean up keys with the same SPIRE Key ID. This would look something like this:

keyName := p.generateKeyName(spireKeyID)
createResp := p.client.CreateKey(ctx, keyName, *parameters)
if keyEntry, ok := p.getKeyEntry(spireKeyID); ok {
	select {
	case p.scheduleDelete <- keyEntry.KeyName:
		p.log.Debug("Key enqueued for deletion", keyNameTag, keyEntry.KeyName)
	default:
		p.log.Error("Failed to enqueue key for deletion", keyNameTag, keyEntry.KeyName)
	}
}

Option 2: Directly delete existing keys with the same SPIRE Key ID when a new key with the same ID is requested. The key name would be the same as the SPIRE Key ID in this case:

func (p *Plugin) createKey(ctx context.Context, spireKeyID string, keyType keymanagerv1.KeyType) (*keyEntry, error) {
        ...
	existingKey, err := p.vc.getKey(ctx, spireKeyID)
	if err != nil {
		// key already exists
		p.vc.deleteKey(ctx, spireKeyID)
	}
	
	err = p.vc.CreateKey(ctx, spireKeyID, *kt)
	if err != nil {
		return nil, err
	}

	return p.vc.getKeyEntry(ctx, spireKeyID)
}

[amartinezfayo]

I think that we should first create the key, and then schedule the old one for deletion if exists. Otherwise, if the deletion happens first and it fails, the key is deleted and there will be no key that replaces it, which is a pretty bad situation.

We need to consider also that in an HA deployment, multiple SPIRE servers will be accessing the same Vault instance, so there should be a way to identify each server. There should be a KeyIdentifierValue config attribute as we have in other key managers for this purpose. With that attribute, the server ID can be persisted and retrieved. The plugin needs that to know which keys are managed by this specific instance of SPIRE Server.

[InverseIntegral]

@amartinezfayo I've implemented it as you suggested. Now, a key is scheduled for deletion and each key has a unique id associated to it.

The KeyIdentifierValue config attribute is also there. Unfortunately, HashiCorp Vault doesn't support tags on keys. So we need a different way to associate keys with SPIRE servers. Perhaps a clever naming schema could solve this problem.

[amartinezfayo]

The KeyIdentifierValue config attribute is also there. Unfortunately, HashiCorp Vault doesn't support tags on keys.

This is unfortunate, because it makes difficult the identification and disposal of stale keys.
I think that we can embed the KeyIdentifierValue as part of the key name. I also think that it would be useful to be able to identify all the keys belonging to a trust domain, but the user could do that through the use of specific transit engine paths for each trust domain.
Prefixing the key names with "spire-key" is probably a good idea, to easily identify that they are SPIRE keys.

[amartinezfayo]

We need to validate that the configuration has a key identifier file or a key identifier value, and that the configuration does not have a key identifier file and a key identifier value at the same time. You can look at how this is validated in other key managers that have those settings.