Introduce the `gcp_kms` KeyManager plugin #3410

amartinezfayo · 2022-09-07T21:12:47Z

This PR introduces the gcp_kms KeyManager plugin, that uses the Google Cloud Key Management Service.

Fixes #3194.

Signed-off-by: Agustín Martínez Fayó <[email protected]>

MarcosDY · 2022-09-09T13:22:18Z

conf/server/server_full.conf

@@ -267,6 +267,24 @@ plugins {
    #     }
    # }

+    # KeyManager  "gcp_kms": A key manager for signing SVIDs which generates


Suggested change

# KeyManager "gcp_kms": A key manager for signing SVIDs which generates

# KeyManager "gcp_kms": A key manager for signing SVIDs which generates

MarcosDY · 2022-09-09T13:26:52Z

conf/server/server_full.conf

+    #
+    #        key_policy_file: A file path location to a custom IAM Policy (v3)
+    #        in JSON format to be attached to created CryptoKeys.
+    #        key_policy_file = "custom-gcp-kms-policy.json"


this file is going to be loaded at start and never updated?

No, the file will be read each time that Configure is called.

MarcosDY · 2022-09-09T13:32:44Z

conf/server/server_full.conf

+    # KeyManager  "gcp_kms": A key manager for signing SVIDs which generates
+    # and stores keys in Google Cloud KMS.
+    # KeyManager "gcp_kms" {
+    #    plugin_data = {


in another plugins we provides a way to configure different service account to be used, for example service_account_file that allows to set a different credentials,
Maybe we can add something like this here?

Yes, the plugin supports that already. I just need to add the example here.

MarcosDY · 2022-09-09T18:15:34Z

doc/plugin_server_keymanager_gcp_kms.md

+
+The plugin requires to have in the configured key ring the following IAM permissions granted to the authenticated service account:
+```
+cloudkms.cryptoKeys.*


what it means .* we really need all cryptoKeys permissions?
can you instead set the ones we really need?

Yes, we need all the permissions over cryptoKeys (cloudkms.cryptoKeys.create, cloudkms.cryptoKeys.get, cloudkms.cryptoKeys.getIamPolicy, cloudkms.cryptoKeys.list, cloudkms.cryptoKeys.setIamPolicy, cloudkms.cryptoKeys.update).

pkg/server/plugin/keymanager/gcpkms/fetcher.go

MarcosDY · 2022-09-13T16:18:00Z

pkg/server/plugin/keymanager/gcpkms/gcpkms.go

+	}
+}
+
+// scheduleDestroyTask ia a long running task that schedules the destruction


Suggested change

// scheduleDestroyTask ia a long running task that schedules the destruction

// scheduleDestroyTask is a long running task that schedules the destruction

pkg/server/plugin/keymanager/gcpkms/gcpkms.go

MarcosDY · 2022-09-13T17:26:39Z

pkg/server/plugin/keymanager/gcpkms/gcpkms.go

+					// The GetCryptoKeyVersion call failed. Log this and re-enqueue
+					// the CryptoKey for destruction. Hopefully, this is a
+					// recoverable error.
+					log.Error("Could not get the CryptoKeyVersion while trying to schedule it for destruction")


can you log the error here?

MarcosDY · 2022-09-13T17:29:53Z

pkg/server/plugin/keymanager/gcpkms/gcpkms.go

+
+	for _, e := range keyEntries {
+		p.entries[e.publicKey.Id] = *e
+		p.log.Debug("Cloud KMS key loaded", cryptoKeyVersionNameTag, e.cryptoKeyVersion.Name, algorithmTag, e.cryptoKeyVersion.Algorithm)


as a general comment, I think log will already have the plugin name as a fields so Cloud KMS is not required as part of the logs.

MarcosDY · 2022-09-13T17:49:59Z

pkg/server/plugin/keymanager/gcpkms/gcpkms.go

+		return "", status.Errorf(codes.Internal, "failed to generate ID for server: %v", err)
+	}
+
+	err = os.WriteFile(idPath, []byte(id), 0600)


this plugin may works on windows too, is that permission lvl enough there?

Signed-off-by: Agustín Martínez Fayó <[email protected]>

…ng a new CryptoKey. Signed-off-by: Agustín Martínez Fayó <[email protected]>

Signed-off-by: Agustín Martínez Fayó <[email protected]>

evan2645 · 2022-10-11T18:52:22Z

This is ready to review, but more tests are on the way.

Signed-off-by: Agustín Martínez Fayó <[email protected]>

…Cloud KMS API Signed-off-by: Agustín Martínez Fayó <[email protected]>

Signed-off-by: Agustín Martínez Fayó <[email protected]>

MarcosDY

some more comments, I still need to take a look to unit tests

MarcosDY · 2022-10-19T19:06:05Z

conf/server/server_full.conf

+    #        key_metadata_file: A file path location where information about
+    #        generated keys will be persisted.
+    #        key_metadata_file = "./file_path"
+    #
+    #        key_policy_file: A file path location to a custom IAM Policy (v3)
+    #        in JSON format to be attached to created CryptoKeys.
+    #        key_policy_file = "custom-gcp-kms-policy.json"
+    #
+    #        key_ring: Resource ID of the key ring where the keys managed by this
+    #        plugin reside.
+    #        key_ring = "projects/project/locations/location/keyRings/key-ring"


Suggested change

# key_metadata_file: A file path location where information about

# generated keys will be persisted.

# key_metadata_file = "./file_path"

#

# key_policy_file: A file path location to a custom IAM Policy (v3)

# in JSON format to be attached to created CryptoKeys.

# key_policy_file = "custom-gcp-kms-policy.json"

#

# key_ring: Resource ID of the key ring where the keys managed by this

# plugin reside.

# key_ring = "projects/project/locations/location/keyRings/key-ring"

# # key_metadata_file: A file path location where information about

# # generated keys will be persisted.

# # key_metadata_file = "./file_path"

#

# # key_policy_file: A file path location to a custom IAM Policy (v3)

# # in JSON format to be attached to created CryptoKeys.

# # key_policy_file = "custom-gcp-kms-policy.json"

#

# # key_ring: Resource ID of the key ring where the keys managed by this

# # plugin reside.

# # key_ring = "projects/project/locations/location/keyRings/key-ring"

MarcosDY · 2022-10-19T19:11:31Z

doc/plugin_server_keymanager_gcp_kms.md

+| Key | Type | Required | Description | Default |
+| --- | ---- | -------- | ----------- | ------- |
+| key_policy_file | string | no | A file path location to a custom [IAM Policy (v3)](https://cloud.google.com/pubsub/docs/reference/rpc/google.iam.v1#google.iam.v1.Policy) in JSON format to be attached to created CryptoKeys. | "" |
+| key_metadata_file | string | yes | A file path location where key metadata used by the plugin will be persisted. | "" |
+| key_ring | string | yes | Resource ID of the key ring where the keys managed by this plugin reside, in the format projects/\*/locations/\*/keyRings/\* | "" |
+| service_account_file | string | no | Path to the service account file used to authenticate with the Cloud KMS API. | Value of `GOOGLE_APPLICATION_CREDENTIALS ` environment variable. |


can you make this table consistent with our new standards?

MarcosDY · 2022-10-19T19:13:53Z

doc/plugin_server_keymanager_gcp_kms.md

@@ -0,0 +1,108 @@
+# Server plugin: KeyManager "gcp_kms"


NIT: we are going to add a md linter, as part of this linter we are going to force the lines to a max of 80 characters, it will be great if you apply that here

MarcosDY · 2022-10-19T19:39:09Z

doc/plugin_server_keymanager_gcp_kms.md

+### Required permissions
+
+The plugin requires to have in the configured key ring the following IAM permissions granted to the authenticated service account:
+```


all code pieces must have a type defined, what do you think to set it as text?

MarcosDY · 2022-10-19T19:39:42Z

doc/plugin_server_keymanager_gcp_kms.md

+```
+KeyManager "gcp_kms" {


Suggested change

```

KeyManager "gcp_kms" {

```hcl

KeyManager "gcp_kms" {

MarcosDY · 2022-10-19T20:50:52Z

pkg/server/plugin/keymanager/gcpkms/client_fake.go

+		return fakeCryptoKeyVersion.privateKey.Sign(rand.Reader, digest, opts)
+	}
+
+	cryptoKeyName := path.Dir(path.Dir(signReq.Name))


why double path.Dir is required?

We need the CryptoKey name here. The signing request comes with the CryptoKeyVersion name, e.g.: projects/project-name/locations/location-name/keyRings/key-ring-name/spire-key-aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee-spireKeyID-1/cryptoKeyVersions/1, so we need to get go up two levels to be at the CryptoKey level, in this case: projects/project-name/locations/location-name/keyRings/key-ring-name/spire-key-aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee-spireKeyID-1.

MarcosDY · 2022-10-20T14:10:53Z

pkg/server/plugin/keymanager/gcpkms/fetcher.go

+		return nil, status.Error(codes.Internal, "cryptoKey is nil")
+	}
+
+	it := kf.kmsClient.ListCryptoKeyVersions(ctx, &kmspb.ListCryptoKeyVersionsRequest{


NIT: can you add a comment, about this List will return only latest versions? (we keep only 2 actives for now)

MarcosDY · 2022-10-20T17:17:54Z

pkg/server/plugin/keymanager/gcpkms/gcpkms.go

+	p.entriesMtx.RLock()
+	defer p.entriesMtx.RUnlock()
+
+	keyEntry, hasKey := p.entries[req.KeyId]
+	if !hasKey {
+		return nil, status.Errorf(codes.NotFound, "key %q not found", req.KeyId)
+	}


may we unlock as soon as we get the entry?
Sings can be a slow process, and I'm worried to unlocking entries until we finish the signing can result in some unexpected locks,
maybe you can add a getEntry(keyID string) (entry, ok) {} and add lock there

MarcosDY · 2022-10-20T20:00:41Z

pkg/server/plugin/keymanager/gcpkms/gcpkms.go

+			if err := p.enqueueDestruction(cryptoKeyVersion.Name); err != nil {
+				p.log.With(cryptoKeyNameTag, cryptoKey.Name).Error("Failed to enqueue CryptoKeyVersion for destruction", reasonTag, err)
+			}


what happens if we are not able to enqueue destruction but continue here?
we can get into a situation where we failed to enqueue all versions, but at the end we marke cryptoKey as inactive.

That's a good point. Calling setInactive after detecting that there are no more CryptoKeyVersions at this point assumed that all versions could be enqueued for destruction, but that may not always be the case if there was an error. Since we already call setInactive if the returned CryptoKey doesn't have enabled CryptoKeyVersions, a safer approach would be to just leave that and remove this one.

MarcosDY · 2022-10-20T20:29:29Z

pkg/server/plugin/keymanager/gcpkms/gcpkms.go

+	defer p.entriesMtx.RUnlock()
+	var errs []string
+	for _, entry := range p.entries {
+		entry.cryptoKey.Labels[labelNameLastUpdate] = fmt.Sprint(p.hooks.clk.Now().Unix())


this function has a RLock, but it is setting things here

Good catch!

evan2645 · 2022-09-27T17:35:40Z

doc/plugin_server_keymanager_gcp_kms.md

+
+If the _Key Metadata File_ is not found during server startup, the file is recreated, with a new auto-generated server ID. Consequently, if the file is lost, the plugin will not be able to identify keys that it has previously managed and will recreate new keys on demand.
+
+When an active key is rotated, the old CryptoKeyVersion is scheduled for destruction, and the corresponding CryptoKey is set as inactive.


Is this correct? The last sentence in "Use of key versions" seems contradictory and makes a little more sense.

Since we already have that sentence, maybe we can just remove this one?

Thanks for catching this. This paragraph was written when the plugin didn't manage multiple CryptoKeyVersions.

evan2645 · 2022-10-20T23:42:50Z

doc/plugin_server_keymanager_gcp_kms.md

+| Key | Type | Required | Description | Default |
+| --- | ---- | -------- | ----------- | ------- |
+| key_policy_file | string | no | A file path location to a custom [IAM Policy (v3)](https://cloud.google.com/pubsub/docs/reference/rpc/google.iam.v1#google.iam.v1.Policy) in JSON format to be attached to created CryptoKeys. | "" |
+| key_metadata_file | string | yes | A file path location where key metadata used by the plugin will be persisted. | "" |


evan2645 · 2022-10-20T23:49:13Z

doc/plugin_server_keymanager_gcp_kms.md

+
+### Required permissions
+
+The plugin requires to have in the configured key ring the following IAM permissions granted to the authenticated service account:


Suggested change

The plugin requires to have in the configured key ring the following IAM permissions granted to the authenticated service account:

The plugin requires the following IAM permissions be granted to the authenticated service account in the configured key ring:

Here's an alternative, I'm not sure if it's more or less confusing 😂

I struggled a lot with that phrase! The suggested change looks a lot better to me.

evan2645 · 2022-10-21T00:45:28Z

pkg/server/plugin/keymanager/gcpkms/gcpkms.go

+// with the CryptoKey, CryptoKeyVersion and public key.
+func (p *Plugin) createKey(ctx context.Context, spireKeyID string, keyType keymanagerv1.KeyType) (*keyEntry, error) {
+	p.entriesMtx.Lock()
+	defer p.entriesMtx.Unlock()


Assuming I understand this correctly ... we'll create a key here when SPIRE prepares a new one ... but, we take a write lock here to do it, so the act of preparing a new key will block signing operations on our other (currently active) key?

That's a good point. The lock window could be reduced using a function that would update the cache holding the write lock only in that function. This function could then call it when it updates the key entries cache. I'll make that change.

evan2645 · 2022-10-21T00:49:36Z

pkg/server/plugin/keymanager/gcpkms/gcpkms.go

+			p.log.Error("Failed to enqueue CryptoKeyVersion for destruction", reasonTag, err)
+		}
+
+		return &newKeyEntry, nil


Since we're effectively branching all of createKey on whether we have an existing crypto key or not, it might be easier to read if it were broken out into two further functions

MarcosDY · 2022-10-21T15:54:08Z