Enable Istio-style SPIFFE IDs; custom namespaces, and trust domains #1011

v0lkan · 2024-06-25T21:16:11Z

Enable Istio-style SPIFFE IDs; custom namespaces, and trust domains

Description

This PR adds additional flexibility to VSecM deployments by enabling custom SPIFFE IDs
for workload identities, custom namespaces, and custom trust domains.

Changes

Update app/safe/internal/server/handle/route.go to fix a regression.
Introduce env.NameRegExpForWorkload() to allow matching workload ids from spiffe ids
based on regular expressions.
Updated WorkloadIDAndParts(spiffeid string) (string, []string) to work with
the above regexp; the implementation needs some simplification, which can be done
as a follow-up PR.
Added stricter default values to VSecMSpiffeIdPrefixSafe, VSecMSpiffeIdPrefixSentinel, VSecMSpiffeIdPrefixWorkload, VSecMNameRegExpForWorkload.
Fix tests and deployment manifests accordingly.
created a values-custom.yaml to test the changes with an inline comment to how to use
it. A follow-up PR will be created to add the test to integration test too.
Necessary documentation updates will be done as a separate PR too.

Test Policy Compliance

I have added or updated unit tests for my changes.
I have included integration tests where applicable.
All new and existing tests pass successfully.

Code Quality

I have followed the coding standards for this project.
I have performed a self-review of my code.
My code is well-commented, particularly in areas that may be difficult
to understand.

Documentation

There are some doc updates. The rest will be completed separately.

Additional Comments

These changes will especially be helpful for OpenShift deployments.

Checklist

Before you submit this PR, please make sure:

You have read the contributing guidelines and
especially the test policy.
You have thoroughly tested your changes.
You have followed all the contributing guidelines for this project.
You understand and agree that your contributions will be publicly available
under the project's license.

By submitting this pull request, you confirm that my contribution is made under
the terms of the project's license and that you have the authority to grant
these rights.

Thank you for your contribution to VMware Secrets Manager
🐢⚡️!

Signed-off-by: Volkan Özçelik <[email protected]>

v0lkan · 2024-06-25T21:18:40Z

cc: @BulldromeQ these changes will be useful for our OpenShift deployment too, since the particular cluster that we were working on was creating its own custom ClusterSPIFFEIds and was using its own custom namespaces (instead of vsecm-system).

v0lkan · 2024-06-25T21:19:14Z

I summarized the changes in the PR body; yet, I’ll annotate the code as needed too.

v0lkan · 2024-06-25T21:20:54Z

Makefile

@@ -31,6 +31,9 @@ VSECM_EKS_REGISTRY_URL ?= "public.ecr.aws/h8y1n7y7"
 VSECM_NAMESPACE_SYSTEM ?= "vsecm-system"
 VSECM_NAMESPACE_SPIRE ?= "spire-system"
 VSECM_NAMESPACE_SPIRE_SERVER ?= "spire-server"
+# VSECM_NAMESPACE_SYSTEM ?= "vsecm-system-custom"
+# VSECM_NAMESPACE_SPIRE ?= "spire-system-custom"
+# VSECM_NAMESPACE_SPIRE_SERVER ?= "spire-server-custom"


These commented out lines are useful to test the thing with the generated k8s manifests instead of directly using helm. — An alternative is to define the environment variables, or pass these as arguments to make. I wanted to keep them explicit as they double as documentation too.

v0lkan · 2024-06-25T21:26:11Z

core/constants/env/env.go

@@ -70,6 +70,7 @@ const VSecMSidecarSuccessThreshold VarName = "VSECM_SIDECAR_SUCCESS_THRESHOLD"
 const VSecMSpiffeIdPrefixSafe VarName = "VSECM_SPIFFEID_PREFIX_SAFE"
 const VSecMSpiffeIdPrefixSentinel VarName = "VSECM_SPIFFEID_PREFIX_SENTINEL"
 const VSecMSpiffeIdPrefixWorkload VarName = "VSECM_SPIFFEID_PREFIX_WORKLOAD"
+const VSecMWorkloadNameRegExp VarName = "VSECM_WORKLOAD_NAME_REGEXP"


This is new. It gives us the flexibility to capture any part of the SPIFFE ID as the workload’s id.

One restriction is; all regexp shall start with ^spifee://$trustDomain/ where $trustDomain is whichever trust domain has been configured.

It is also recommended for the regExps to end with a $ and make the matches as specific as possible whenever possible.

v0lkan · 2024-06-25T21:27:32Z

core/env/spiffeid.go

+func NameRegExpForWorkload() string {
+	p := env.Value(env.VSecMWorkloadNameRegExp)
+	if p == "" {
+		p = string(env.VSecMNameRegExpForWorkloadDefault)


Having default values in the codebase has both benefits, and liabilities; maybe we should create an ADR for this outlining why we prefer it that way (as opposed to, for example, using a configuration file, or a ConfigMap).

v0lkan · 2024-06-25T21:28:27Z

core/env/spiffeid_test.go

 		},
 		{
 			name: "safe_spiffeid_prefix_from_env",
 			setup: func() error {
-				return os.Setenv("VSECM_SPIFFEID_PREFIX_WORKLOAD", "spiffe://vsecm.com/workload/test/")
+				return os.Setenv("VSECM_SPIFFEID_PREFIX_WORKLOAD", "spiffe://vsecm.com/workload/test/ns/test/sa/test/n/test")


Thanks to the stricter SPIFFE ID validation, we had to update the tests to be stricter too. – the test do not pass if the ids do not conform to a certain pattern.

v0lkan · 2024-06-25T21:30:27Z

core/validation/validation.go

+// `^spiffe://$trustDomain` prefix for extra security.
+//
+// This variable shall be treated as constant and should not be modified.
+var spiffeRegexPrefixStart = "^spiffe://" + env.SpiffeTrustDomain() + "/"


This is intentionally hard coded and kept reconfigurable because

Any spiffe id has to start with spiffe:// as part of the standard and then followed by the trust domain.

Starting the matcher with a ^ makes it more restrictive and more secure, since you cannot just use any regexp, and you have to match thing starting from the beginning of the string.

v0lkan · 2024-06-25T21:31:09Z

core/validation/validation.go

@@ -43,13 +47,18 @@ const spiffeRegexPrefixStart = "^"
 //
 //	bool: `true` if the SPIFFE ID belongs to VSecM Sentinel, `false` otherwise.
 func IsSentinel(spiffeid string) bool {
+	if !IsWorkload(spiffeid) {
+		return false


Any VSecM entity is also a workload. So we run an IsWorkload() check before running other checks.

v0lkan · 2024-06-25T21:39:09Z

helm-charts/0.25.4/charts/spire/templates/spire-controller-manager-config.yaml

@@ -26,7 +26,7 @@ data:
      resourceName: 98c9c988.spiffe.io
      resourceNamespace: {{ .Values.global.spire.serverNamespace }}
    clusterName: vsecm-cluster
-    trustDomain: vsecm.com
+    trustDomain: {{ .Values.global.spire.trustDomain }}


We have missed this; the recent customization test helped us realize that :) .

v0lkan added 26 commits June 24, 2024 14:47

testing with custom manifests

7881ec7

Signed-off-by: Volkan Özçelik <[email protected]>

env var change

faf0129

Signed-off-by: Volkan Özçelik <[email protected]>

updated trust domain of SCM.

f7162b8

Signed-off-by: Volkan Özçelik <[email protected]>

manifest update

3efa66b

Signed-off-by: Volkan Özçelik <[email protected]>

add SPIFFE trust domain to charts

6abf0a9

Signed-off-by: Volkan Özçelik <[email protected]>

add diagnostic code

a59f094

Signed-off-by: Volkan Özçelik <[email protected]>

add diagnostic code

7a6b0f3

Signed-off-by: Volkan Özçelik <[email protected]>

manifest and business logic update

a5adef7

Signed-off-by: Volkan Özçelik <[email protected]>

more diagnostics

8199a0b

Signed-off-by: Volkan Özçelik <[email protected]>

more diagnostic

a67f217

Signed-off-by: Volkan Özçelik <[email protected]>

more diagnostics

c2ac350

Signed-off-by: Volkan Özçelik <[email protected]>

typo fix

f67d3d1

Signed-off-by: Volkan Özçelik <[email protected]>

manifest update

136d3f7

Signed-off-by: Volkan Özçelik <[email protected]>

more diagnostics

7035a5a

Signed-off-by: Volkan Özçelik <[email protected]>

manifest update

b7ab01e

Signed-off-by: Volkan Özçelik <[email protected]>

manifest update

15d75b3

Signed-off-by: Volkan Özçelik <[email protected]>

Add missing route.

12f9fbd

Signed-off-by: Volkan Özçelik <[email protected]>

manifest changes and validation fixes

e10e099

Signed-off-by: Volkan Özçelik <[email protected]>

manifest updates

e11811e

Signed-off-by: Volkan Özçelik <[email protected]>

debug code

2806fc2

Signed-off-by: Volkan Özçelik <[email protected]>

more debug code

bbf8a1d

Signed-off-by: Volkan Özçelik <[email protected]>

manifest fix

228d832

Signed-off-by: Volkan Özçelik <[email protected]>

manifest update

9dca752

Signed-off-by: Volkan Özçelik <[email protected]>

manifest update

a1d0c43

Signed-off-by: Volkan Özçelik <[email protected]>

Fix failing tests

2151cc1

Signed-off-by: Volkan Özçelik <[email protected]>

test fix

1050836

Signed-off-by: Volkan Özçelik <[email protected]>

v0lkan requested a review from BulldromeQ as a code owner June 25, 2024 21:16

v0lkan self-assigned this Jun 25, 2024

v0lkan requested review from st96d045 and abhishek44sharma June 25, 2024 21:16

v0lkan requested review from ats0stv and farhan-pasha June 25, 2024 21:16

v0lkan commented Jun 25, 2024

View reviewed changes

v0lkan merged commit 2ac30f9 into main Jun 25, 2024

v0lkan deleted the ovolkan/params branch June 25, 2024 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Istio-style SPIFFE IDs; custom namespaces, and trust domains #1011

Enable Istio-style SPIFFE IDs; custom namespaces, and trust domains #1011

v0lkan commented Jun 25, 2024

v0lkan commented Jun 25, 2024

v0lkan commented Jun 25, 2024

v0lkan Jun 25, 2024

v0lkan Jun 25, 2024

v0lkan Jun 25, 2024

v0lkan Jun 25, 2024

v0lkan Jun 25, 2024

v0lkan Jun 25, 2024

v0lkan Jun 25, 2024

Enable Istio-style SPIFFE IDs; custom namespaces, and trust domains #1011

Enable Istio-style SPIFFE IDs; custom namespaces, and trust domains #1011

Conversation

v0lkan commented Jun 25, 2024

Enable Istio-style SPIFFE IDs; custom namespaces, and trust domains

Description

Changes

Test Policy Compliance

Code Quality

Documentation

Additional Comments

Checklist

v0lkan commented Jun 25, 2024

v0lkan commented Jun 25, 2024

v0lkan Jun 25, 2024

Choose a reason for hiding this comment

v0lkan Jun 25, 2024

Choose a reason for hiding this comment

v0lkan Jun 25, 2024

Choose a reason for hiding this comment

v0lkan Jun 25, 2024

Choose a reason for hiding this comment

v0lkan Jun 25, 2024

Choose a reason for hiding this comment

v0lkan Jun 25, 2024

Choose a reason for hiding this comment

v0lkan Jun 25, 2024

Choose a reason for hiding this comment