[v8] backport #12057 (panic in CertAuthority.Clone) (#12107)

* improve ca cmp (#10351) * Fix panic in `CertAuthority.Clone` because of non-UTC times. (#12057) Co-authored-by: Forrest <[email protected]>
gravitational · Apr 20, 2022 · 6538551 · 6538551
1 parent 67673ab
commit 6538551
Show file tree

Hide file tree

Showing 3 changed files with 79 additions and 14 deletions.
diff --git a/lib/services/authority.go b/lib/services/authority.go
@@ -352,6 +352,18 @@ func UnmarshalCertAuthority(bytes []byte, opts ...MarshalOption) (types.CertAuth
 		if cfg.ID != 0 {
 			ca.SetResourceID(cfg.ID)
 		}
+		// Correct problems with existing CAs that contain non-UTC times, which
+		// causes panics when doing a gogoproto Clone; should only ever be
+		// possible with LastRotated, but we enforce it on all the times anyway.
+		// See https://github.com/gogo/protobuf/issues/519 .
+		if ca.Spec.Rotation != nil {
+			apiutils.UTC(&ca.Spec.Rotation.Started)
+			apiutils.UTC(&ca.Spec.Rotation.LastRotated)
+			apiutils.UTC(&ca.Spec.Rotation.Schedule.UpdateClients)
+			apiutils.UTC(&ca.Spec.Rotation.Schedule.UpdateServers)
+			apiutils.UTC(&ca.Spec.Rotation.Schedule.Standby)
+		}
+
 		return &ca, nil
 	}
 

diff --git a/lib/services/authority_test.go b/lib/services/authority_test.go
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package services
+package services_test
 
 import (
 	"crypto/x509/pkix"
@@ -24,7 +24,10 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/gravitational/teleport/api/types"
+	"github.com/gravitational/teleport/lib/auth/testauthority"
+	. "github.com/gravitational/teleport/lib/services"
 	"github.com/gravitational/teleport/lib/tlsca"
+	"github.com/gravitational/teleport/lib/utils"
 )
 
 func TestCertPoolFromCertAuthorities(t *testing.T) {
@@ -157,3 +160,47 @@ func TestCertAuthorityEquivalence(t *testing.T) {
 	ca1modID.SetResourceID(ca1.GetResourceID() + 1)
 	require.True(t, CertAuthoritiesEquivalent(ca1, ca1modID))
 }
+
+func TestCertAuthorityUTCUnmarshal(t *testing.T) {
+	t.Parallel()
+	ta := testauthority.New()
+	t.Cleanup(ta.Close)
+
+	_, pub, err := ta.GenerateKeyPair("")
+	require.NoError(t, err)
+	_, cert, err := tlsca.GenerateSelfSignedCA(pkix.Name{CommonName: "clustername"}, nil, time.Hour)
+	require.NoError(t, err)
+
+	caLocal, err := types.NewCertAuthority(types.CertAuthoritySpecV2{
+		Type:        types.HostCA,
+		ClusterName: "clustername",
+		ActiveKeys: types.CAKeySet{
+			SSH: []*types.SSHKeyPair{{PublicKey: pub}},
+			TLS: []*types.TLSKeyPair{{Cert: cert}},
+		},
+		Rotation: &types.Rotation{
+			LastRotated: time.Now().In(time.FixedZone("not UTC", 2*60*60)),
+		},
+	})
+	require.NoError(t, err)
+	// needed for CertAuthoritiesEquivalent, as this will get called by
+	// UnmarshalCertAuthority
+	require.NoError(t, SyncCertAuthorityKeys(caLocal))
+
+	_, offset := caLocal.GetRotation().LastRotated.Zone()
+	require.NotZero(t, offset)
+
+	item, err := utils.FastMarshal(caLocal)
+	require.NoError(t, err)
+	require.Contains(t, string(item), "+02:00\"")
+	caUTC, err := UnmarshalCertAuthority(item)
+	require.NoError(t, err)
+
+	_, offset = caUTC.GetRotation().LastRotated.Zone()
+	require.Zero(t, offset)
+
+	// see https://github.com/gogo/protobuf/issues/519
+	require.NotPanics(t, func() { caUTC.Clone() })
+
+	require.True(t, CertAuthoritiesEquivalent(caLocal, caUTC))
+}
diff --git a/lib/services/local/trust.go b/lib/services/local/trust.go
@@ -92,33 +92,39 @@ func (s *CA) UpsertCertAuthority(ca types.CertAuthority) error {
 }
 
 // CompareAndSwapCertAuthority updates the cert authority value
-// if the existing value matches existing parameter, returns nil if succeeds,
+// if the existing value matches expected parameter, returns nil if succeeds,
 // trace.CompareFailed otherwise.
-func (s *CA) CompareAndSwapCertAuthority(new, existing types.CertAuthority) error {
+func (s *CA) CompareAndSwapCertAuthority(new, expected types.CertAuthority) error {
 	if err := services.ValidateCertAuthority(new); err != nil {
 		return trace.Wrap(err)
 	}
-	newValue, err := services.MarshalCertAuthority(new)
+
+	key := backend.Key(authoritiesPrefix, string(new.GetType()), new.GetName())
+
+	actualItem, err := s.Get(context.TODO(), key)
 	if err != nil {
 		return trace.Wrap(err)
 	}
-	newItem := backend.Item{
-		Key:     backend.Key(authoritiesPrefix, string(new.GetType()), new.GetName()),
-		Value:   newValue,
-		Expires: new.Expiry(),
+	actual, err := services.UnmarshalCertAuthority(actualItem.Value)
+	if err != nil {
+		return trace.Wrap(err)
+	}
+
+	if !services.CertAuthoritiesEquivalent(actual, expected) {
+		return trace.CompareFailed("cluster %v settings have been updated, try again", new.GetName())
 	}
 
-	existingValue, err := services.MarshalCertAuthority(existing)
+	newValue, err := services.MarshalCertAuthority(new)
 	if err != nil {
 		return trace.Wrap(err)
 	}
-	existingItem := backend.Item{
-		Key:     backend.Key(authoritiesPrefix, string(existing.GetType()), existing.GetName()),
-		Value:   existingValue,
-		Expires: existing.Expiry(),
+	newItem := backend.Item{
+		Key:     key,
+		Value:   newValue,
+		Expires: new.Expiry(),
 	}
 
-	_, err = s.CompareAndSwap(context.TODO(), existingItem, newItem)
+	_, err = s.CompareAndSwap(context.TODO(), *actualItem, newItem)
 	if err != nil {
 		if trace.IsCompareFailed(err) {
 			return trace.CompareFailed("cluster %v settings have been updated, try again", new.GetName())