SDS v3 endpoints support SPIFFE validator #2435

MarcosDY · 2021-08-03T13:02:59Z

Update SDS v3 endpoints to use "SPIFFE Certificate Validator", it is used as default for Envoys from v1.18.0.
Add a new configurable to set default_all_bundles_name that i used to configure a validator config that contains Bundle together with all Federated Bundles.

Which issue this PR fixes
fixes #2420

amartinezfayo

Looking great! I have some small comments.

amartinezfayo · 2021-08-10T14:07:00Z

doc/spire_agent.md

+| -------------------------- | --------------------------------------------------------------------------------------- | -------------------- |
+| `default_svid_name`        | The TLS Certificate resource name to use for the default X509-SVID with Envoy SDS       | default              |
+| `default_bundle_name`      | The Validation Context resource name to use for the default X.509 bundle with Envoy SDS | ROOTCA               |
+| `default_all_bundle_sname` | The Validation Context resource name to use when fetching X.509 bundle together with federated bundles with Envoy SDS | ALL               |


Suggested change

| `default_all_bundle_sname` | The Validation Context resource name to use when fetching X.509 bundle together with federated bundles with Envoy SDS | ALL |

| `default_all_bundles_name` | The Validation Context resource name to use when fetching X.509 bundle together with federated bundles with Envoy SDS | ALL |

amartinezfayo · 2021-08-10T14:08:53Z

doc/spire_agent.md

@@ -273,6 +274,9 @@ resource name (e.g. `spiffe://example.org`). Alternatively, if the default name
 `auth.CertificateValidationContext` containing the trusted CA certificates for the agent's trust domain is fetched.
 The default name is configurable (see `default_bundle_name` under [SDS Configuration](#sds-configuration)).

+Another option is to request for `ALL` that creates a [SPIFFE Certificate Validator](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto) that contains TrustDomain together with all Federated Bundles. This feature is supported from Envoy v1.18.0.


Suggested change

Another option is to request for `ALL` that creates a [SPIFFE Certificate Validator](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto) that contains TrustDomain together with all Federated Bundles. This feature is supported from Envoy v1.18.0.

Another option is to request for `ALL` that creates a [SPIFFE Certificate Validator](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto) that contains TrustDomain together with all Federated Bundles. This feature is supported starting with Envoy v1.18.0.

amartinezfayo · 2021-08-10T14:11:24Z

pkg/agent/endpoints/sdsv3/handler.go

@@ -244,29 +247,79 @@ func (h *Handler) buildResponse(versionInfo string, req *discovery_v3.DiscoveryR
 	}
 	returnAllEntries := len(names) == 0

+	// Use RootCA as default, but replace for SPIFFE auth when envoy version is at least v1.18.0


Suggested change

// Use RootCA as default, but replace for SPIFFE auth when envoy version is at least v1.18.0

// Use RootCA as default, but replace with SPIFFE auth when Envoy version is at least v1.18.0

amartinezfayo · 2021-08-10T14:15:25Z

pkg/agent/endpoints/sdsv3/handler.go

+
+		case names[h.c.DefaultAllBundlesName]:
+			if useRootCAValidator {
+				return nil, status.Error(codes.Internal, `unable to use "SPIFFE validator" on envoy below 1.17`)


Suggested change

return nil, status.Error(codes.Internal, `unable to use "SPIFFE validator" on envoy below 1.17`)

return nil, status.Error(codes.Internal, `unable to use "SPIFFE validator" on Envoy below 1.17`)

azdagron

Thanks for putting this together @MarcosDY! I wonder if some of the "builder" stuff can be simplified.... I've left some comments to that effect.

azdagron · 2021-08-10T22:27:31Z

pkg/agent/endpoints/sdsv3/handler.go

+	useRootCAValidator := true
+	if buildVersion := req.Node.GetUserAgentBuildVersion(); buildVersion != nil {
+		version := buildVersion.Version
+		useRootCAValidator = version.MajorNumber <= 1 && version.MinorNumber < 18
+	}


Maybe put this in another function, maybe named supportsSPIFFEAuthExtension?

azdagron · 2021-08-10T22:40:09Z

pkg/agent/endpoints/sdsv3/handler.go

@@ -244,29 +247,79 @@ func (h *Handler) buildResponse(versionInfo string, req *discovery_v3.DiscoveryR
 	}
 	returnAllEntries := len(names) == 0

+	// Use RootCA as default, but replace for SPIFFE auth when envoy version is at least v1.18.0
+	useRootCAValidator := true


The builder interface doesn't seem very useful since we still check this boolean in most spots in order to set up the correct interface.

Perhaps, the builder interface looks like:

type validationContextBuilder interface { buildOne(resourceName, trustDomainID string) (*any.Any, error) buildAll(resourceName string, td string) (*any.Any, error) }

Then that interface could be set once, up at top, once we determine if there is support for the SPIFFE Auth Extension. buildOne would be called in all cases except when the "ALL
name is requested, which would use buildAll. The buildAll method for the "root ca" builder would fail.

azdagron

Sorry for the additional back-and-forth on this, I noticed a few more things.

azdagron · 2021-08-17T15:46:21Z

cmd/spire-agent/cli/run/run.go

@@ -400,6 +402,10 @@ func NewAgentConfig(c *Config, logOptions []log.Option, allowUnknownConfig bool)
 	ac.DataDir = c.Agent.DataDir
 	ac.DefaultSVIDName = c.Agent.SDS.DefaultSVIDName
 	ac.DefaultBundleName = c.Agent.SDS.DefaultBundleName
+	ac.DefaultAllBundlesName = c.Agent.SDS.DefaultAllBundlesName
+	if ac.DefaultAllBundlesName == ac.DefaultBundleName {
+		logger.Warn(`"default_bundle_name" and "default_all_bundles_name" has the same value, "default_bundle_name" will be used. In future versions this misconfiguration will cause agent to not start.`)


Small suggestion on rewording emphasizing that the "all" configurable will be ignored and an added call to action.

Suggested change

logger.Warn(`"default_bundle_name" and "default_all_bundles_name" has the same value, "default_bundle_name" will be used. In future versions this misconfiguration will cause agent to not start.`)

logger.Warn(`The "default_bundle_name" and "default_all_bundles_name" configurables have the same value. "default_all_bundles_name" will be ignored. Please configure distinct values or use the defaults. This will be a configuration error In a future release.`)

azdagron · 2021-08-17T15:49:49Z

conf/agent/agent_full.conf

+    #     # default_all_bundles_name: The Validation Context resource name to use for when 
+    #     # fetching X.509 bundle togehter with federated bundles with Envoy SDS
+    #     # default_all_bundles_name = "ALL"


Suggested change

# # default_all_bundles_name: The Validation Context resource name to use for when

# # fetching X.509 bundle togehter with federated bundles with Envoy SDS

# # default_all_bundles_name = "ALL"

# # default_all_bundles_name: The Validation Context resource name to use to fetch

# # all bundles (including federated bundles) with Envoy SDS. Cannot be used with

# # Envoy releases prior to 1.18.

# # default_all_bundles_name = "ALL"

azdagron · 2021-08-17T15:50:37Z

doc/spire_agent.md

+| -------------------------- | --------------------------------------------------------------------------------------- | -------------------- |
+| `default_svid_name`        | The TLS Certificate resource name to use for the default X509-SVID with Envoy SDS       | default              |
+| `default_bundle_name`      | The Validation Context resource name to use for the default X.509 bundle with Envoy SDS | ROOTCA               |
+| `default_all_bundles_name` | The Validation Context resource name to use when fetching X.509 bundle together with federated bundles with Envoy SDS | ALL               |


Suggested change

| `default_all_bundles_name` | The Validation Context resource name to use when fetching X.509 bundle together with federated bundles with Envoy SDS | ALL |

| `default_all_bundles_name` | The Validation Context resource name to use for all bundles (including federated) with Envoy SDS | ALL |

azdagron · 2021-08-17T15:51:09Z

doc/spire_agent.md

@@ -273,6 +274,9 @@ resource name (e.g. `spiffe://example.org`). Alternatively, if the default name
 `auth.CertificateValidationContext` containing the trusted CA certificates for the agent's trust domain is fetched.
 The default name is configurable (see `default_bundle_name` under [SDS Configuration](#sds-configuration)).

+Another option is to request for `ALL` that creates a [SPIFFE Certificate Validator](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto) that contains TrustDomain together with all Federated Bundles. This feature is supported starting with Envoy v1.18.0.
+The default name is configurable (see `default_all_bundles_name` under [SDS Configuration](#sds-configuration)


Suggested change

The default name is configurable (see `default_all_bundles_name` under [SDS Configuration](#sds-configuration)

The default name is configurable (see `default_all_bundles_name` under [SDS Configuration](#sds-configuration).

azdagron · 2021-08-17T16:04:07Z

doc/spire_agent.md

@@ -273,6 +274,9 @@ resource name (e.g. `spiffe://example.org`). Alternatively, if the default name
 `auth.CertificateValidationContext` containing the trusted CA certificates for the agent's trust domain is fetched.
 The default name is configurable (see `default_bundle_name` under [SDS Configuration](#sds-configuration)).

+Another option is to request for `ALL` that creates a [SPIFFE Certificate Validator](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto) that contains TrustDomain together with all Federated Bundles. This feature is supported starting with Envoy v1.18.0.


I think we should combine this and the previous paragraph, since it isn't exactly clear, at least with the current formatting, that this paragraph is still in reference to the validation context. My suggestion also has a little rewording:

[`auth.CertificateValidationContext`](https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/auth/cert.proto#auth-certificatevalidationcontext) resources containing trusted CA certificates can be fetched using the SPIFFE ID of the desired trust domain as the resource name (e.g. `spiffe://example.org`). In addition, two other special resource names are available. The first, which defaults to "ROOTCA", provides the CA certificates for the trust domain the agent belongs to. The second, which defaults to "ALL", returns the trusted CA certificates for both the trust domain the agent belongs to as well as any federated trust domains applicable to the Envoy workload. The default names for these resource names are configurable via the `default_bundle_name` and `default_all_bundles_name`, respectively. The "ALL" resource name requires support for the [SPIFFE Certificate Validator](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto) extension, which is only available starting with Envoy 1.18.

azdagron · 2021-08-17T16:04:55Z

pkg/agent/endpoints/sdsv3/handler.go

+		// Create validation context.
+		// - RootCA validator for v1.17 and below
+		// - SPIFFE validator from v1.18


This comment now seems out of place....

azdagron · 2021-08-17T16:07:17Z

pkg/agent/endpoints/sdsv3/handler.go

+func (b *rootCABuilder) buildOne(resourceName, trustDomain string) (*any.Any, error) {
+	bundle, ok := b.bundles[trustDomain]
+	if !ok {
+		return nil, status.Errorf(codes.Internal, "no bundle found for trust domain: %q", trustDomain)


I'm not sure this should be codes.Internal, it's easy for someone to make a request for a trust domain ID that they don't have access to. I'd recommend either codes.InvalidRequest or codes.NotFound.

I choose codes.NotFound

azdagron · 2021-08-17T16:09:32Z

pkg/agent/endpoints/sdsv3/handler.go

+func newSpiffeBuilder(tdBundle *bundleutil.Bundle, federatedBundles map[spiffeid.TrustDomain]*bundleutil.Bundle) (validationContextBuilder, error) {
+	bundles := make(map[spiffeid.TrustDomain]*bundleutil.Bundle, len(federatedBundles)+1)
+
+	// Add td bundle if provided


FWIW, tdBundle should never be nil. I'm ok with the defensive code, but we should adjust the comment so folks don't think it can be nil in normal circumstances.

Suggested change

// Add td bundle if provided

// Only include tdBundle if it is not nil, which shouldn't ever be the case. This is purely defensive.

azdagron · 2021-08-17T16:09:44Z

pkg/agent/endpoints/sdsv3/handler.go

+
+func newRootCABuilder(bundle *bundleutil.Bundle, federatedBundles map[spiffeid.TrustDomain]*bundleutil.Bundle) validationContextBuilder {
+	bundles := make(map[string]*bundleutil.Bundle, len(federatedBundles)+1)
+	// Add td bundle if provided


FWIW, tdBundle should never be nil. I'm ok with the defensive code, but we should adjust the comment so folks don't think it can be nil in normal circumstances.

Suggested change

// Add td bundle if provided

// Only include tdBundle if it is not nil, which shouldn't ever be the case. This is purely defensive.

azdagron · 2021-08-17T16:12:52Z

pkg/agent/endpoints/sdsv3/handler.go

+func supportsSPIFFEAuthExtension(req *discovery_v3.DiscoveryRequest) bool {
+	if buildVersion := req.Node.GetUserAgentBuildVersion(); buildVersion != nil {
+		version := buildVersion.Version
+		return version.MajorNumber >= 1 && version.MinorNumber > 17


This check is potentially incorrect for Envoy 2.0 - 2.17 (which may or may not support the extension, but we can assume they do until we know better).

Suggested change

return version.MajorNumber >= 1 && version.MinorNumber > 17

return (version.MajorNumber == 1 && version.MinorNumber > 17) || version.MajorNumber > 1

azdagron

\o/, awesome work, @MarcosDY !

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

MarcosDY requested review from amartinezfayo, APTy, azdagron, evan2645 and rturner3 as code owners August 3, 2021 13:02

MarcosDY force-pushed the support-auth-validator-on-sds branch from 19d8b31 to 60d7f52 Compare August 3, 2021 14:27

amartinezfayo reviewed Aug 10, 2021

View reviewed changes

azdagron reviewed Aug 10, 2021

View reviewed changes

azdagron assigned azdagron and amartinezfayo Aug 12, 2021

azdagron added this to the 1.0.2 milestone Aug 17, 2021

azdagron reviewed Aug 17, 2021

View reviewed changes

azdagron previously approved these changes Aug 17, 2021

View reviewed changes

MarcosDY added 3 commits August 17, 2021 15:09

SDS v3 endpoints support SPIFFE validator

1ab6f75

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

PR changes

3b3af84

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

PR changes

78351a4

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

MarcosDY dismissed azdagron’s stale review via 78351a4 August 17, 2021 18:16

MarcosDY force-pushed the support-auth-validator-on-sds branch from cb8d17b to 78351a4 Compare August 17, 2021 18:16

azdagron approved these changes Aug 17, 2021

View reviewed changes

Merge branch 'main' into support-auth-validator-on-sds

ac66755

azdagron merged commit 8b905e9 into spiffe:main Aug 17, 2021

evan2645 pushed a commit to evan2645/spire that referenced this pull request Sep 2, 2021

SDS v3 endpoints support SPIFFE validator (spiffe#2435)

c54c090

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

evan2645 pushed a commit that referenced this pull request Sep 2, 2021

SDS v3 endpoints support SPIFFE validator (#2435)

1059570

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

StupidScience mentioned this pull request Apr 27, 2022

Envoy SDS custom validator issue #3001

Closed

MarcosDY deleted the support-auth-validator-on-sds branch August 2, 2022 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDS v3 endpoints support SPIFFE validator #2435

SDS v3 endpoints support SPIFFE validator #2435

MarcosDY commented Aug 3, 2021

amartinezfayo left a comment

amartinezfayo Aug 10, 2021

amartinezfayo Aug 10, 2021

amartinezfayo Aug 10, 2021

amartinezfayo Aug 10, 2021

azdagron left a comment

azdagron Aug 10, 2021

azdagron Aug 10, 2021

azdagron left a comment

azdagron Aug 17, 2021

azdagron Aug 17, 2021

azdagron Aug 17, 2021

azdagron Aug 17, 2021

azdagron Aug 17, 2021

azdagron Aug 17, 2021

azdagron Aug 17, 2021

MarcosDY Aug 17, 2021

azdagron Aug 17, 2021

azdagron Aug 17, 2021

azdagron Aug 17, 2021

azdagron left a comment

	\| `default_all_bundle_sname` \| The Validation Context resource name to use when fetching X.509 bundle together with federated bundles with Envoy SDS \| ALL \|
	\| `default_all_bundles_name` \| The Validation Context resource name to use when fetching X.509 bundle together with federated bundles with Envoy SDS \| ALL \|

	Another option is to request for `ALL` that creates a [SPIFFE Certificate Validator](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto) that contains TrustDomain together with all Federated Bundles. This feature is supported from Envoy v1.18.0.
	Another option is to request for `ALL` that creates a [SPIFFE Certificate Validator](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto) that contains TrustDomain together with all Federated Bundles. This feature is supported starting with Envoy v1.18.0.

	// Use RootCA as default, but replace for SPIFFE auth when envoy version is at least v1.18.0
	// Use RootCA as default, but replace with SPIFFE auth when Envoy version is at least v1.18.0

	return nil, status.Error(codes.Internal, `unable to use "SPIFFE validator" on envoy below 1.17`)
	return nil, status.Error(codes.Internal, `unable to use "SPIFFE validator" on Envoy below 1.17`)

	logger.Warn(`"default_bundle_name" and "default_all_bundles_name" has the same value, "default_bundle_name" will be used. In future versions this misconfiguration will cause agent to not start.`)
	logger.Warn(`The "default_bundle_name" and "default_all_bundles_name" configurables have the same value. "default_all_bundles_name" will be ignored. Please configure distinct values or use the defaults. This will be a configuration error In a future release.`)

	The default name is configurable (see `default_all_bundles_name` under [SDS Configuration](#sds-configuration)
	The default name is configurable (see `default_all_bundles_name` under [SDS Configuration](#sds-configuration).

	// Add td bundle if provided
	// Only include tdBundle if it is not nil, which shouldn't ever be the case. This is purely defensive.

	return version.MajorNumber >= 1 && version.MinorNumber > 17
	return (version.MajorNumber == 1 && version.MinorNumber > 17) \|\| version.MajorNumber > 1

SDS v3 endpoints support SPIFFE validator #2435

SDS v3 endpoints support SPIFFE validator #2435

Conversation

MarcosDY commented Aug 3, 2021

amartinezfayo 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

azdagron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azdagron 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azdagron left a comment

Choose a reason for hiding this comment