fix(pki): refuse to silently reissue or blank an existing kubernetes CA keypair

kops update cluster could propose creating a new kubernetes-ca keypair and
blank ConfigServer.CACertificates in generated nodeup configs even when a
valid CA already existed. If applied, this would break node bootstrap
(empty CA bundle) or, worse, rotate the cluster's CA out from under it.

- CreateKeyset() now re-checks the keystore immediately before minting a
  new "ca"-type certificate and refuses if one unexpectedly already
  exists there, protecting the dangerous real-apply outcome.
- Keyset gains an IsPlaceholder() helper, and nodeup config rendering
  (loadCertificates) now emits an unambiguous placeholder marker instead
  of a bare empty string for an unresolved keyset, so a preview can never
  look like an existing CA cert was blanked.

Neither change affects a genuinely new cluster's first update cluster
dry run, since nothing exists in the keystore to conflict with there.

See kubernetes/kops issue 18680 for background.

Author: ntmspavan

pkg/nodemodel/nodeupconfigbuilder.go

@@ -465,6 +465,19 @@ func loadCertificates(keysets map[string]*fi.Keyset, name string, config *nodeup
 	if keyset == nil {
 		return fmt.Errorf("key %q not found", name)
 	}
+	if keyset.IsPlaceholder() {
+		// The keypair has not actually been created yet (e.g. this is a preview
+		// of a not-yet-created cluster/keypair). Emit an unambiguous, obviously
+		// non-certificate placeholder rather than an empty string: an empty
+		// string here is indistinguishable from "this CA legitimately has no
+		// certificates" and, for an *existing* CA, could be silently mistaken
+		// for a real blanking of a live trust bundle.
+		config.CAs[name] = fi.PlaceholderKeypairID
+		if includeKeypairID {
+			config.KeypairIDs[name] = fi.PlaceholderKeypairID
+		}
+		return nil
+	}
 	certificates, err := keyset.ToCertificateBytes()
 	if err != nil {
 		return fmt.Errorf("failed to read %q certificates: %w", name, err)

pkg/nodemodel/nodeupconfigbuilder_test.go

@@ -17,10 +17,19 @@ limitations under the License.
 package nodemodel
 
 import (
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"math/big"
 	"reflect"
 	"testing"
+	"time"
 
 	"k8s.io/kops/pkg/apis/kops"
+	"k8s.io/kops/pkg/apis/nodeup"
+	"k8s.io/kops/pkg/pki"
+	"k8s.io/kops/upup/pkg/fi"
 )
 
 func TestSelectControlPlaneIPs(t *testing.T) {
@@ -104,3 +113,90 @@ func TestBuildConfigServerOptionsUsesDNSNameByDefault(t *testing.T) {
 		t.Fatalf("Servers = %v, want %v", got, want)
 	}
 }
+
+func selfSignedTestCert(t *testing.T, commonName string) *pki.Certificate {
+	t.Helper()
+
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatalf("generating test key: %v", err)
+	}
+	template := &x509.Certificate{
+		SerialNumber:          big.NewInt(1),
+		Subject:               pkix.Name{CommonName: commonName},
+		NotBefore:             time.Now(),
+		NotAfter:              time.Now().Add(10 * 365 * 24 * time.Hour),
+		KeyUsage:              x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
+		BasicConstraintsValid: true,
+		IsCA:                  true,
+	}
+	der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key)
+	if err != nil {
+		t.Fatalf("creating test certificate: %v", err)
+	}
+	cert, err := x509.ParseCertificate(der)
+	if err != nil {
+		t.Fatalf("parsing test certificate: %v", err)
+	}
+	return &pki.Certificate{Certificate: cert}
+}
+
+// TestLoadCertificatesNeverEmitsBlankCAForPlaceholderKeyset is a regression test for
+// https://github.com/kubernetes/kops/issues/18680: an unresolved (not-yet-created)
+// keypair must never silently turn into an empty-string CA bundle, which is
+// indistinguishable from a real, dangerous blanking of an existing trust bundle.
+func TestLoadCertificatesNeverEmitsBlankCAForPlaceholderKeyset(t *testing.T) {
+	config := &nodeup.Config{
+		CAs:        map[string]string{},
+		KeypairIDs: map[string]string{},
+	}
+	keysets := map[string]*fi.Keyset{
+		fi.CertificateIDCA: {
+			Primary: &fi.KeysetItem{Id: fi.PlaceholderKeypairID},
+		},
+	}
+
+	if err := loadCertificates(keysets, fi.CertificateIDCA, config, true); err != nil {
+		t.Fatalf("unexpected error for a placeholder (not-yet-created) keypair: %v", err)
+	}
+
+	if got := config.CAs[fi.CertificateIDCA]; got == "" {
+		t.Errorf("CAs[%q] = %q; must not be blank for a placeholder keyset", fi.CertificateIDCA, got)
+	}
+	if got, want := config.CAs[fi.CertificateIDCA], fi.PlaceholderKeypairID; got != want {
+		t.Errorf("CAs[%q] = %q, want unambiguous placeholder %q", fi.CertificateIDCA, got, want)
+	}
+	if got, want := config.KeypairIDs[fi.CertificateIDCA], fi.PlaceholderKeypairID; got != want {
+		t.Errorf("KeypairIDs[%q] = %q, want %q", fi.CertificateIDCA, got, want)
+	}
+}
+
+// TestLoadCertificatesPreservesRealCertificate is the control case: a real,
+// resolved keyset must still round-trip its actual certificate bytes unchanged.
+func TestLoadCertificatesPreservesRealCertificate(t *testing.T) {
+	cert := selfSignedTestCert(t, "kubernetes-ca")
+
+	config := &nodeup.Config{
+		CAs:        map[string]string{},
+		KeypairIDs: map[string]string{},
+	}
+	keysets := map[string]*fi.Keyset{
+		fi.CertificateIDCA: {
+			Primary: &fi.KeysetItem{Id: "123", Certificate: cert},
+			Items: map[string]*fi.KeysetItem{
+				"123": {Id: "123", Certificate: cert},
+			},
+		},
+	}
+
+	if err := loadCertificates(keysets, fi.CertificateIDCA, config, true); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if got := config.CAs[fi.CertificateIDCA]; got == "" || got == fi.PlaceholderKeypairID {
+		t.Errorf("CAs[%q] = %q, want the real certificate PEM", fi.CertificateIDCA, got)
+	}
+	if got, want := config.KeypairIDs[fi.CertificateIDCA], "123"; got != want {
+		t.Errorf("KeypairIDs[%q] = %q, want %q", fi.CertificateIDCA, got, want)
+	}
+}

upup/pkg/fi/ca.go

@@ -47,6 +47,12 @@ const (
 	keysetFormatLatest = "v1alpha2"
 )
 
+// PlaceholderKeypairID is the sentinel Primary.Id used by a Keypair task's
+// Keyset() when the real certificate/key data has not yet been resolved by
+// Find() or Render() (e.g. a keypair that is only about to be created). It
+// must never be mistaken for a real keypair identifier.
+const PlaceholderKeypairID = "<< TO BE GENERATED >>"
+
 // Keyset is a parsed api.Keyset.
 type Keyset struct {
 	// LegacyFormat instructs a keypair task to convert a Legacy Keyset to the new Keyset API format.
@@ -58,6 +64,12 @@ type Keyset struct {
 	Primary *KeysetItem
 }
 
+// IsPlaceholder returns true if this Keyset is an unresolved placeholder
+// (see PlaceholderKeypairID) rather than a real result from the keystore.
+func (k *Keyset) IsPlaceholder() bool {
+	return k != nil && k.Primary != nil && k.Primary.Id == PlaceholderKeypairID && len(k.Items) == 0
+}
+
 // KeysetItem is a certificate/key pair in a Keyset.
 type KeysetItem struct {
 	// Id is the identifier of this keypair.

upup/pkg/fi/ca_test.go

@@ -375,6 +375,60 @@ func TestAddItem(t *testing.T) {
 	}
 }
 
+func TestKeysetIsPlaceholder(t *testing.T) {
+	cert, _ := pki.ParsePEMCertificate([]byte(certData))
+	privateKey, _ := pki.ParsePEMPrivateKey([]byte(privatekeyData))
+
+	grid := []struct {
+		name   string
+		keyset *fi.Keyset
+		want   bool
+	}{
+		{
+			name:   "nil keyset",
+			keyset: nil,
+			want:   false,
+		},
+		{
+			name:   "zero-value keyset",
+			keyset: &fi.Keyset{},
+			want:   false,
+		},
+		{
+			name: "unresolved placeholder",
+			keyset: &fi.Keyset{
+				Primary: &fi.KeysetItem{Id: fi.PlaceholderKeypairID},
+			},
+			want: true,
+		},
+		{
+			name: "real keyset with matching item count coincidence",
+			keyset: &fi.Keyset{
+				Primary: &fi.KeysetItem{
+					Id:          "6952323604391556590983096308",
+					Certificate: cert,
+					PrivateKey:  privateKey,
+				},
+				Items: map[string]*fi.KeysetItem{
+					"6952323604391556590983096308": {
+						Id:          "6952323604391556590983096308",
+						Certificate: cert,
+						PrivateKey:  privateKey,
+					},
+				},
+			},
+			want: false,
+		},
+	}
+
+	for _, g := range grid {
+		t.Run(g.name, func(t *testing.T) {
+			got := g.keyset.IsPlaceholder()
+			assert.Equal(t, g.want, got)
+		})
+	}
+}
+
 func assertSerialNotInFuture(t *testing.T, id string) {
 	version, ok := big.NewInt(0).SetString(id, 10)
 	require.True(t, ok, "parses as integer")

upup/pkg/fi/fitasks/keypair.go

@@ -273,6 +273,15 @@ func CreateKeyset(ctx context.Context, keystore fi.Keystore, name string, req pk
 		keyset = &fi.Keyset{
 			Items: map[string]*fi.KeysetItem{},
 		}
+	} else if req.Type == "ca" && keyset.Primary != nil && keyset.Primary.Certificate != nil {
+		// The caller believed this CA needed to be (re)created, but a direct,
+		// authoritative lookup right before mutating the keystore shows an
+		// existing, valid CA certificate. Silently reissuing it here would
+		// rotate the cluster's trust anchor out from under it. This mismatch
+		// indicates a task-planning inconsistency (e.g. the keypair task's
+		// own Find() did not see this same data) rather than a genuinely
+		// missing CA, so refuse instead of proceeding.
+		return nil, fmt.Errorf("refusing to create keypair %q: an existing CA certificate was found in the keystore; this indicates a task-planning inconsistency rather than a genuinely missing CA — aborting to avoid rotating a cluster's trust anchor", name)
 	}
 
 	if req.Serial == nil {
@@ -348,12 +357,12 @@ func parsePkixName(s string) (*pkix.Name, error) {
 func (e *Keypair) ensureResources() {
 	if e.certificates == nil {
 		e.certificates = &fi.CloudupTaskDependentResource{
-			Resource: fi.NewStringResource("<< TO BE GENERATED >>\n"),
+			Resource: fi.NewStringResource(fi.PlaceholderKeypairID + "\n"),
 			Task:     e,
 		}
 		e.keyset = &fi.Keyset{
 			Primary: &fi.KeysetItem{
-				Id: "<< TO BE GENERATED >>",
+				Id: fi.PlaceholderKeypairID,
 			},
 		}
 	}

upup/pkg/fi/fitasks/keypair_test.go

@@ -17,10 +17,19 @@ limitations under the License.
 package fitasks
 
 import (
+	"context"
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"math/big"
 	"strings"
 	"testing"
+	"time"
 
+	"k8s.io/kops/pkg/pki"
 	"k8s.io/kops/upup/pkg/fi"
+	"k8s.io/kops/util/pkg/vfs"
 )
 
 func TestKeypairDeps(t *testing.T) {
@@ -46,3 +55,111 @@ func TestKeypairDeps(t *testing.T) {
 		t.Errorf("unexpected dependencies for cert: %v", deps["cert"])
 	}
 }
+
+// fakeKeystore is a minimal in-memory fi.Keystore for exercising CreateKeyset
+// without going through a real VFS-backed store.
+type fakeKeystore struct {
+	keysets map[string]*fi.Keyset
+	stored  bool
+}
+
+func (f *fakeKeystore) FindKeyset(ctx context.Context, name string) (*fi.Keyset, error) {
+	return f.keysets[name], nil
+}
+
+func (f *fakeKeystore) StoreKeyset(ctx context.Context, name string, keyset *fi.Keyset) error {
+	f.stored = true
+	if f.keysets == nil {
+		f.keysets = map[string]*fi.Keyset{}
+	}
+	f.keysets[name] = keyset
+	return nil
+}
+
+func (f *fakeKeystore) MirrorTo(ctx context.Context, basedir vfs.Path) error {
+	return nil
+}
+
+var _ fi.Keystore = &fakeKeystore{}
+
+// selfSignedTestCA builds a minimal self-signed CA certificate/key pair for tests.
+func selfSignedTestCA(t *testing.T, commonName string) (*pki.Certificate, *pki.PrivateKey) {
+	t.Helper()
+
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatalf("generating test key: %v", err)
+	}
+
+	template := &x509.Certificate{
+		SerialNumber:          big.NewInt(1),
+		Subject:               pkix.Name{CommonName: commonName},
+		NotBefore:             time.Now(),
+		NotAfter:              time.Now().Add(10 * 365 * 24 * time.Hour),
+		KeyUsage:              x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
+		BasicConstraintsValid: true,
+		IsCA:                  true,
+	}
+
+	der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key)
+	if err != nil {
+		t.Fatalf("creating test certificate: %v", err)
+	}
+	cert, err := x509.ParseCertificate(der)
+	if err != nil {
+		t.Fatalf("parsing test certificate: %v", err)
+	}
+
+	return &pki.Certificate{Certificate: cert}, &pki.PrivateKey{Key: key}
+}
+
+func TestCreateKeysetRefusesToReissueExistingCA(t *testing.T) {
+	ctx := context.Background()
+
+	existingCert, existingKey := selfSignedTestCA(t, "kubernetes-ca")
+
+	store := &fakeKeystore{
+		keysets: map[string]*fi.Keyset{
+			"kubernetes-ca": {
+				Primary: &fi.KeysetItem{Id: "1", Certificate: existingCert, PrivateKey: existingKey},
+				Items: map[string]*fi.KeysetItem{
+					"1": {Id: "1", Certificate: existingCert, PrivateKey: existingKey},
+				},
+			},
+		},
+	}
+
+	_, err := CreateKeyset(ctx, store, "kubernetes-ca", pki.IssueCertRequest{
+		Type:    "ca",
+		Subject: pkix.Name{CommonName: "kubernetes-ca"},
+	})
+
+	if err == nil {
+		t.Fatal("expected CreateKeyset to refuse reissuing an existing CA, got nil error")
+	}
+	if !strings.Contains(err.Error(), "refusing to create keypair") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+	if store.stored {
+		t.Error("CreateKeyset must not store a new keyset when refusing to reissue an existing CA")
+	}
+}
+
+func TestCreateKeysetCreatesNewCAWhenNoneExists(t *testing.T) {
+	ctx := context.Background()
+	store := &fakeKeystore{}
+
+	keyset, err := CreateKeyset(ctx, store, "kubernetes-ca", pki.IssueCertRequest{
+		Type:    "ca",
+		Subject: pkix.Name{CommonName: "kubernetes-ca"},
+	})
+	if err != nil {
+		t.Fatalf("unexpected error creating a brand-new CA: %v", err)
+	}
+	if keyset == nil || keyset.Primary == nil || keyset.Primary.Certificate == nil {
+		t.Fatal("expected a populated keyset for a brand-new CA")
+	}
+	if !store.stored {
+		t.Error("expected CreateKeyset to store the newly created CA")
+	}
+}