CertAuthority watcher filtering by espadolini · Pull Request #10020 · gravitational/teleport

espadolini · 2022-01-28T17:28:37Z

This PR:

fixes the filtering on CA updates that trigger a rotation state sync
makes it so that watchers can filter CertAuthority resources by type (host/user/jwt) and name, and adds the appropriate CA filter to the cache for Node roles (host CAs with a name that matches the cluster name, all user CAs) and for the syncRotationStateCycle loop (only host CAs from the current cluster)
adds the same filtering support to the cache initialization, so we don't fetch unnecessary data
adds filtering on the server side for older node clients, gated by version number
fixes potential inconsistencies in CAs when adding a trusted cluster/remote cluster relationship
filters out spurious CA updates that would result in no changes

The version cutoff for applying the server-side filter was set to 9.0.0; any backport at v7 and earlier should remove the client side filters and rely on that, as clients are allowed to connect to auth servers with a higher major version that don't have the backport yet.

espadolini · 2022-02-01T23:43:38Z

Open questions:

should we add a CertAuthorityFilter in protobuf form, if it's only ever used for translating to and from a watcher's filter map?
does the design of the filter need product decision? It's technically exposed API
how should the version cutoff work? Anything less than the next point release at time of backport? Should it be backport-only?

fspmarshall

Added some minor nits, but LGTM overall.

rosstimothy

Overall looks good. Can you add some test coverage for the things you added/fixed?

This still lets us do the CA filtering for the Node cache and the syncRotationStateCycle watcher without adding new data to CAs. As a bonus, the cache can now fetch only the required CAs on initialization.

r0mant · 2022-02-18T21:39:04Z

+		if services.CertAuthoritiesEquivalent(existing, ca) {
+			return nil
+		}


@espadolini @fspmarshall I'm a little worried that silently skipping updates like this might result into very subtle bugs where e.g. we forget to update CertAuthoritiesEquivalent method after introducing another CA field or something. What do you think about at least logging it here?

CertAuthoritiesEquivalent is defined as cmp.Equal skipping the metadata ID field so we shouldn't miss anything. Adding a debug log line would be fine, anything more would get quite spammy however.

Ah yes, I forgot it does cmp.Equal, should be fine then.

r0mant · 2022-02-18T21:39:27Z

 				continue
 			}
-			if ca.GetType() != types.HostCA && ca.GetClusterName() != conn.ClientIdentity.ClusterName {
+			if ca.GetType() != types.HostCA || ca.GetClusterName() != conn.ClientIdentity.ClusterName {


Is that intended change?

Yes, the original version had a bug and called syncRotationStateAndBroadcast unnecessarily often; syncRotationStateAndBroadcast only ever cares about the local host CA, so it should be called when just that one has potentially changed.

Got it, thanks for the clarification.

Including similar changes to the v7 backport

CertAuthority watcher filtering (#10020) Including similar changes to the v7 backport

espadolini force-pushed the espadolini/ca-watcher-filter branch 7 times, most recently from 293c5d5 to 8954967 Compare February 1, 2022 23:02

espadolini requested a review from fspmarshall February 1, 2022 23:22

espadolini force-pushed the espadolini/ca-watcher-filter branch from 8954967 to bf9376b Compare February 1, 2022 23:26

espadolini added 4 commits February 2, 2022 00:26

Fix potential inconsistency in CAs when adding a remote cluster

dd928d4

Fix filtering on CA updates that should trigger a rotation state sync

0925870

Keep track of the trust relationship of CertAuthorities

c8f7b4f

Allow filtering CertAuthority watchers by trust relationship

e031f4e

espadolini force-pushed the espadolini/ca-watcher-filter branch from bf9376b to 2d2cb6d Compare February 1, 2022 23:27

espadolini changed the title ~~CA replication filter~~ CertAuthority watcher filtering Feb 1, 2022

WIP: CertAuthority filtering on the server side for older nodes

04db0a4

espadolini force-pushed the espadolini/ca-watcher-filter branch from 2d2cb6d to 04db0a4 Compare February 2, 2022 10:40

russjones requested a review from rosstimothy February 2, 2022 18:08

fspmarshall previously approved these changes Feb 2, 2022

View reviewed changes

Comment thread lib/auth/grpcserver.go Outdated

Comment thread lib/auth/grpcserver.go Outdated

Comment thread lib/auth/rotate.go Outdated

Comment thread lib/auth/trustedcluster.go Outdated

Comment thread lib/auth/trustedcluster.go Outdated

Comment thread lib/auth/trustedcluster.go Outdated

espadolini added 3 commits February 2, 2022 23:22

Add a CertAuthorityFilter utility type

4fc1eaa

Renames to make trust relationships more clear

7007ed6

Version cutoff for the server-side filter injection

ae51ee5

espadolini marked this pull request as ready for review February 3, 2022 14:48

github-actions Bot requested review from gabrielcorado and zmb3 February 3, 2022 14:49

espadolini removed request for gabrielcorado and zmb3 February 3, 2022 15:39

rosstimothy reviewed Feb 3, 2022

View reviewed changes

Add trust relationship checks in the trustedClusters() integration test

e26a3bd

espadolini added 3 commits February 15, 2022 18:38

Switch the CA filtering to name+type instead

d8a0370

This still lets us do the CA filtering for the Node cache and the syncRotationStateCycle watcher without adding new data to CAs. As a bonus, the cache can now fetch only the required CAs on initialization.

Ignore unknown WatchKind filters for the future

569b1d1

Remove trust labels

bf1221a

espadolini force-pushed the espadolini/ca-watcher-filter branch from 2e089d2 to bd70797 Compare February 15, 2022 17:43

Don't unnecessarily upsert unchanged CAs

67d97a0

espadolini force-pushed the espadolini/ca-watcher-filter branch 2 times, most recently from ead3b88 to 67d97a0 Compare February 16, 2022 00:49

rosstimothy reviewed Feb 16, 2022

View reviewed changes

Comment thread lib/auth/grpcserver.go Outdated

rosstimothy approved these changes Feb 16, 2022

View reviewed changes

fspmarshall approved these changes Feb 16, 2022

View reviewed changes

Comment thread lib/auth/grpcserver.go

Comment thread lib/auth/grpcserver.go Outdated

Comment thread lib/cache/collections.go Outdated

espadolini force-pushed the espadolini/ca-watcher-filter branch from 103898e to 051ed4c Compare February 17, 2022 08:37

espadolini added 2 commits February 17, 2022 09:39

Revert changes to the certAuthority collector

7cee49e

Undo spurious changes that are no longer related

f148698

espadolini force-pushed the espadolini/ca-watcher-filter branch from 051ed4c to f148698 Compare February 17, 2022 08:39

Use Wildcard instead of a literal asterisk in CertAuthorityFilter

4fd3f16

espadolini added the merge-for-v9 label Feb 18, 2022

espadolini force-pushed the espadolini/ca-watcher-filter branch from cadf294 to dd40196 Compare February 18, 2022 17:08

Bump the version cutoff for the filter injection to v9

e8f2a9b

espadolini force-pushed the espadolini/ca-watcher-filter branch from dd40196 to e8f2a9b Compare February 18, 2022 19:45

r0mant reviewed Feb 18, 2022

View reviewed changes

r0mant approved these changes Feb 19, 2022

View reviewed changes

r0mant enabled auto-merge (squash) February 19, 2022 00:27

Merge branch 'master' into espadolini/ca-watcher-filter

a1dda63

r0mant merged commit 6033148 into master Feb 19, 2022

r0mant deleted the espadolini/ca-watcher-filter branch February 19, 2022 00:48

espadolini added a commit that referenced this pull request Mar 2, 2022

CertAuthority watcher filtering (#10020)

b7b7d05

espadolini mentioned this pull request Mar 2, 2022

[v7] backport #10741 (leaf cluster CA sanitizing) #10744

Merged

espadolini added a commit that referenced this pull request Jun 29, 2022

CertAuthority watcher filtering (#10020)

64bed35

Including similar changes to the v7 backport

espadolini mentioned this pull request Jun 29, 2022

[v8] backport #10020 (CertAuthority watcher filtering) #13961

Merged

espadolini added a commit that referenced this pull request Jun 29, 2022

[v8] backport #10020 (CertAuthority watcher filtering) (#13961)

3f553eb

CertAuthority watcher filtering (#10020) Including similar changes to the v7 backport

Conversation

espadolini commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

espadolini commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fspmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rosstimothy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

r0mant Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

espadolini Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

r0mant Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

r0mant Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

espadolini Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

r0mant Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

espadolini commented Jan 28, 2022 •

edited

Loading

espadolini commented Feb 1, 2022 •

edited

Loading