From d78a76010edaa8693e44a54ad28e6184a6fbbe2a Mon Sep 17 00:00:00 2001 From: Ian Littman Date: Sat, 5 Jul 2025 19:44:31 -0500 Subject: [PATCH] Properly filter host certificates by host on update when multiple hosts share the same certificate (#30578) Fixes #30574. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [x] Added/updated automated tests - [x] Manual QA for all new/changed functionality ## Summary by CodeRabbit * **Bug Fixes** * Resolved issues with recording host certificate sources when multiple hosts share the same certificate but have different usernames, improving accuracy and performance. * Addressed related performance and database load problems for these scenarios. * **Tests** * Added new tests to ensure certificate source records remain properly isolated per host, even when certificates are shared across hosts. --- changes/30574-confused-certs | 1 + server/datastore/mysql/host_certificates.go | 20 ++- .../datastore/mysql/host_certificates_test.go | 151 ++++++++++++++++++ 3 files changed, 168 insertions(+), 4 deletions(-) create mode 100644 changes/30574-confused-certs diff --git a/changes/30574-confused-certs b/changes/30574-confused-certs new file mode 100644 index 0000000000..74a01e5b35 --- /dev/null +++ b/changes/30574-confused-certs @@ -0,0 +1 @@ +* Fixed host certificate source recording (including associated performance/database load issues) when multiple hosts share the same certificate on user keychains with differing usernames diff --git a/server/datastore/mysql/host_certificates.go b/server/datastore/mysql/host_certificates.go index dd474afc24..65a0c1dec0 100644 --- a/server/datastore/mysql/host_certificates.go +++ b/server/datastore/mysql/host_certificates.go @@ -77,6 +77,17 @@ func (ds *Datastore) UpdateHostCertificates(ctx context.Context, hostID uint, ho for sha1, incoming := range incomingBySHA1 { incomingSources := incomingSourcesBySHA1[sha1] existingSources := existingSourcesBySHA1[sha1] + + // sort by keychain (user/system) and username to ensure consistent ordering + sliceSortFunc := func(a, b certSourceToSet) int { + if a.Source != b.Source { + return strings.Compare(string(a.Source), string(b.Source)) + } + return strings.Compare(a.Username, b.Username) + } + slices.SortFunc(incomingSources, sliceSortFunc) + slices.SortFunc(existingSources, sliceSortFunc) + if !slices.Equal(incomingSources, existingSources) { toSetSourcesBySHA1[sha1] = incomingSources } @@ -141,7 +152,7 @@ func (ds *Datastore) UpdateHostCertificates(ctx context.Context, hostID uint, ho if len(toSetSourcesBySHA1) > 0 { // must reload the DB IDs to insert the host_certificates_sources rows - certIDsBySHA1, err := loadHostCertIDsForSHA1DB(ctx, tx, slices.Collect(maps.Keys(toSetSourcesBySHA1))) + certIDsBySHA1, err := loadHostCertIDsForSHA1DB(ctx, tx, hostID, slices.Collect(maps.Keys(toSetSourcesBySHA1))) if err != nil { return ctxerr.Wrap(ctx, err, "load host certs ids") } @@ -172,7 +183,7 @@ func (ds *Datastore) UpdateHostCertificates(ctx context.Context, hostID uint, ho }) } -func loadHostCertIDsForSHA1DB(ctx context.Context, tx sqlx.QueryerContext, sha1s []string) (map[string]uint, error) { +func loadHostCertIDsForSHA1DB(ctx context.Context, tx sqlx.QueryerContext, hostID uint, sha1s []string) (map[string]uint, error) { if len(sha1s) == 0 { return nil, nil } @@ -190,10 +201,11 @@ func loadHostCertIDsForSHA1DB(ctx context.Context, tx sqlx.QueryerContext, sha1s FROM host_certificates hc WHERE - hc.sha1_sum IN (?)` + hc.sha1_sum IN (?) AND hc.host_id = ?` var certs []*fleet.HostCertificateRecord - stmt, args, err := sqlx.In(stmt, binarySHA1s) + stmt, args, err := sqlx.In(stmt, binarySHA1s, hostID) + if err != nil { return nil, ctxerr.Wrap(ctx, err, "building load host cert ids query") } diff --git a/server/datastore/mysql/host_certificates_test.go b/server/datastore/mysql/host_certificates_test.go index 07f7b5d825..8eefc8f186 100644 --- a/server/datastore/mysql/host_certificates_test.go +++ b/server/datastore/mysql/host_certificates_test.go @@ -26,6 +26,7 @@ func TestHostCertificates(t *testing.T) { }{ {"UpdateAndList", testUpdateAndListHostCertificates}, {"Update with host_mdm_managed_certificates to update", testUpdatingHostMDMManagedCertificates}, + {"Update certificate sources isolation", testUpdateHostCertificatesSourcesIsolation}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -318,3 +319,153 @@ func generateTestHostCertificateRecord(t *testing.T, hostID uint, template *x509 return fleet.NewHostCertificateRecord(hostID, parsed) } + +func testUpdateHostCertificatesSourcesIsolation(t *testing.T, ds *Datastore) { + // regression test for #30574 + ctx := context.Background() + + host1, err := ds.NewHost(ctx, &fleet.Host{ + DetailUpdatedAt: time.Now(), + LabelUpdatedAt: time.Now(), + PolicyUpdatedAt: time.Now(), + SeenTime: time.Now(), + OsqueryHostID: ptr.String("host1-osquery-id"), + NodeKey: ptr.String("host1-node-key"), + UUID: "host1-uuid", + Hostname: "host1", + }) + require.NoError(t, err) + + host2, err := ds.NewHost(ctx, &fleet.Host{ + DetailUpdatedAt: time.Now(), + LabelUpdatedAt: time.Now(), + PolicyUpdatedAt: time.Now(), + SeenTime: time.Now(), + OsqueryHostID: ptr.String("host2-osquery-id"), + NodeKey: ptr.String("host2-node-key"), + UUID: "host2-uuid", + Hostname: "host2", + }) + require.NoError(t, err) + + // Create identical certificates for both hosts (same SHA1 sum) + // This simulates the real-world scenario where multiple hosts have the same certificate + // installed (e.g., a company root CA certificate) + sharedCert := x509.Certificate{ + Subject: pkix.Name{ + Country: []string{"US"}, + CommonName: "shared.example.com", + Organization: []string{"Shared Org"}, + OrganizationalUnit: []string{"Engineering"}, + }, + Issuer: pkix.Name{ + Country: []string{"US"}, + CommonName: "issuer.example.com", + Organization: []string{"Issuer"}, + }, + SerialNumber: big.NewInt(12345), + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + + SignatureAlgorithm: x509.SHA256WithRSA, + NotBefore: time.Now().Add(-time.Hour).Truncate(time.Second).UTC(), + NotAfter: time.Now().Add(24 * time.Hour).Truncate(time.Second).UTC(), + BasicConstraintsValid: true, + } + + // Generate certificate records for both hosts using the same certificate data + // We need to create the certificate bytes once and reuse them to ensure same SHA1 + certBytes, _, err := GenerateTestCertBytes(&sharedCert) + require.NoError(t, err) + + block, _ := pem.Decode(certBytes) + parsed, err := x509.ParseCertificate(block.Bytes) + require.NoError(t, err) + + host1Cert := fleet.NewHostCertificateRecord(host1.ID, parsed) + host1Cert.Source = fleet.UserHostCertificate + host1Cert.Username = "jdoe" + + host2Cert := fleet.NewHostCertificateRecord(host2.ID, parsed) + host2Cert.Source = fleet.UserHostCertificate + host2Cert.Username = "jsmith" + + // Add the same certificate to both hosts + require.NoError(t, ds.UpdateHostCertificates(ctx, host1.ID, host1.UUID, []*fleet.HostCertificateRecord{host1Cert})) + require.NoError(t, ds.UpdateHostCertificates(ctx, host2.ID, host2.UUID, []*fleet.HostCertificateRecord{host2Cert})) + + // Verify both hosts have the correct certs, with the correct sources + host1Certs, _, err := ds.ListHostCertificates(ctx, host1.ID, fleet.ListOptions{}) + require.NoError(t, err) + require.Len(t, host1Certs, 1) + require.Equal(t, fleet.UserHostCertificate, host1Certs[0].Source) + require.Equal(t, "jdoe", host1Certs[0].Username) + + host2Certs, _, err := ds.ListHostCertificates(ctx, host2.ID, fleet.ListOptions{}) + require.NoError(t, err) + require.Len(t, host2Certs, 1) + require.Equal(t, fleet.UserHostCertificate, host2Certs[0].Source) + require.Equal(t, "jsmith", host2Certs[0].Username) + + // Trigger a change in host 2's cert sources to force delete/recreate of source records. Prior to the fix this + // wouldn't modify the correct certs because the DB query would return host 1's cert for the given hash. + host2CertUpdated := fleet.NewHostCertificateRecord(host2.ID, parsed) + host2CertUpdated.Source = fleet.UserHostCertificate + host2CertUpdated.Username = "janesmith" + + require.NoError(t, ds.UpdateHostCertificates(ctx, host2.ID, host2.UUID, []*fleet.HostCertificateRecord{host2CertUpdated})) + + // Verify host1's certificate source was *not* updated + host1CertsAfter, _, err := ds.ListHostCertificates(ctx, host1.ID, fleet.ListOptions{}) + require.NoError(t, err) + require.Len(t, host1CertsAfter, 1) + require.Equal(t, "jdoe", host1CertsAfter[0].Username) + + // Verify host2's certificate source *was* updated + host2CertsAfter, _, err := ds.ListHostCertificates(ctx, host2.ID, fleet.ListOptions{}) + require.NoError(t, err) + require.Len(t, host2CertsAfter, 1) + require.Equal(t, "janesmith", host2CertsAfter[0].Username) + + // Verify no-op case + err = ds.UpdateHostCertificates(ctx, host2.ID, host2.UUID, []*fleet.HostCertificateRecord{host2CertUpdated}) + require.NoError(t, err) + + // Verify host2's certificate source was updated + host2CertsNoop, _, err := ds.ListHostCertificates(ctx, host2.ID, fleet.ListOptions{}) + require.NoError(t, err) + require.Len(t, host2CertsNoop, 1) + require.Equal(t, fleet.UserHostCertificate, host2CertsNoop[0].Source) + require.Equal(t, "janesmith", host2CertsNoop[0].Username) + + // Confirm that adding the cert to the system store gets picked up properly + systemCertOnHost2 := fleet.NewHostCertificateRecord(host2.ID, parsed) + systemCertOnHost2.Source = fleet.SystemHostCertificate + + require.NoError(t, ds.UpdateHostCertificates(ctx, host2.ID, host2.UUID, []*fleet.HostCertificateRecord{host2CertUpdated, systemCertOnHost2})) + + // Verify host2 now has the certificate with both sources + host2CertsMultiSource, _, err := ds.ListHostCertificates(ctx, host2.ID, fleet.ListOptions{}) + require.NoError(t, err) + require.Len(t, host2CertsMultiSource, 2) + var hasUserCert, hasSystemCert bool + for _, cert := range host2CertsMultiSource { + if cert.Source == fleet.UserHostCertificate { + require.Equal(t, "janesmith", cert.Username) + hasUserCert = true + } else { + require.Empty(t, cert.Username) + require.Equal(t, fleet.SystemHostCertificate, cert.Source) + hasSystemCert = true + } + } + require.True(t, hasUserCert) + require.True(t, hasSystemCert) + + // Verify host1 still has only its original certificate source + host1CertsMultiSource, _, err := ds.ListHostCertificates(ctx, host1.ID, fleet.ListOptions{}) + require.NoError(t, err) + require.Len(t, host1CertsMultiSource, 1) + require.Equal(t, fleet.UserHostCertificate, host1CertsMultiSource[0].Source) + require.Equal(t, "jdoe", host1CertsMultiSource[0].Username) +}