Add support for San selectors in x509pop node attestor plugin #5775

snanjundaswamy · 2025-01-15T07:46:06Z

Pull Request check list

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

Affected functionality
x509pop Node Attestor plugin now supports SAN based additional selectors. Each san is for the URI format x509pop:///key:value. Each of these SANs is exposed as a selector with x509pop:san::. These selectors can also be used as a part of agent template with {{ .San.}}

Description of change
The change contains feature augmentation on x509pop node attestor plugin along with tests. The certificates for testing were generated with test/fixture/nodeattestor/x509pop/generate.go

Which issue this PR fixes
fixes #5746

Signed-off-by: snanjundaswamy <[email protected]>

kfox1111 · 2025-01-15T17:27:04Z

test/fixture/nodeattestor/x509pop/generate.go

@@ -48,6 +48,10 @@ func main() {
 		KeyUsage:     x509.KeyUsageDigitalSignature,
 		NotAfter:     neverExpires,
 		Subject:      pkix.Name{CommonName: "COMMONNAME"},
+		URIs: []*url.URL{
+			{Scheme: "x509pop", Host: "example.org", Path: "/datacenter:us-east-1"},
+			{Scheme: "x509pop", Host: "example.org", Path: "/environment:production"},


just curious (naming things is hard), there a reason for /environment:production over say, /environment/production ?

Looks like : is a reserved character for URLs so it might not be the best choice. It may work now, but the standard library could add more validation in the future. /name/value seems like a reasonable choice to me.

Although by https://datatracker.ietf.org/doc/html/rfc3986#section-3.3 it seems to allowed in the path.

I'm not too opinionated on ':', I simply chose the character for consistency with separators in selectors. Based on what I hear, '/' sounds like a natural fit. I will make this change.

Since : is allowed in URL paths, I would suggest we stick to it as the SAN selector delimiter since it's what we use everywhere else.

sorindumitru

Thanks for the contribution. Generally looks good to me, with some small comments. It would also be good if we had some tests for using the selectors in the path component, do you think that could be done?

sorindumitru · 2025-01-17T21:46:14Z

pkg/common/plugin/x509pop/x509pop.go

@@ -36,6 +36,7 @@ type agentPathTemplateData struct {
 	PluginName      string
 	TrustDomain     string
 	SVIDPathTrimmed string
+	San             map[string]string


Maybe something like this for the name, to be a bit clearer and to avoid confusion with other kinds of Sans

Suggested change

San map[string]string

URISanSelectors map[string]string

sorindumitru · 2025-01-17T21:54:23Z

pkg/server/plugin/nodeattestor/x509pop/x509pop.go

+			if strings.Contains(unprefixedUriSan, ":") {
+				lastIndex := strings.LastIndex(unprefixedUriSan, ":")
+				uriSelectorKey := unprefixedUriSan[:lastIndex]
+				uriSelectorValue := unprefixedUriSan[lastIndex+1:]
+				uriSelectorMap[uriSelectorKey] = uriSelectorValue
+			}


Could we also log something if it's not possible to extract a selector from the path? The x509pop scheme isn't likely to be used for anything else so the user might want to know that one of the URIs couldn't be used.

sorindumitru · 2025-01-17T22:00:11Z

pkg/server/plugin/nodeattestor/x509pop/x509pop.go

+				lastIndex := strings.LastIndex(unprefixedUriSan, ":")
+				uriSelectorKey := unprefixedUriSan[:lastIndex]
+				uriSelectorValue := unprefixedUriSan[lastIndex+1:]
+				uriSelectorMap[uriSelectorKey] = uriSelectorValue


Any reason for using LastIndex for splitting the string? Something like x509pop://example.org/datacenter:ny:1 would be split as datacenter:ny => 1, but to me it makes more sense as datacenter => ny:1 (which could be done through SplitN).

Definitely a bit of an uncommon edge case, but it would be good to figure out from the beginning.

I thought about this a little bit myself. In my experience, the Key is usually fully qualified as against the values.
Something like company.org.service.datacenter = us-east-1. References on k8s annotations here.
Either way, this should be a simple change, lmk what you think

We have a convention in the project around how selectors are formatted - key:value, where key cannot contain a : delimiter. See this helper method for reference:

spire/cmd/spire-server/util/util.go

Line 185 in 50976c3

parts := strings.SplitAfterN(str, ":", 2)

We should be consistent with that convention here as well.

@rturner3 @sorindumitru
Ok makes sense now.
But what about the values? Should we also retain the '/' in the values coming from the URI San? This will help us eliminate ambiguity around the usage of ':' in values. Here's an example for reference
datacenter:company/foo/us-east-1

Yes, I think we should keep the SAN value as-is.

sorindumitru · 2025-01-17T22:01:34Z

doc/plugin_server_nodeattestor_x509pop.md

@@ -44,6 +44,7 @@ A sample configuration:
 | Common Name      | `x509pop:subject:cn:example.org`                                  | The Subject's Common Name (see X.500 Distinguished Names)                                |
 | SHA1 Fingerprint | `x509pop:ca:fingerprint:0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33` | The SHA1 fingerprint as a hex string for each cert in the PoP chain, excluding the leaf. |
 | SerialNumber     | `x509pop:serialnumber:0a1b2c3d4e5f`                               | The leaf certificate serial number as a lowercase hexadecimal string                     |
+| San     | `x509pop:san.<key>:<value>`                               | The san selectors on the leaf selectors. The expected format of the uri san is `x509pop://<trust_domain>/<key>:<value>`  string                     |


Suggested change

| San | `x509pop:san.<key>:<value>` | The san selectors on the leaf selectors. The expected format of the uri san is `x509pop://<trust_domain>/<key>:<value>` string |

| San | `x509pop:san:<key>:<value>` | The san selectors on the leaf selectors. The expected format of the uri san is `x509pop://<trust_domain>/<key>:<value>` string |

snanjundaswamy requested review from evan2645, amartinezfayo, azdagron, MarcosDY and rturner3 as code owners January 15, 2025 07:46

Add support for San selectors in x509pop node attestor plugin

a313ae5

Signed-off-by: snanjundaswamy <[email protected]>

snanjundaswamy force-pushed the I-5746 branch from ac2835f to a313ae5 Compare January 15, 2025 07:47

snanjundaswamy mentioned this pull request Jan 15, 2025

Support for optional SAN based label selectors in X509 pop Node Attestor #5746

Open

kfox1111 reviewed Jan 15, 2025

View reviewed changes

sorindumitru reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for San selectors in x509pop node attestor plugin #5775

Add support for San selectors in x509pop node attestor plugin #5775

snanjundaswamy commented Jan 15, 2025 •

edited

Loading

kfox1111 Jan 15, 2025

sorindumitru Jan 16, 2025

sorindumitru Jan 16, 2025

snanjundaswamy Jan 17, 2025

rturner3 Jan 21, 2025

sorindumitru left a comment

sorindumitru Jan 17, 2025

sorindumitru Jan 17, 2025

sorindumitru Jan 17, 2025

snanjundaswamy Jan 20, 2025

rturner3 Jan 21, 2025 •

edited

Loading

snanjundaswamy Jan 30, 2025

rturner3 Jan 30, 2025

sorindumitru Jan 17, 2025

	\| San \| `x509pop:san.<key>:<value>` \| The san selectors on the leaf selectors. The expected format of the uri san is `x509pop://<trust_domain>/<key>:<value>` string \|
	\| San \| `x509pop:san:<key>:<value>` \| The san selectors on the leaf selectors. The expected format of the uri san is `x509pop://<trust_domain>/<key>:<value>` string \|

Add support for San selectors in x509pop node attestor plugin #5775

Are you sure you want to change the base?

Add support for San selectors in x509pop node attestor plugin #5775

Conversation

snanjundaswamy commented Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorindumitru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rturner3 Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snanjundaswamy commented Jan 15, 2025 •

edited

Loading

rturner3 Jan 21, 2025 •

edited

Loading