Create Database CA #9593

jakule · 2021-12-29T20:16:21Z

This PR introduces a new Certificate Authority type: Database CA.

New CA is introduced for signing certificates used in Database access.
New migration has been added to migrated existing HostCA (which is currently used for signing DB certs) as a Database CA. This will allow to upgrade existing clusters to v9.
For new installations Database CA will be generated in the same way as other certificates on the first start up.

Closes #5029

api/types/trust.go

lib/auth/init.go

r0mant · 2022-01-12T03:00:29Z

lib/cache/collections.go

 	processEvent(ctx context.Context, e types.Event) error
 	// watchKind returns a watch
 	// required for this collection
 	watchKind() types.WatchKind
-	// erase erases all data in the collection
-	erase(ctx context.Context) error


Was this unused?

Yes, I think that not the only method that was not used. I didn't want to clean up all of them as this is not the goal of this PR.

lib/services/authority.go

lib/auth/init.go

r0mant · 2022-01-12T03:11:13Z

lib/auth/init.go

+	// Copy the Host CA as Database CA.
+	log.Infof("Migrating Database CA")
+
+	cav2, ok := hostCA.(*types.CertAuthorityV2)


Instead of copying the entire resource, how about we just make a new resource for the database CA and populate TLS key pairs appropriately? This way we don't end up with unnecessary fields in the new CA (e.g. SSH keys).

r0mant · 2022-01-12T03:12:42Z

lib/auth/init.go

+	err = asrv.Trust.CreateCertAuthority(cav2)
+	if err != nil {
+		return trace.Wrap(err)
+	}


We usually recommend scaling auth servers to one before upgrading but not sure how many people actually do that. So there is a possibility of a race if multiple auth servers are attempting the migration concurrently - one will create the CA here and then the other one will get an error. I would handle AlreadyExists error here.

r0mant · 2022-01-12T03:15:44Z

lib/auth/db.go

+	databaseCA, err := s.GetCertAuthority(types.CertAuthID{
+		Type:       types.DatabaseCA,
 		DomainName: clusterName.GetClusterName(),
 	}, true)


We need a fallback to HostCA here. In general, the proper upgrade procedure is to upgrade auth servers first but in case there's a newer database agent connects to an older auth server, this will fail. It seems easy enough to do a fallback here.

r0mant · 2022-01-12T03:48:34Z

lib/auth/init.go

@@ -1020,6 +1028,57 @@ func migrateCertAuthorities(ctx context.Context, asrv *Server) error {
 	return nil
 }

+// migrateDBAuthority copies Host CA as Database CA. Before v9.0 database access was using host CA to sign all


I'm thinking more about this and now pondering whether this is a good approach from the security standpoint. The scenario I've in my mind is this: say your host CA gets compromised, you rotate it but aren't aware that your db CA after this migration is the same and that you need to rotate it too. In a situation like this, you would probably rotate all your CAs anyway but I wonder if this may not be a desired outcome following the principle of least surprise.

@fspmarshall @russjones Do you guys have any thoughts about this migration approach versus just leaving database CA empty and falling back to host CA and letting users explicitly initialize their database CA by performing rotation? We considered both and chose this one for several reasons (e.g. we'd likely need to update rotation logic to support empty CAs for the other one, instruct users to perform rotation after upgrade if they want to migrate to separate database CA, etc.) but now wondering if this might not be the best way.

@r0mant I find the scenario you described pretty convincing. If a user wants a CA gone, we should protect them from accidentally leaving it in circulation like that.

If updating rotation logic to support empty CAs is tricky, what about updating it to check for duplicate CAs and rotate all CAs that match the targeted CA? E.g. if I try to rotate my host CA, and teleport notices that the DB ca happens to be equivalent, could it just automatically rotate it in lockstep? Injecting an additional CA type here would be fairly straightforward.

While doing that kind of thing silently is generally sketchy, I think we can safely assume that we won't have matching CAs by chance, meaning that it will basically always be a case like this one; we've split the duties that one CA used to perform into two or more separate CAs, and are now rotating for the first time since that split.

Makes sense. @jakule Could you please take a look which approach looks more straightforward to implement (supporting empty CA vs rotating all matching CAs in lockstep) and let's do it.

@r0mant I'll look into that. Right now I'm leaning towards rotating all matching certificates. Fallback is probably simpler to implement, but if we decide to go this route, I don't think we will be able to remove that logic anytime soon, as we don't know when everyone decide to rotate their certs. Rotating all matching certs keeps the logic in one place and allows us to reuse it if we ever need it (Desktop Access for example).

smallinsky

tctl auth export command handler also need to updated to handle DbCA : https://github.com/gravitational/teleport/blob/jakule/database-ca/tool/tctl/common/auth_command.go#L178
https://goteleport.com/docs/setup/reference/cli/#tctl-auth-export

Also the DBCa seems to be missing in updating trusted cluster flow:

teleport/lib/auth/trustedcluster.go

Line 519 in 9ac75d0

for _, caType := range []types.CertAuthType{types.HostCA, types.UserCA} {

smallinsky · 2022-01-17T10:15:54Z

lib/auth/init.go

@@ -313,8 +313,14 @@ func Init(cfg InitConfig, opts ...ServerOption) (*Server, error) {
 	}
 	log.Infof("Created namespace: %q.", apidefaults.Namespace)

+	// Migrate Host CA as Database CA before certificates generation. Otherwise, the Database CA will be
+	// generated which we don't want for existing installations.
+	if err := migrateDBAuthority(asrv); err != nil {


I think that for this func should be called after migrateLegacyResources is invoked or migrateLegacyResources should be updated to take into account DB CA.

I was thinking about it, but if a CA is missing we will create it here:

teleport/lib/auth/init.go

Line 323 in 9ac75d0

for _, caType := range types.CertAuthTypes {

Then there is nothing to "migrate" in migrateLegaceResources as DB CA has been already created. That's why I created a new function and I call it before we create any new cert.

zmb3

LGTM assuming all of Roman's comments are addressed.

integration/helpers.go

jakule · 2022-01-26T18:35:25Z

tctl auth export command handler also need to updated to handle DbCA : https://github.com/gravitational/teleport/blob/jakule/database-ca/tool/tctl/common/auth_command.go#L178 https://goteleport.com/docs/setup/reference/cli/#tctl-auth-export

Also the DBCa seems to be missing in updating trusted cluster flow:

teleport/lib/auth/trustedcluster.go

Line 519 in 9ac75d0

for _, caType := range []types.CertAuthType{types.HostCA, types.UserCA} {

@smallinsky Thanks. Good catch. I missed that.

r0mant · 2022-01-26T22:30:33Z

lib/auth/db.go

@@ -89,7 +99,7 @@ func getServerNames(req *proto.DatabaseCertRequest) []string {

 // SignDatabaseCSR generates a client certificate used by proxy when talking
 // to a remote database service.
-func (s *Server) SignDatabaseCSR(ctx context.Context, req *proto.DatabaseCSRRequest) (*proto.DatabaseCSRResponse, error) {
+func (s *Server) SignDatabaseCSR(_ context.Context, req *proto.DatabaseCSRRequest) (*proto.DatabaseCSRResponse, error) {


@jakule I have just noticed that we're still using HostCA here. I think it should be updated as well.

@r0mant
The HostCA flag in scope of SignDatabaseCSR method is used to obtain HostCA certificate and add HostCA to the trusted pool were will used during reverse tunnel connection [teleport db proxy <--> db service ] mTLS handshake. DB service terminates the incoming connection with a server identity that was generated by HostCA during RegisterUsingToken (or LocalRegister if DB service is running with auth service) call.

The connection established with a cert generated by the SignDatabaseCSR method is teleport "internal component" connection between teleport proxy and DB agent thus do we really want to introduce different server identity generation for RoleDatabase SystemRole for DB service and sign RoleDatabase identity with DatabaseCA ?

@smallinsky @jakule So there are two aspects to it I think:

There's an agent (process) identity that gets generated when a new agent starts up. Currently it is signed by the Host CA. I think that ideally each agent's identity would also be signed by their respective CA (i.e. SSH nodes - by Host CA, database agents - by Database CA, desktop agents - by Desktop CA, etc.) but that would likely be a pretty large change and it's a little orthogonal to what this PR does. So I propose to leave agent identity as it is for now.

There's a certificate that's generated by this SignDatabaseCSR method that as you guys mentioned is used to transfer identity information over reverse tunnel to the database agent. I think it should use Database CA instead of Host CA as I suggested here. It makes sense that when communicating with a database agent the proxy would use the Database CA to sign those certs. This way one compromised CA does not affect others. It should be pretty easy to do if we make sure that database agent loads both Host CA and Database CA in its client cert pool.

One thing to keep in mind is that we should maintain backwards compatibility to avoid situation where a new Proxy signs a certificate with Database CA which older database agent can't verify (our upgrade instructions say to upgrade Proxies before agents). To make sure compatibility is not broken, Proxy can check the version of the agent it's connecting to (Teleport version should be available as a part of DatabaseServer resource) and sign the cert with Host CA if it's talking to an older agent (e.g. < 9.x) and with Database CA otherwise. Then in Teleport 10 we will remove Host CA from database agents' client pools.

WDYGT?

Yes, Agree Connecting to an DB agent service with certs generated by DatabaseCA based on the agent version where DatabaseCA will be added to the DB agent poll sounds reasonable and should be pretty easy to implement.
Though without changing agent (process) identity signer to DatabaseCA we will still be using HostCA pool on the proxy side to verify cert presented by DB agent during reverse tunnel connection. But changing this might be more complicated ( for example identify is generated once and cached on local agent backend) and probably should be kept out of scope for this PR.

Remove SSH key form Database CA

jakule · 2022-03-18T16:13:52Z

I can see that PR updates the webassets and e submodule that was intended ?

@smallinsky No, those changes were not intended. I think I did something unusual when resolving the latest git merge conflicts. I reverted them. Thanks for pointing that out.

Co-authored-by: Marek Smoliński <[email protected]>

r0mant · 2022-03-18T18:37:45Z

lib/auth/rotate.go

+
+// findDuplicatedCertificates checks if any CA provided as caTypes has a duplicate in allCerts. List of all duplicates
+// is returned as a result. If no duplicates are found, then caTypes is returned in unmodified form.
+func findDuplicatedCertificates(caTypes []types.CertAuthType, allCerts CertAuthorityMap) []types.CertAuthType {


If we found a match here, I think we should print a warning for the user that their database CA will be rotated alongside host CA (and vice versa). Otherwise they might not expect it and it will break their database access.

lib/srv/db/proxyserver.go

r0mant · 2022-03-18T18:42:16Z

lib/srv/db/proxyserver.go

-		ClusterName: identity.RouteToCluster,
+		CSR:                csr,
+		ClusterName:        identity.RouteToCluster,
+		SignWithDatabaseCA: teleportVer.LessThan(*semver.New("9.1.0")),


Nit: Move version to a constant. And also add a TODO item to remove this in Teleport 11.

r0mant · 2022-03-18T18:46:30Z

lib/srv/db/proxyserver.go

@@ -660,7 +668,7 @@ func getConfigForClient(conf *tls.Config, ap auth.ReadDatabaseAccessPoint, log l
 				log.Debugf("Ignoring unsupported cluster name %q.", info.ServerName)
 			}
 		}
-		pool, err := auth.ClientCertPool(ap, clusterName)
+		pool, err := auth.ClientCertPool(ap, clusterName, types.HostCA, types.DatabaseCA, types.UserCA)


Do we need to load all CAs here? Client certificates are signed by user CA, no?

When I refactored the function I wanted to keep the same sets of certs as before, but I agree. We need only UserCA. Changed

Fix "remove in" version

r0mant · 2022-03-30T20:52:55Z

@jakule @smallinsky Can we rebase and merge this?

* Add DatabaseCA to trusted cluster rotation * Add DatabaseCA test * Refactor NewCertAuthorityWatcher - remove code duplication. * Refactor TestDatabaseRotateTrustedCluster. * Add comments to a DatabaseCA test. * Improve database rotation test. * Clean up test Add comments * Rename variable * Fix logger in DB integration test.

jakule force-pushed the jakule/database-ca branch 2 times, most recently from 903fbd8 to 465c64a Compare January 6, 2022 22:48

jakule marked this pull request as ready for review January 7, 2022 04:11

jakule requested a review from r0mant January 7, 2022 04:11

github-actions bot requested review from greedy52 and zmb3 January 7, 2022 04:11

github-actions bot added database-access Database access related issues and PRs tctl tctl - Teleport admin tool labels Jan 7, 2022

r0mant requested a review from smallinsky January 12, 2022 01:17

r0mant reviewed Jan 12, 2022

View reviewed changes

smallinsky reviewed Jan 17, 2022

View reviewed changes

zmb3 approved these changes Jan 24, 2022

View reviewed changes

integration/helpers.go Outdated Show resolved Hide resolved

jakule force-pushed the jakule/database-ca branch from 9ac75d0 to 841480a Compare January 26, 2022 02:33

r0mant reviewed Jan 26, 2022

View reviewed changes

jakule force-pushed the jakule/database-ca branch 2 times, most recently from d3c0c4f to 9bace68 Compare February 1, 2022 18:34

jakule force-pushed the jakule/database-ca branch 2 times, most recently from 76a45f1 to 3bc32e8 Compare February 17, 2022 02:48

jakule requested review from r0mant and smallinsky February 17, 2022 22:07

jakule added 9 commits February 17, 2022 20:26

Add Database CA - initial draft.

e79dc5c

Add database CA in a few more places.

a871ed2

Initial Database CA migration.

a18e6c6

Add Database CA tests.

3f7bfbc

Fix tests

30b659c

Reduce code duplication.

91e536d

Remove SSH key form Database CA

Fix system tests.

e1c465f

Add missing database CA to system tests.

2af0b59

Comments improvements.

d42da7e

Revert e changes

368d33f

jakule and others added 2 commits March 18, 2022 12:16

Update code comments

cf6cde9

Co-authored-by: Marek Smoliński <[email protected]>

Add DELTELE IN comments

2f36e63

r0mant reviewed Mar 18, 2022

View reviewed changes

jakule added 3 commits March 21, 2022 21:32

Merge remote-tracking branch 'origin/master' into jakule/database-ca

c926d2d

Address code review comments.

e77e8ee

Improve error message.

f1451a7

r0mant approved these changes Mar 22, 2022

View reviewed changes

jakule added 6 commits March 22, 2022 21:15

Add DatabaseCA to TestValidateTrustedCluster

80c29f4

Update DatabaseCA release version.

51f280f

Add DatabaseCA back to client certificate pool.

a79b81c

Fix DatabaseCA version check for prereleases.

b545cfd

Remove UserCA from a DB proxy client pools

0f7aa6a

Fix "remove in" version

Merge remote-tracking branch 'origin/master' into jakule/database-ca

38c116a

Merge remote-tracking branch 'origin/master' into jakule/database-ca

4a73e20

jakule mentioned this pull request Mar 31, 2022

Database CA - trusted cluster changes #11615

Merged

jakule added 4 commits April 4, 2022 11:59

Merge remote-tracking branch 'origin/master' into jakule/database-ca

650986c

Skip flaky TestDatabaseRotateTrustedCluster test

80b2207

Merge branch 'master' into jakule/database-ca

f35833d

jakule enabled auto-merge (squash) April 5, 2022 16:09

jakule added 2 commits April 5, 2022 13:57

Merge branch 'master' into jakule/database-ca

26d6c27

Merge branch 'master' into jakule/database-ca

47e132c

jakule merged commit 1aa38f4 into master Apr 5, 2022

jakule deleted the jakule/database-ca branch April 5, 2022 19:44

jakule mentioned this pull request Apr 25, 2022

Add new config templates to tbot for databases and identity files #11596

Merged

jakule mentioned this pull request Jun 6, 2022

Fix tctl Database CA compatibility with v9 cluster #13177

Merged

GavinFrazar mentioned this pull request Oct 2, 2023

Remove deprecated CertAuthTypeAll #23608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Database CA #9593

Create Database CA #9593

jakule commented Dec 29, 2021 •

edited

Loading

r0mant Jan 12, 2022

jakule Jan 26, 2022

r0mant Jan 12, 2022

jakule Feb 17, 2022

r0mant Jan 12, 2022

jakule Feb 17, 2022

r0mant Jan 12, 2022

jakule Feb 17, 2022

r0mant Jan 12, 2022

fspmarshall Jan 26, 2022

r0mant Jan 26, 2022

jakule Jan 26, 2022

smallinsky left a comment

smallinsky Jan 17, 2022

jakule Jan 26, 2022

zmb3 left a comment

jakule commented Jan 26, 2022

r0mant Jan 26, 2022

smallinsky Jan 31, 2022

r0mant Jan 31, 2022

smallinsky Feb 1, 2022

jakule commented Mar 18, 2022

r0mant Mar 18, 2022

jakule Mar 22, 2022

r0mant Mar 18, 2022

jakule Mar 22, 2022

r0mant Mar 18, 2022

jakule Mar 22, 2022

r0mant commented Mar 30, 2022

Create Database CA #9593

Create Database CA #9593

Conversation

jakule commented Dec 29, 2021 • 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

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

smallinsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmb3 left a comment

Choose a reason for hiding this comment

jakule commented Jan 26, 2022

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

jakule commented Mar 18, 2022

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

r0mant commented Mar 30, 2022

jakule commented Dec 29, 2021 •

edited

Loading