Support foreign trust domains admin ids config #3642

guilhermocc · 2022-11-28T20:10:36Z

Pull Request check list ✔️

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

Affected functionality 🤔

admin_ids configuration values can now reside in different trust domains than the server, but this foreign trust domain must be federated.

Description of change ✍️

Updated admin_ids configuration validation
Updated endpoints getCerts method, to include federated bundle CAs for establishing TLS connections with spire server.

Which issue this PR fixes ❓
fixes #3400

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

…nection Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

azdagron · 2022-11-29T20:29:02Z

pkg/server/endpoints/endpoints.go

-		rootCACerts, err := x509.ParseCertificates(rootCA.DerBytes)
-		if err != nil {
-			return nil, nil, fmt.Errorf("parse bundle: %w", err)
+	for _, bundle := range listBundlesResponse.Bundles {


We should probably scope which bundles we use here to the trust domain for the server and other trust domains expected in admin_ids.

azdagron · 2022-11-29T20:30:32Z

pkg/server/endpoints/endpoints.go

-	caPool := x509.NewCertPool()
-	for _, c := range caCerts {
-		caPool.AddCert(c)
+		for _, c := range caCerts {


Adding all the certs to the same CA pool is a security risk. It allows trust domain impersonation since any trust domain could mint an SVID for any other trusted trust domain and have it authenticated.

We might need a small refactor to this function to go proper spiffe authentication. The tlsconfig package might prove helpful.

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc · 2022-12-02T17:29:42Z

pkg/server/endpoints/endpoints.go

+// certificate is not provided, the function will not make any verification and return nil.
+func (e *Endpoints) buildServerSpiffeAuthenticationFunction(bundleSet *x509bundle.Set) func(_ [][]byte, _ [][]*x509.Certificate) error {
+	return func(rawCerts [][]byte, _ [][]*x509.Certificate) error {
+		if rawCerts == nil {


This is the same verification logic that occurs on go-spiffe mtls configuration, except that it skips verification for connections with empty cert (so we can still accept connections with no auth in spire server), and applies a custom authorization logic that is a combination of tlsconfig.AuthorizeMemberOf and tlsconfig.AuthorizeOneOf authorizers

guilhermocc · 2022-12-02T17:34:51Z

pkg/server/endpoints/endpoints.go

+				e.Log.
+					WithField(telemetry.AdminID, adminID.String()).
+					WithField(telemetry.TrustDomain, adminID.TrustDomain().String()).
+					Warn("No bundle found for foreign admin trust domain, admins from this trust doiman will not be able to connect")


If an adminID from a foreign trust domain not yet federated with the spire server is configured, this warning will be triggered for all connection attempts. This could have been prevented if we had validation for admin_ids configuration during server start-up, what do you guys think about this? should I put work on another PR implementing this validation, or this warning log (and the documentation) should be enough for an operator to understand and resolve this kind of misconfiguration?

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

azdagron

Thanks, @guilhermocc! In general I think this is looking good.

azdagron · 2022-12-14T14:00:33Z

conf/server/server_full.conf

+	# that caller admin privileges. The admin IDs must reside in a federated trust domain or on the same as the server,
+  # and need not have a corresponding admin registration.


There is something up with the indentation here. Also, i think this file has imposed a column limit on comments, we should probably stick with that.

azdagron · 2022-12-14T14:01:49Z

doc/spire_server.md

-| `trust_domain`          | The trust domain that this server belongs to (should be no more than 255 characters)                                                                                                                                                    |                                                                |
+| Configuration           | Description                                                                                                                                                                                                                                                | Default                                                        |
+|:------------------------|:-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:---------------------------------------------------------------|
+| `admin_ids`             | SPIFFE IDs that, when present in a caller's X509-SVID, grant that caller admin privileges. The admin IDs must reside in a federated trust domain or on the same as the server, and need not have a corresponding admin registration entry with the server. |                                                                |


Suggested change

| `admin_ids` | SPIFFE IDs that, when present in a caller's X509-SVID, grant that caller admin privileges. The admin IDs must reside in a federated trust domain or on the same as the server, and need not have a corresponding admin registration entry with the server. | |

| `admin_ids` | SPIFFE IDs that, when present in a caller's X509-SVID, grant that caller admin privileges. The admin IDs can be from any trust domain and need not have a corresponding admin registration entry with the server. | |

azdagron · 2022-12-14T14:03:56Z

pkg/server/endpoints/auth.go

+						"Make sure this trust domain is correctly federated.",
+				)
+		}
+		return nil, fmt.Errorf("no bundle found for trust domain %s", e.TrustDomain.String())


String() method is implicitly called when using %s or %q formatting:

Suggested change

return nil, fmt.Errorf("no bundle found for trust domain %s", e.TrustDomain.String())

return nil, fmt.Errorf("no bundle found for trust domain %q", e.TrustDomain)

azdagron · 2022-12-14T14:04:32Z

pkg/server/endpoints/auth.go

+			e.Log.
+				WithField(telemetry.TrustDomain, td.String()).
+				Warn(
+					"No bundle found for foreign admin trust domain, admins from this trust domain will not be able to connect. " +


Suggested change

"No bundle found for foreign admin trust domain, admins from this trust domain will not be able to connect. " +

"No bundle found for foreign admin trust domain; admins from this trust domain will not be able to connect. " +

azdagron · 2022-12-14T14:21:23Z

pkg/server/endpoints/auth.go

+	return func(peerSpiffeID spiffeid.ID) error {
+		permissiveIDsSet := make(map[spiffeid.ID]struct{})
+		for _, adminID := range adminIds {
+			permissiveIDsSet[adminID] = struct{}{}
+		}


Let's lift this out of the closure so that we don't recompute this map on every invocation of the matcher.

Suggested change

return func(peerSpiffeID spiffeid.ID) error {

permissiveIDsSet := make(map[spiffeid.ID]struct{})

for _, adminID := range adminIds {

permissiveIDsSet[adminID] = struct{}{}

}

permissiveIDsSet := make(map[spiffeid.ID]struct{})

for _, adminID := range adminIds {

permissiveIDsSet[adminID] = struct{}{}

}

return func(peerID spiffeid.ID) error {

azdagron · 2022-12-14T19:00:59Z

pkg/server/endpoints/endpoints.go

+		})
+
+		spiffeTLSConfig := tlsconfig.MTLSServerConfig(serverSvid, bundle, nil)
+		spiffeTLSConfig.ClientAuth = tls.RequestClientCert


Can we leave a note here just emphasizing that client certificates will still be verified using the custom VerifyPeerCertificates callback?

azdagron · 2022-12-14T19:03:58Z

pkg/server/endpoints/endpoints_test.go

+			_, err = entryv1.NewEntryClient(conn).ListEntries(ctx, nil)
+			require.Error(t, err)


Instead of actually issuing an RPC we can use grpc.FailOnNonTempDialError(true) and grpc.WithReturnConnectionError() to cause the dial to fail when the handshake is unsuccessful.

azdagron · 2022-12-14T19:08:58Z

pkg/server/endpoints/endpoints.go

-	}
-
+// getServerCertificate returns the server certificate to be used in the TLS config.
+func (e *Endpoints) getServerCertificate() *tls.Certificate {
 	svidState := e.SVIDObserver.State()


I wonder if we can change the SVIDObserver to store an x509svid.SVID instead of the tls.Certificate and push the parsing the conversion to the server SVID rotator so we don't have to repeat it on each handshake.

azdagron · 2022-12-14T19:10:24Z

pkg/server/endpoints/auth.go

+}
+
+// parseBundle parses a *x509bundle.Bundle from a *common.bundle.
+func parseBundle(td spiffeid.TrustDomain, commonBundle *common.Bundle) (*x509bundle.Bundle, error) {


It's a little unfortunate that we parse this on each handshake. I think we are already doing that today, but we should probably push this conversion behind the cache so we don't have to do it so much. That can happen in a later PR though.

azdagron · 2022-12-14T19:11:48Z

pkg/server/endpoints/auth.go

+
+// shouldLogFederationMisconfiguration returns true if the last time a misconfiguration
+// was logged was more than misconfigLogEvery ago.
+func shouldLogFederationMisconfiguration() bool {


hmm... should we be doing this logging rate limiting per trust domain?

Yep, that's a good idea!

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

MarcosDY · 2022-12-20T14:54:51Z

pkg/server/endpoints/auth.go

+
+// machMemberOrOneOf is a custom spiffeid.Matcher which will validate that the peerSpiffeID belongs to the server
+// trust domain or if it is included in the admin_ids configuration permissive list.
+func machMemberOrOneOf(trustDomain spiffeid.TrustDomain, adminIds ...spiffeid.ID) spiffeid.Matcher {


Suggested change

func machMemberOrOneOf(trustDomain spiffeid.TrustDomain, adminIds ...spiffeid.ID) spiffeid.Matcher {

func matchMemberOrOneOf(trustDomain spiffeid.TrustDomain, adminIds ...spiffeid.ID) spiffeid.Matcher {

Thanks @MarcosDY 🙈
3f743fc

MarcosDY · 2022-12-20T16:05:22Z

pkg/server/endpoints/auth.go

+
+	serverBundle, err := parseBundle(e.TrustDomain, commonServerBundle)
+	if err != nil {
+		return nil, fmt.Errorf("parse bundle: %w", err)


this error will result in: "parse bundle: parse bundle: SOME ERROR" I think we may remove fmt error from here or from parseBundle

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

azdagron

This is looking pretty close! Just a few small comments.

azdagron · 2023-01-04T02:50:49Z

pkg/server/endpoints/auth.go

+						"Make sure this trust domain is correctly federated.",
+				)
+		}
+		return nil, fmt.Errorf("no bundle found for trust domain %q", e.TrustDomain)


Shouldn't these be logging td?

Suggested change

return nil, fmt.Errorf("no bundle found for trust domain %q", e.TrustDomain)

return nil, fmt.Errorf("no bundle found for trust domain %q", td)

azdagron · 2023-01-04T02:51:27Z

pkg/server/endpoints/auth.go

+
+var (
+	misconfigLogMtx   sync.Mutex
+	misconfigLogTimes = make(map[string]time.Time)


nit: we should use the concrete trust domain and ID types when possible

Suggested change

misconfigLogTimes = make(map[string]time.Time)

misconfigLogTimes = make(map[spiffeid.ID]time.Time)

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

azdagron · 2023-01-04T15:02:32Z

pkg/server/endpoints/auth.go

+	return func(rawCerts [][]byte, _ [][]*x509.Certificate) error {
+		if rawCerts == nil {
+			return nil
+		}
+
+		return tlsconfig.VerifyPeerCertificate(
+			bundleSource,
+			tlsconfig.AdaptMatcher(matchMemberOrOneOf(e.TrustDomain, e.AdminIDs...)),
+		)(rawCerts, nil)


nit: we should probably create this function before the closure to avoid unnecessary allocations on the handshake:

Suggested change

return func(rawCerts [][]byte, _ [][]*x509.Certificate) error {

if rawCerts == nil {

return nil

}

return tlsconfig.VerifyPeerCertificate(

bundleSource,

tlsconfig.AdaptMatcher(matchMemberOrOneOf(e.TrustDomain, e.AdminIDs...)),

)(rawCerts, nil)

verifyPeerCertificate := tlsconfig.VerifyPeerCertificate(

bundleSource,

tlsconfig.AdaptMatcher(matchMemberOrOneOf(e.TrustDomain, e.AdminIDs...)),

)

return func(rawCerts [][]byte, _ [][]*x509.Certificate) error {

if rawCerts == nil {

return nil

}

return verifyPeerCertificate(rawCerts, nil)

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

azdagron

\o/, thanks for this @guilhermocc !

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc added 4 commits November 28, 2022 11:37

Fix comments typos

bd3af89

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Remove foreign trust domain validation for adminIDs server config

a42ecfb

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Update getCerts method to include federated bundles for admin Tls con…

aebc767

…nection Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Update documentation regarding new admin_ids behavior

890fd0f

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc requested review from evan2645, amartinezfayo, azdagron, MarcosDY and rturner3 as code owners November 28, 2022 20:10

Merge branch 'main' into support-foreign-trust-domain-admin-ids-config

1ba2fb3

azdagron reviewed Nov 29, 2022

View reviewed changes

Implement spiffe authentication in spire server

6ad6274

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc force-pushed the support-foreign-trust-domain-admin-ids-config branch 2 times, most recently from 934cb12 to bcb47f4 Compare December 2, 2022 13:27

Add error log when some problem occurs creating TLS config for server

e1c306e

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc force-pushed the support-foreign-trust-domain-admin-ids-config branch from bcb47f4 to e1c306e Compare December 2, 2022 13:28

guilhermocc requested a review from azdagron December 2, 2022 13:54

Merge branch 'main' into support-foreign-trust-domain-admin-ids-config

7291f2c

guilhermocc commented Dec 2, 2022

View reviewed changes

Merge branch 'main' into support-foreign-trust-domain-admin-ids-config

8d98af1

rturner3 assigned azdagron Dec 6, 2022

guilhermocc and others added 3 commits December 13, 2022 22:28

Use tlsconfig config and isolate auth logic in new file

af453cf

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Improve unfederated foreign admin id warning message

c9df0ee

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Merge branch 'main' into support-foreign-trust-domain-admin-ids-config

0be5985

guilhermocc force-pushed the support-foreign-trust-domain-admin-ids-config branch from 94fdee3 to 7dea1ab Compare December 14, 2022 14:54

Remove duplicated opts

55c936f

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc force-pushed the support-foreign-trust-domain-admin-ids-config branch from 7dea1ab to 55c936f Compare December 14, 2022 17:08

Fix flaky test

fa07255

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc force-pushed the support-foreign-trust-domain-admin-ids-config branch from 1afff30 to fa07255 Compare December 14, 2022 18:26

azdagron reviewed Dec 14, 2022

View reviewed changes

Improve code addressing review comments

ec23d9c

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc requested a review from azdagron December 16, 2022 21:34

Merge branch 'main' into support-foreign-trust-domain-admin-ids-config

25a95b9

MarcosDY reviewed Dec 20, 2022

View reviewed changes

Fix typo in function name

3f743fc

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

evan2645 added this to the 1.5.4 milestone Dec 20, 2022

azdagron reviewed Jan 4, 2023

View reviewed changes

guilhermocc added 2 commits January 4, 2023 11:16

Use concrete trust domain id type and fix log

e70e7b0

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Improve argument naming

ec21334

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

azdagron reviewed Jan 4, 2023

View reviewed changes

Remove unnecessary allocations from the handshake

5c51de8

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

azdagron approved these changes Jan 4, 2023

View reviewed changes

Merge branch 'main' into support-foreign-trust-domain-admin-ids-config

47abc90

amartinezfayo merged commit 7bff3a0 into spiffe:main Jan 5, 2023

amartinezfayo pushed a commit that referenced this pull request Jan 5, 2023

Support foreign trust domains admin ids config (#3642)

8e324a5

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc mentioned this pull request Jan 6, 2023

Optimize bundle conversion amount in spire server TLS connections #3744

Closed

stevend-uber pushed a commit to stevend-uber/spire that referenced this pull request Oct 16, 2023

Support foreign trust domains admin ids config (spiffe#3642)

ffc508f

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support foreign trust domains admin ids config #3642

Support foreign trust domains admin ids config #3642

guilhermocc commented Nov 28, 2022 •

edited

Loading

azdagron Nov 29, 2022

azdagron Nov 29, 2022

guilhermocc Dec 2, 2022

guilhermocc Dec 2, 2022 •

edited

Loading

azdagron left a comment

azdagron Dec 14, 2022

azdagron Dec 14, 2022

azdagron Dec 14, 2022

azdagron Dec 14, 2022

azdagron Dec 14, 2022

azdagron Dec 14, 2022

guilhermocc Dec 14, 2022

azdagron Dec 14, 2022

azdagron Dec 14, 2022

azdagron Dec 14, 2022

azdagron Dec 14, 2022

guilhermocc Dec 14, 2022

MarcosDY Dec 20, 2022

guilhermocc Dec 20, 2022

MarcosDY Dec 20, 2022

guilhermocc Dec 20, 2022

azdagron left a comment

azdagron Jan 4, 2023

guilhermocc Jan 4, 2023

guilhermocc Jan 4, 2023

azdagron Jan 4, 2023

guilhermocc Jan 4, 2023

azdagron Jan 4, 2023

azdagron left a comment

		# that caller admin privileges. The admin IDs must reside in a federated trust domain or on the same as the server,
		# and need not have a corresponding admin registration.

	\| `admin_ids` \| SPIFFE IDs that, when present in a caller's X509-SVID, grant that caller admin privileges. The admin IDs must reside in a federated trust domain or on the same as the server, and need not have a corresponding admin registration entry with the server. \| \|
	\| `admin_ids` \| SPIFFE IDs that, when present in a caller's X509-SVID, grant that caller admin privileges. The admin IDs can be from any trust domain and need not have a corresponding admin registration entry with the server. \| \|

	return nil, fmt.Errorf("no bundle found for trust domain %s", e.TrustDomain.String())
	return nil, fmt.Errorf("no bundle found for trust domain %q", e.TrustDomain)

	"No bundle found for foreign admin trust domain, admins from this trust domain will not be able to connect. " +
	"No bundle found for foreign admin trust domain; admins from this trust domain will not be able to connect. " +

		_, err = entryv1.NewEntryClient(conn).ListEntries(ctx, nil)
		require.Error(t, err)

	func machMemberOrOneOf(trustDomain spiffeid.TrustDomain, adminIds ...spiffeid.ID) spiffeid.Matcher {
	func matchMemberOrOneOf(trustDomain spiffeid.TrustDomain, adminIds ...spiffeid.ID) spiffeid.Matcher {

	misconfigLogTimes = make(map[string]time.Time)
	misconfigLogTimes = make(map[spiffeid.ID]time.Time)

Support foreign trust domains admin ids config #3642

Support foreign trust domains admin ids config #3642

Conversation

guilhermocc commented Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhermocc Dec 2, 2022 • edited Loading

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

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

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

guilhermocc commented Nov 28, 2022 •

edited

Loading

guilhermocc Dec 2, 2022 •

edited

Loading