Rework PKI test TestBackend_Root_Idempotency

 - Validate that generate/root calls are no longer idempotent, but the bundle importing
   does not generate new keys/issuers
 - As before make sure that the delete root api resets everything
 - Address a bug within the storage that we bombed when we had multiple different
   key types within storage.

builtin/logical/pki/backend_test.go

@@ -2276,90 +2276,107 @@ func TestBackend_Root_Idempotency(t *testing.T) {
 	cluster.Start()
 	defer cluster.Cleanup()
 	client := cluster.Cores[0].Client
-	var err error
-	err = client.Sys().Mount("pki", &api.MountInput{
-		Type: "pki",
-		Config: api.MountConfigInput{
-			DefaultLeaseTTL: "16h",
-			MaxLeaseTTL:     "32h",
-		},
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
 
+	mountPKIEndpoint(t, client, "pki")
+
+	// This is a change within 1.11, we are no longer idempotent across generate/internal calls.
 	resp, err := client.Logical().Write("pki/root/generate/internal", map[string]interface{}{
 		"common_name": "myvault.com",
 	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	if resp == nil {
-		t.Fatal("expected ca info")
-	}
+	require.NoError(t, err)
+	require.NotNil(t, resp, "expected ca info")
+	keyId1 := resp.Data["key_id"]
+	issuerId1 := resp.Data["issuer_id"]
+
 	resp, err = client.Logical().Read("pki/cert/ca_chain")
-	if err != nil {
-		t.Fatalf("error reading ca_chain: %v", err)
-	}
+	require.NoError(t, err, "error reading ca_chain: %v", err)
 
 	r1Data := resp.Data
 
-	// Try again, make sure it's a 204 and same CA
+	// Calling generate/internal should generate a new CA as well.
 	resp, err = client.Logical().Write("pki/root/generate/internal", map[string]interface{}{
 		"common_name": "myvault.com",
 	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	if resp == nil {
-		t.Fatal("expected a warning")
-	}
-	if resp.Data != nil || len(resp.Warnings) == 0 {
-		t.Fatalf("bad response: %#v", *resp)
-	}
+	require.NoError(t, err)
+	require.NotNil(t, resp, "expected ca info")
+	keyId2 := resp.Data["key_id"]
+	issuerId2 := resp.Data["issuer_id"]
+
+	// Make sure that we actually generated different issuer and key values
+	require.NotEqual(t, keyId1, keyId2)
+	require.NotEqual(t, issuerId1, issuerId2)
+
+	// Now because the issued CA's have no links, the call to ca_chain should return the same data (ca chain from default)
 	resp, err = client.Logical().Read("pki/cert/ca_chain")
-	if err != nil {
-		t.Fatalf("error reading ca_chain: %v", err)
-	}
+	require.NoError(t, err, "error reading ca_chain: %v", err)
+
 	r2Data := resp.Data
 	if !reflect.DeepEqual(r1Data, r2Data) {
 		t.Fatal("got different ca certs")
 	}
 
+	// Now let's validate that the import bundle is idempotent.
+	pemBundleRootCA := string(cluster.CACertPEM) + string(cluster.CAKeyPEM)
+	resp, err = client.Logical().Write("pki/config/ca", map[string]interface{}{
+		"pem_bundle": pemBundleRootCA,
+	})
+	require.NoError(t, err)
+	require.NotNil(t, resp, "expected ca info")
+	firstImportedKeys := resp.Data["imported_keys"].([]interface{})
+	firstImportedIssuers := resp.Data["imported_issuers"].([]interface{})
+
+	require.NotContains(t, firstImportedKeys, keyId1)
+	require.NotContains(t, firstImportedKeys, keyId2)
+	require.NotContains(t, firstImportedIssuers, issuerId1)
+	require.NotContains(t, firstImportedIssuers, issuerId2)
+
+	// Performing this again should result in no key/issuer ids being imported/generated.
+	resp, err = client.Logical().Write("pki/config/ca", map[string]interface{}{
+		"pem_bundle": pemBundleRootCA,
+	})
+	require.NoError(t, err)
+	require.NotNil(t, resp, "expected ca info")
+	secondImportedKeys := resp.Data["imported_keys"]
+	secondImportedIssuers := resp.Data["imported_issuers"]
+
+	require.Nil(t, secondImportedKeys)
+	require.Nil(t, secondImportedIssuers)
+
 	resp, err = client.Logical().Delete("pki/root")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if resp != nil {
-		t.Fatal("expected nil response")
-	}
-	// Make sure it behaves the same
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.Equal(t, 1, len(resp.Warnings))
+
+	// Make sure we can delete twice...
 	resp, err = client.Logical().Delete("pki/root")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if resp != nil {
-		t.Fatal("expected nil response")
-	}
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.Equal(t, 1, len(resp.Warnings))
 
 	_, err = client.Logical().Read("pki/cert/ca_chain")
-	if err == nil {
-		t.Fatal("expected error")
-	}
+	require.Error(t, err, "expected an error fetching deleted ca_chain")
 
-	resp, err = client.Logical().Write("pki/root/generate/internal", map[string]interface{}{
-		"common_name": "myvault.com",
+	// We should be able to import the same ca bundle as before and get a different key/issuer ids
+	resp, err = client.Logical().Write("pki/config/ca", map[string]interface{}{
+		"pem_bundle": pemBundleRootCA,
 	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	if resp == nil {
-		t.Fatal("expected ca info")
-	}
+	require.NoError(t, err)
+	require.NotNil(t, resp, "expected ca info")
+	postDeleteImportedKeys := resp.Data["imported_keys"]
+	postDeleteImportedIssuers := resp.Data["imported_issuers"]
 
-	_, err = client.Logical().Read("pki/cert/ca_chain")
-	if err != nil {
-		t.Fatal(err)
+	// Make sure that we actually generated different issuer and key values, then the previous import
+	require.NotNil(t, postDeleteImportedKeys)
+	require.NotNil(t, postDeleteImportedIssuers)
+	require.NotEqual(t, postDeleteImportedKeys, firstImportedKeys)
+	require.NotEqual(t, postDeleteImportedIssuers, firstImportedIssuers)
+
+	resp, err = client.Logical().Read("pki/cert/ca_chain")
+	require.NoError(t, err)
+
+	caChainPostDelete := resp.Data
+	if reflect.DeepEqual(r1Data, caChainPostDelete) {
+		t.Fatal("ca certs from ca_chain were the same post delete, should have changed.")
 	}
 }
 

builtin/logical/pki/storage.go

@@ -217,7 +217,7 @@ func importKey(ctx context.Context, s logical.Storage, keyValue string, keyName
 			return nil, false, err
 		}
 
-		equal, err := certutil.ComparePublicKeys(cert.PublicKey, keyPublic)
+		equal, err := certutil.ComparePublicKeysAndType(cert.PublicKey, keyPublic)
 		if err != nil {
 			return nil, false, err
 		}
@@ -437,7 +437,7 @@ func importIssuer(ctx context.Context, s logical.Storage, certValue string, issu
 			return nil, false, err
 		}
 
-		equal, err := certutil.ComparePublicKeys(issuerCert.PublicKey, signer.Public())
+		equal, err := certutil.ComparePublicKeysAndType(issuerCert.PublicKey, signer.Public())
 		if err != nil {
 			return nil, false, err
 		}

sdk/helper/certutil/certutil_test.go

@@ -2,6 +2,7 @@ package certutil
 
 import (
 	"bytes"
+	"crypto"
 	"crypto/ecdsa"
 	"crypto/ed25519"
 	"crypto/elliptic"
@@ -853,6 +854,72 @@ func setCerts() {
 	issuingCaChainPem = []string{intCertPEM, caCertPEM}
 }
 
+func TestComparePublicKeysAndType(t *testing.T) {
+	rsa1 := genRsaKey(t).Public()
+	rsa2 := genRsaKey(t).Public()
+	eddsa1 := genEdDSA(t).Public()
+	eddsa2 := genEdDSA(t).Public()
+	ed25519_1, _ := genEd25519Key(t)
+	ed25519_2, _ := genEd25519Key(t)
+
+	type args struct {
+		key1Iface crypto.PublicKey
+		key2Iface crypto.PublicKey
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    bool
+		wantErr bool
+	}{
+		{name: "RSA_Equal", args: args{key1Iface: rsa1, key2Iface: rsa1}, want: true, wantErr: false},
+		{name: "RSA_NotEqual", args: args{key1Iface: rsa1, key2Iface: rsa2}, want: false, wantErr: false},
+		{name: "EDDSA_Equal", args: args{key1Iface: eddsa1, key2Iface: eddsa1}, want: true, wantErr: false},
+		{name: "EDDSA_NotEqual", args: args{key1Iface: eddsa1, key2Iface: eddsa2}, want: false, wantErr: false},
+		{name: "ED25519_Equal", args: args{key1Iface: ed25519_1, key2Iface: ed25519_1}, want: true, wantErr: false},
+		{name: "ED25519_NotEqual", args: args{key1Iface: ed25519_1, key2Iface: ed25519_2}, want: false, wantErr: false},
+		{name: "Mismatched_RSA", args: args{key1Iface: rsa1, key2Iface: ed25519_2}, want: false, wantErr: false},
+		{name: "Mismatched_EDDSA", args: args{key1Iface: ed25519_1, key2Iface: rsa1}, want: false, wantErr: false},
+		{name: "Mismatched_ED25519", args: args{key1Iface: ed25519_1, key2Iface: rsa1}, want: false, wantErr: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ComparePublicKeysAndType(tt.args.key1Iface, tt.args.key2Iface)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ComparePublicKeysAndType() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if got != tt.want {
+				t.Errorf("ComparePublicKeysAndType() got = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func genRsaKey(t *testing.T) *rsa.PrivateKey {
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return key
+}
+
+func genEdDSA(t *testing.T) *ecdsa.PrivateKey {
+	key, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return key
+}
+
+func genEd25519Key(t *testing.T) (ed25519.PublicKey, ed25519.PrivateKey) {
+	key, priv, err := ed25519.GenerateKey(rand.Reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return key, priv
+}
+
 var (
 	initTest           sync.Once
 	privRSA8KeyPem     string

sdk/helper/certutil/helpers.go

@@ -352,7 +352,21 @@ func generateSerialNumber(randReader io.Reader) (*big.Int, error) {
 	return serial, nil
 }
 
-// ComparePublicKeys compares two public keys and returns true if they match
+// ComparePublicKeysAndType compares two public keys and returns true if they match,
+// false if their types or contents differ, and an error on unsupported key types.
+func ComparePublicKeysAndType(key1Iface, key2Iface crypto.PublicKey) (bool, error) {
+	equal, err := ComparePublicKeys(key1Iface, key2Iface)
+	if err != nil {
+		if strings.Contains(err.Error(), "key types do not match:") {
+			return false, nil
+		}
+	}
+
+	return equal, err
+}
+
+// ComparePublicKeys compares two public keys and returns true if they match,
+// returns an error if public key types are mismatched, or they are an unsupported key type.
 func ComparePublicKeys(key1Iface, key2Iface crypto.PublicKey) (bool, error) {
 	switch key1Iface.(type) {
 	case *rsa.PublicKey: