From 64a5aecfdde7a068680a56bbf3af23d40cc7d52b Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Tue, 3 Nov 2020 21:37:29 +0900 Subject: [PATCH 1/6] Handle Vault issuer working as intermediate correctly This patch changes a certificate issued with Vault issuer as follows: - `ca.crt`: a root certificate, returned in `ca_chain` from Vault - `tls.crt`: a leaf certificate, plus intermediate certificates if available in `ca_chain` i.e. `tls.crt` won't include a root certificate This is a breaking change; Vault issuer had included an issuing CA as a chain in `tls.crt`, but after this change it will no longer include a root certificate when the issuing CA is not an intermediate. For `ca.crt`, it had included a issuing CA only, which can be an intermediate. `tls.crt` is not expected to contain a root certificate, as generally clients must trust root certificates in advance. It is considered redundant transmitting a root certificate from servers to clients during TLS handshake. Other issuers, e.g. ACME, behave the same. This fixes https://github.com/jetstack/cert-manager/issues/2166 This patch is based on https://github.com/jetstack/cert-manager/pull/3340 Co-authored-by: Chris Randles Signed-off-by: Sorah Fukumori --- pkg/internal/vault/vault.go | 9 +- pkg/internal/vault/vault_test.go | 289 ++++++++++++++++++++++--------- 2 files changed, 211 insertions(+), 87 deletions(-) diff --git a/pkg/internal/vault/vault.go b/pkg/internal/vault/vault.go index 7b3080bf7..df06491c9 100644 --- a/pkg/internal/vault/vault.go +++ b/pkg/internal/vault/vault.go @@ -142,12 +142,15 @@ func (v *Vault) Sign(csrPEM []byte, duration time.Duration) (cert []byte, ca []b return nil, nil, fmt.Errorf("unable to convert certificate bundle to PEM bundle: %s", err.Error()) } - var caPem []byte = nil + var caPem []byte if len(bundle.CAChain) > 0 { - caPem = []byte(bundle.CAChain[0]) + caPem = []byte(bundle.CAChain[len(bundle.CAChain)-1]) } - return []byte(bundle.ToPEMBundle()), caPem, nil + crtPems := []string{bundle.Certificate} + crtPems = append(crtPems, bundle.CAChain[0:len(bundle.CAChain)-1]...) + + return []byte(strings.Join(crtPems, "\n")), caPem, nil } func (v *Vault) setToken(client Client) error { diff --git a/pkg/internal/vault/vault_test.go b/pkg/internal/vault/vault_test.go index a151d3285..d1cc3fe1f 100644 --- a/pkg/internal/vault/vault_test.go +++ b/pkg/internal/vault/vault_test.go @@ -38,7 +38,7 @@ import ( "github.com/hashicorp/vault/sdk/helper/jsonutil" corev1 "k8s.io/api/core/v1" - "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" vaultfake "github.com/jetstack/cert-manager/pkg/internal/vault/fake" "github.com/jetstack/cert-manager/pkg/util/pki" @@ -47,24 +47,99 @@ import ( ) const ( - testCertBundle = `-----BEGIN CERTIFICATE----- -MIIDBTCCAe2gAwIBAgIUAR4qkcRVjl3D2ZNXyAjGrmJJafUwDQYJKoZIhvcNAQEL -BQAwEjEQMA4GA1UEAwwHZm9vLmJhcjAeFw0xOTA2MTExMTQ4MjZaFw0yOTA2MDgx -MTQ4MjZaMBIxEDAOBgNVBAMMB2Zvby5iYXIwggEiMA0GCSqGSIb3DQEBAQUAA4IB -DwAwggEKAoIBAQC6nycZ01dEcR1xaMdhP7HWeHEZVTCMBkvk9NJ7CjHBjEcRFPbo -koMfIeuQ2lO+mFXpLo9iJOE+fh+Pl8/vNihS9Xan23EFNYGNukmpup4zcZ5sBueA -sE9A1LwuHxCIhwutvSOatfzbw5i4LrXNncIRabNjHmJgd4j7hhRJF0PR3x5uTV0t -lMsPVtBUX2FehR3ZvJBaYRFk4ITa7wX8a9p2JQeavoeoSxX2UWGxdE9v2oMUU0Sn -+LjzoNHVWzkTZv5yn8X3GKS1Co4bWaeDmZywL8HSkK//ST/rk7UDItWgeetMRvTt -UO1xLjEYU4HO4aPEdmwVha58nzS87pdJm+LfAgMBAAGjUzBRMB0GA1UdDgQWBBR0 -B9MwNgun4l7JAyd2tqL24oRmGDAfBgNVHSMEGDAWgBR0B9MwNgun4l7JAyd2tqL2 -4oRmGDAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQAOofYo23Xv -I5fh1sg4cmayLU5TSZ1hv9/qLzqYDu/9MSJtY0ww8RotkZOL5E3sphh8JQfKnj0G -0NvJrq6RP3Bd9FfizF2k1y2Z6D/dorztd5uum6ctdylfBPgeZEemv9aCfdigAwd+ -nh5C+XrIPnsN7Xeq3N4gzyLVzdkFHbuMWTqmqJo5XaMEWP3/dzPl447z/QlSXVqe -nCSne2t3DgvoiqS+A1hVLzHeEiwwd9kmQdPUrybwXZ/i6B1sfcxf8eklbiuhtunQ -jy1M5ZaOOfj2WFwmydx1ycGdJbJiKppN3oehi7EJ2lAxwbGoKy4VD4Ks/nMu4TEY -2lUQ3SmEzoFL + testLeafCertificate = `-----BEGIN CERTIFICATE----- +MIIFFTCCAv2gAwIBAgICEAAwDQYJKoZIhvcNAQELBQAwRjELMAkGA1UEBhMCVVMx +CzAJBgNVBAgMAkNBMRQwEgYDVQQKDAtDRVJUTUFOQUdFUjEUMBIGA1UEAwwLZm9v +LmJhci5pbnQwHhcNMjAxMDAyMTQ1NzMwWhcNMjExMDEyMTQ1NzMwWjBKMQswCQYD +VQQGEwJVUzELMAkGA1UECAwCQ0ExFDASBgNVBAoMC0NFUlRNQU5BR0VSMRgwFgYD +VQQDDA9leGFtcGxlLmZvby5iYXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK +AoIBAQC8yTGzYIX3OoRma11vewbNf8dgKHc9GgvJJ29SVjaNwRAJjKOXokGOwcyQ +7Ieb1puYQ5KdSPC1IxyUx77URovIvd3Wql+J1gIxyrdN3om3uQdJ2ck6xatBZ8BI +Y3Z+6WpUQ2067Wk4KpUGfMrbGg5zVcesh6zc8J9yEiItUENeR+6GyEf+B8IJ0xqe +5lps2LaxZp6I6vaKeMELjj17Nb9r81Rjyk8BN7yX74tFE1mUGX9o75tsODU9IrYW +nqSl5gr2PO9Zb/bd6zhoncLJr9kj2tk6cLRPht+JOPoA2LAP6D0aEdC3a2XWuj2E +EsUYJR9e5C/X49VQaak0VdNnhO6RAgMBAAGjggEHMIIBAzAJBgNVHRMEAjAAMBEG +CWCGSAGG+EIBAQQEAwIGQDAzBglghkgBhvhCAQ0EJhYkT3BlblNTTCBHZW5lcmF0 +ZWQgU2VydmVyIENlcnRpZmljYXRlMB0GA1UdDgQWBBQ41U/GiA2rQtuMz6tNL55C +o4pnBDBqBgNVHSMEYzBhgBSfus9cb7UA/PCfHJAGtL6ot2EpLKFFpEMwQTEPMA0G +A1UEAwwGYmFyLmNhMQswCQYDVQQGEwJVUzELMAkGA1UECAwCQ0ExFDASBgNVBAoM +C0NFUlRNQU5BR0VSggIQADAOBgNVHQ8BAf8EBAMCBaAwEwYDVR0lBAwwCgYIKwYB +BQUHAwEwDQYJKoZIhvcNAQELBQADggIBAFFTJNqKSkkJNWWt+R7WFNIEKoaPcFH5 +yupCQRYX9LK2cXdBQpF458/PFxREyt5jKFUcDyzQhOglFYq0hfcoAc2EB3Vw8Ww9 +c4QCiCU6ehJVMRt7MzZ9uUVGCRVOA+Fa1tIFfL3dKlI+4pTSbDhNHRqDtFhfWOZK +bgtruQEUOW1lQR61AsidOF1iwDBU6ckpVY9Lc2SHEAfQFs0MoXmJ8B4MqFptF4+H +al+IAeQ1bC/2EccFYg3tq9+YKHDCyghHf8qeKJR9tZslvkHrAzuX56e0MHxM3AD6 +D0L8nG3DsrHcjK0MlVUWmq0QFnY5t+78iocLoQZzpILZYuZn3p+XNlUdW4lcqSBn +y5fUwQ3RIuvN66GBhTeDV4vzYPa7g3i9PoBFoG50Ayr6VtIVn08rnl03lgp57Edv +A5oRrSHcd8Hd8/lk0Y9BpFTnZEg7RLhFhh9nazVp1/pjwaGx449uHIGEoxREQoPq +9Q+KLGMJR2IqiNI6+U1z2j8BChTOPkuAvsnSuAXyotu4BXBL5zbDzfDoggEk1ps1 +bfHWnmdelE0WP7h7B0PSA0EXn0pdg2VQIQsknV6y3MCzFQCCSAog/OSguokXG1PG +l6fctDJ3+AF07EjtgArOBkUn7Nt3/CgMN8I1rnBZ1Vmd8yrHEP0E3yRXBL7cDj5j +Fqmd89NQLlGs +-----END CERTIFICATE-----` + + testIntermediateCa = `-----BEGIN CERTIFICATE----- +MIIFaTCCA1GgAwIBAgICEAAwDQYJKoZIhvcNAQELBQAwQTEPMA0GA1UEAwwGYmFy +LmNhMQswCQYDVQQGEwJVUzELMAkGA1UECAwCQ0ExFDASBgNVBAoMC0NFUlRNQU5B +R0VSMB4XDTIwMTAwMjE0NTY1MFoXDTMwMDkzMDE0NTY1MFowRjELMAkGA1UEBhMC +VVMxCzAJBgNVBAgMAkNBMRQwEgYDVQQKDAtDRVJUTUFOQUdFUjEUMBIGA1UEAwwL +Zm9vLmJhci5pbnQwggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoICAQCv1hQq +Gg5LN0zekkaOzu/Xk2PUDA3ysEOXfzjL1EQ5guno93sH5WCG0tUbGQbzQyCXuyG+ +bbeGiUXhF3vPPH5yAHNoLz14GQ9WRe3Y93Lco0qD3jcgw90ctrEcOsq69Eqr6PB/ +fV3P5fKmqZcy3kywaM+suvZ3AvMVMHXKODpPSo9f8uuXd5VYvjqUUllA+iGJNUu8 +hfThn+pUMDbdAEXckIfDHqZlDQxRmRsySi0kHB1Tjgo3eH/Mj+00ZSbxEhxj2HFs +0Vn81Uo0lVjvQ6oPIm19KIo2/nOZowuy94idXSWkdXCv1UjVSVRo2NHWlH3meKWs +pALeV8Z68shyVC8HjHBAbElumBTLiWQfpQYOa0MNaBRBvp3x8ajDcx5RfqG7n45i +Ezd/UrC0mcnC0dzhFySEjPgkqZ35vEcxoDDfUJ/aLt7laBL4MPZ2U2BddQjfJVQA +A0mP1l7MPBk3+qrwErci8D+iYT4I4sQs31ip/Ht5sfc9zsF9wVlOgMx44LN0AWrE +FK+ufWeB8npJAwkfwoyfwlyvJGwwhP7PMR57y24nQvife29FnqjYdA43QQzhgXO5 +iktI0B9ApVw12/cxfMFC64fcUZF8ENpCMIM0IpoYMeFNyvod4UV0OZY06zUGindh +4qwh8U3qxQ4+bTTF9hkuiKq+9nd52GRt7BSpxwIDAQABo2YwZDAdBgNVHQ4EFgQU +n7rPXG+1APzwnxyQBrS+qLdhKSwwHwYDVR0jBBgwFoAUl2WJOv19aAspu+JphX7I +PkCr2ogwEgYDVR0TAQH/BAgwBgEB/wIBADAOBgNVHQ8BAf8EBAMCAYYwDQYJKoZI +hvcNAQELBQADggIBAGLWfRG5rHuf+t+ebU+XJj0QFUWxt08glVwJLvR7EBTxMtfa +D4Da3OYCyY78SIqWHpa51UC727GRUxKZxFrNtyPBQaBmLF4lm0xRoM1EZTqtF+Qf +yJTd92YQQJNYUCZS8NvCt2+9h9d7Gb11wOc/r5IqQ9boRP2VW7UjqmqZeBeo+Dfw +5VBg8ytF+XvrEJtp2nEobbGK2ZXPkOBt1aUVz57e3BpDw6bRbWnz8fbiEsN2b7Zs +Z5JtP8NaX81ZMV8MYeX2GRH5z8xeXjdb5xyVZHlv8mWrlN7uaRhK4AlWJ9Cdcq+x +Z5rhvg7nTfd4GoTuAhGIgW8qBNGdn6DVGURu33+w1roZn4J1tfD+Se2Oo5TScM+F +068TweDlwfYP0p+2YMKmUvQxB5R74x0Y2+49btpXXHSd/euT1PVHtc84g3acTwfK +rFQvv3jOPJ99IUpYpJydpv0Rfds3zMy2FJ6uex8gmlF7+XDZUSLQLuuFcjyHybfB +zwZlUQ6EVmjOWLDdAmpeIAIkNEiogPuSz9E7xKdU+5bFYSgm6uxe8tFZSge2VEMC +vPY2RcZ6uPZwpItqPmna8beydzlYohPcNcs4eK3hblLLacBV6eltP+q4/td+y87N +yNCb90/k5dhi3YML4qoFeZjYfbY65RKHTztv5iqHH36dIZos0LucuphEWlKK +-----END CERTIFICATE-----` + testRootCa = `-----BEGIN CERTIFICATE----- +MIIFczCCA1ugAwIBAgIUcq3TKhc/RJfQOLCF2UQ805R+lkIwDQYJKoZIhvcNAQEL +BQAwQTEPMA0GA1UEAwwGYmFyLmNhMQswCQYDVQQGEwJVUzELMAkGA1UECAwCQ0Ex +FDASBgNVBAoMC0NFUlRNQU5BR0VSMB4XDTIwMTAwMjE0NTY0MVoXDTQwMDkyNzE0 +NTY0MVowQTEPMA0GA1UEAwwGYmFyLmNhMQswCQYDVQQGEwJVUzELMAkGA1UECAwC +Q0ExFDASBgNVBAoMC0NFUlRNQU5BR0VSMIICIjANBgkqhkiG9w0BAQEFAAOCAg8A +MIICCgKCAgEAmEzgpmKns5FOmfjmOm6WBcdAZ8N0oXpVmx3atfsTeedavgi71AyL +NTwL1cwi3Y6UANMvSk0tca4YAZ2phVSvgaUmNXWXNbrrS/K5R73XpDos3igHTTvP +0kuVXgYSY2iXELA+UmzQFMpjCwBOeriz3oFA0JZxQaX4h7vZqSsabOArywTihwNt +SMH8a3ErdKwkYx9DNZYIwsU8+YIViJgx55Covenj/qoJ3ULsm5B3ILlvN4KDcVeQ +UPKPa5B5MeMS5XNoX1wNzwpLGV+9QVFQScxLAm9rcksWLm3xoUkPvNZkT3891VPo +MHT5kXFmM/7ynTC/fvG5Z4vxWm105d2WwYOJqmJR7q8/TmsNzL8+J8SojqWL1xYQ +wds4/5XYgdXBMVaaFAOYDOczIU7v3hu6Zn4UGJWSlgNiC1+xBRY0iYQSnNrhtwev +w+oO8qvXZYN05/g1/o21iJRdbquE7gU7PRIzNQKZg6mMqloJxzeh1HDoOq1LIMj0 +TSYxX8971lUZVmv7ydWafzLY5JV0FM1tGANhyTuHJtDwVy8qZYiTjl75WQ3CLFof +SsoS3gFR09XhZ/2wKE5sGLGsQvlAA3arPgi9Sv5qUjye3ZpZWrjd0hSVfTNpKDxc +hIoIem892zq0KHnV7xhu1L80b6oQ9ZSGhw7drRMBoljYxNqmn4UYKp0CAwEAAaNj +MGEwHQYDVR0OBBYEFJdliTr9fWgLKbviaYV+yD5Aq9qIMB8GA1UdIwQYMBaAFJdl +iTr9fWgLKbviaYV+yD5Aq9qIMA8GA1UdEwEB/wQFMAMBAf8wDgYDVR0PAQH/BAQD +AgGGMA0GCSqGSIb3DQEBCwUAA4ICAQBXwhW6hoG0ooL6B8Mr6GlqQ40Jesv9lEHS +rhLjd3RpCoK162eL7RJQsZAKfj8wViV2FjD1t0N4L94NnkGo1fuVds3auw1ghrFd +1HEdr37s/nMvpFu6SS4bGwzdYZEEweRr2iQGxPXTXMu+Vu7EDGTXkqDKeSYZoHNX +3GfR0I8eXfNfqW+tdCYzBCOGGTQAHmfyM9YiCnMAQtHQu0ljnBdMhkoBi2EzULAR +lDWBxQE5NmzTCxhd0JHXroKXBvq2Sig2FdKsFIbN9y7colJga6eSN8xARLoLO99q +7dzly9gVOrzBkdX5kRQyzu9CesXbPc0EAqUzj3mXaxeVchZzQ1IlUQO31Mzl5y/z +e1WvQuW1BJQdcIkYp0JVEacYGi6CjjcDphFiaAajRsi7rtiODo8pidMOTXxBitD6 +O4MslwSiWvenq5apF9PzvwDndFIfSKzIE6A7/gyKKKuMYz87FTiHipsA/GAOjNUO +8kQ91o6TIF7YTjNS3u8xICa7M8qTxQIHsbsRWzPqAxRnYQkY0aRjO9+5Qqqsg5j4 +Pc7TUJiY8gW9SWhPVUPaMkTIBgfN11c6BzLlhzN2r1zaZyghXr8QmcG4kWywkX7k +oXeN5eS8iO5fx0EOvIcYQ4yRZLGafZxsLHlsZmt32N/ZZtcl4KDP5LRE7iZEOaE/ +UXY5wAUH2A== -----END CERTIFICATE-----` ) @@ -98,7 +173,7 @@ func generateCSR(t *testing.T, secretKey crypto.Signer) []byte { } type testSignT struct { - issuer *v1.Issuer + issuer *cmapi.Issuer fakeLister *listers.FakeSecretLister fakeClient *vaultfake.Client @@ -108,17 +183,29 @@ type testSignT struct { expectedCA string } +func bundlePEM(csrPEM []byte, caPEM ...string) ([]byte, error) { + bundle := certutil.Secret{ + Data: map[string]interface{}{ + "certificate": testLeafCertificate, + }, + } + if len(caPEM) > 0 { + bundle.Data["ca_chain"] = caPEM + } + return jsonutil.EncodeJSON(&bundle) +} + func TestSign(t *testing.T) { privatekey := generateRSAPrivateKey(t) csrPEM := generateCSR(t, privatekey) - bundle := certutil.Secret{ - Data: map[string]interface{}{ - "certificate": testCertBundle, - }, + bundleData, err := bundlePEM(csrPEM, testIntermediateCa) + if err != nil { + t.Errorf("failed to encode bundle for testing: %s", err) + t.FailNow() } - bundleData, err := jsonutil.EncodeJSON(&bundle) + bundleDataWithIntermediate, err := bundlePEM(csrPEM, testIntermediateCa, testRootCa) if err != nil { t.Errorf("failed to encode bundle for testing: %s", err) t.FailNow() @@ -135,7 +222,7 @@ func TestSign(t *testing.T) { "a good csr but failed request should error": { csrPEM: csrPEM, issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{}), + gen.SetIssuerVault(cmapi.VaultIssuer{}), ), fakeClient: vaultfake.NewFakeClient().WithRawRequest(nil, errors.New("request failed")), expectedErr: errors.New("failed to sign certificate by vault: request failed"), @@ -146,29 +233,45 @@ func TestSign(t *testing.T) { "a good csr and good response should return a certificate": { csrPEM: csrPEM, issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{}), + gen.SetIssuerVault(cmapi.VaultIssuer{}), ), fakeClient: vaultfake.NewFakeClient().WithRawRequest(&vault.Response{ Response: &http.Response{ Body: ioutil.NopCloser(bytes.NewReader(bundleData))}, }, nil), expectedErr: nil, - expectedCert: testCertBundle, - expectedCA: testCertBundle, + expectedCert: testLeafCertificate, + expectedCA: testIntermediateCa, }, "vault issuer with namespace specified": { csrPEM: csrPEM, issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{Namespace: "test"}), + gen.SetIssuerVault(cmapi.VaultIssuer{Namespace: "test"}), ), fakeClient: vaultfake.NewFakeClient().WithRawRequest(&vault.Response{ Response: &http.Response{ Body: ioutil.NopCloser(bytes.NewReader(bundleData))}, }, nil), expectedErr: nil, - expectedCert: testCertBundle, - expectedCA: testCertBundle, + expectedCert: testLeafCertificate, + expectedCA: testIntermediateCa, + }, + + "vault issuer with a CA chain (issuer working as a intermediate CA)": { + csrPEM: csrPEM, + issuer: gen.Issuer("vault-issuer", + gen.SetIssuerVault(cmapi.VaultIssuer{ + Namespace: "test", + }), + ), + fakeClient: vaultfake.NewFakeClient().WithRawRequest(&vault.Response{ + Response: &http.Response{ + Body: ioutil.NopCloser(bytes.NewReader(bundleDataWithIntermediate))}, + }, nil), + expectedErr: nil, + expectedCert: strings.Join([]string{testLeafCertificate, testIntermediateCa}, "\n"), + expectedCA: testRootCa, }, } @@ -180,7 +283,7 @@ func TestSign(t *testing.T) { client: test.fakeClient, } - cert, _, err := v.Sign(test.csrPEM, time.Minute) + cert, ca, err := v.Sign(test.csrPEM, time.Minute) if ((test.expectedErr == nil) != (err == nil)) && test.expectedErr != nil && test.expectedErr.Error() != err.Error() { @@ -188,9 +291,27 @@ func TestSign(t *testing.T) { name, test.expectedErr, err) } - if test.expectedCert != string(cert) { + if (test.expectedCert == "" || string(cert) == "") && test.expectedCert != string(cert) { t.Errorf("unexpected certificate in response bundle, exp=%s got=%s", test.expectedCert, cert) + } else if test.expectedCert != string(cert) { + parsedBundle, err := certutil.ParsePEMBundle(string(cert)) + if err != nil { + t.Errorf("%s: failed to decode bundle: %s", name, err) + } + bundle, err := parsedBundle.ToCertBundle() + if err != nil { + t.Errorf("%s: failed to convert bundle: %s", name, err) + } + if test.expectedCert != bundle.Certificate { + t.Errorf("%s: unexpected certificate in response bundle, exp=%s got=%s", + name, test.expectedCert, cert) + } + } + + if test.expectedCA != string(ca) { + t.Errorf("unexpected ca in response bundle, exp=%s got=%s; %s", + test.expectedCA, ca, name) } } } @@ -199,7 +320,7 @@ type testSetTokenT struct { expectedToken string expectedErr error - issuer *v1.Issuer + issuer *cmapi.Issuer fakeLister *listers.FakeSecretLister fakeClient *vaultfake.Client } @@ -225,9 +346,9 @@ func TestSetToken(t *testing.T) { tests := map[string]testSetTokenT{ "if neither token secret ref, app role secret ref, or kube auth then not found then error": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ - CABundle: []byte(testCertBundle), - Auth: v1.VaultAuth{}, + gen.SetIssuerVault(cmapi.VaultIssuer{ + CABundle: []byte(testLeafCertificate), + Auth: cmapi.VaultAuth{}, }), ), fakeLister: listers.FakeSecretListerFrom(listers.NewFakeSecretLister()), @@ -240,9 +361,9 @@ func TestSetToken(t *testing.T) { "if token secret ref is set but secret doesn't exist should error": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ - CABundle: []byte(testCertBundle), - Auth: v1.VaultAuth{ + gen.SetIssuerVault(cmapi.VaultIssuer{ + CABundle: []byte(testLeafCertificate), + Auth: cmapi.VaultAuth{ TokenSecretRef: &cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ Name: "secret-ref-name", @@ -261,9 +382,9 @@ func TestSetToken(t *testing.T) { "if token secret ref set, return client using token stored": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ - CABundle: []byte(testCertBundle), - Auth: v1.VaultAuth{ + gen.SetIssuerVault(cmapi.VaultIssuer{ + CABundle: []byte(testLeafCertificate), + Auth: cmapi.VaultAuth{ TokenSecretRef: &cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ Name: "secret-ref-name", @@ -283,10 +404,10 @@ func TestSetToken(t *testing.T) { "if app role set but secret token not but vault fails to return token, error": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ - CABundle: []byte(testCertBundle), - Auth: v1.VaultAuth{ - AppRole: &v1.VaultAppRole{ + gen.SetIssuerVault(cmapi.VaultIssuer{ + CABundle: []byte(testLeafCertificate), + Auth: cmapi.VaultAuth{ + AppRole: &cmapi.VaultAppRole{ RoleId: "my-role-id", SecretRef: cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ @@ -308,10 +429,10 @@ func TestSetToken(t *testing.T) { "if app role secret ref set, return client using token stored": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ - CABundle: []byte(testCertBundle), - Auth: v1.VaultAuth{ - AppRole: &v1.VaultAppRole{ + gen.SetIssuerVault(cmapi.VaultIssuer{ + CABundle: []byte(testLeafCertificate), + Auth: cmapi.VaultAuth{ + AppRole: &cmapi.VaultAppRole{ RoleId: "my-role-id", SecretRef: cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ @@ -340,10 +461,10 @@ func TestSetToken(t *testing.T) { "if kubernetes role auth set but reference secret doesn't exist return error": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ - CABundle: []byte(testCertBundle), - Auth: v1.VaultAuth{ - Kubernetes: &v1.VaultKubernetesAuth{ + gen.SetIssuerVault(cmapi.VaultIssuer{ + CABundle: []byte(testLeafCertificate), + Auth: cmapi.VaultAuth{ + Kubernetes: &cmapi.VaultKubernetesAuth{ Role: "kube-vault-role", SecretRef: cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ @@ -365,10 +486,10 @@ func TestSetToken(t *testing.T) { "if kubernetes role auth set but reference secret doesn't contain data at key error": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ - CABundle: []byte(testCertBundle), - Auth: v1.VaultAuth{ - Kubernetes: &v1.VaultKubernetesAuth{ + gen.SetIssuerVault(cmapi.VaultIssuer{ + CABundle: []byte(testLeafCertificate), + Auth: cmapi.VaultAuth{ + Kubernetes: &cmapi.VaultKubernetesAuth{ Role: "kube-vault-role", SecretRef: cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ @@ -390,10 +511,10 @@ func TestSetToken(t *testing.T) { "if kubernetes role auth set but errors with a raw request should error": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ - CABundle: []byte(testCertBundle), - Auth: v1.VaultAuth{ - Kubernetes: &v1.VaultKubernetesAuth{ + gen.SetIssuerVault(cmapi.VaultIssuer{ + CABundle: []byte(testLeafCertificate), + Auth: cmapi.VaultAuth{ + Kubernetes: &cmapi.VaultKubernetesAuth{ Role: "kube-vault-role", SecretRef: cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ @@ -415,10 +536,10 @@ func TestSetToken(t *testing.T) { "foo": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ - CABundle: []byte(testCertBundle), - Auth: v1.VaultAuth{ - Kubernetes: &v1.VaultKubernetesAuth{ + gen.SetIssuerVault(cmapi.VaultIssuer{ + CABundle: []byte(testLeafCertificate), + Auth: cmapi.VaultAuth{ + Kubernetes: &cmapi.VaultKubernetesAuth{ Role: "kube-vault-role", SecretRef: cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ @@ -447,10 +568,10 @@ func TestSetToken(t *testing.T) { "if app role secret ref and token secret set, take preference on token secret": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ - CABundle: []byte(testCertBundle), - Auth: v1.VaultAuth{ - AppRole: &v1.VaultAppRole{ + gen.SetIssuerVault(cmapi.VaultIssuer{ + CABundle: []byte(testLeafCertificate), + Auth: cmapi.VaultAuth{ + AppRole: &cmapi.VaultAppRole{ RoleId: "my-role-id", SecretRef: cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ @@ -506,7 +627,7 @@ type testAppRoleRefT struct { expectedSecretID string expectedErr error - appRole *v1.VaultAppRole + appRole *cmapi.VaultAppRole fakeLister *listers.FakeSecretLister } @@ -514,7 +635,7 @@ type testAppRoleRefT struct { func TestAppRoleRef(t *testing.T) { errSecretGet := errors.New("no secret found") - basicAppRoleRef := &v1.VaultAppRole{ + basicAppRoleRef := &cmapi.VaultAppRole{ RoleId: "my-role-id", } @@ -530,7 +651,7 @@ func TestAppRoleRef(t *testing.T) { }, "no data in key should fail": { - appRole: &v1.VaultAppRole{ + appRole: &cmapi.VaultAppRole{ RoleId: "", SecretRef: cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ @@ -553,7 +674,7 @@ func TestAppRoleRef(t *testing.T) { }, "should return roleID and secretID with trimmed space": { - appRole: &v1.VaultAppRole{ + appRole: &cmapi.VaultAppRole{ RoleId: " my-role-id ", SecretRef: cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ @@ -701,7 +822,7 @@ func TestTokenRef(t *testing.T) { type testNewConfigT struct { expectedErr error - issuer *v1.Issuer + issuer *cmapi.Issuer checkFunc func(cfg *vault.Config) error } @@ -709,7 +830,7 @@ func TestNewConfig(t *testing.T) { tests := map[string]testNewConfigT{ "no CA bundle set in issuer should return nil": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ + gen.SetIssuerVault(cmapi.VaultIssuer{ CABundle: nil, }), ), @@ -718,7 +839,7 @@ func TestNewConfig(t *testing.T) { "a bad cert bundle should error": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ + gen.SetIssuerVault(cmapi.VaultIssuer{ CABundle: []byte("a bad cert bundle"), }), ), @@ -727,14 +848,14 @@ func TestNewConfig(t *testing.T) { "a good cert bundle should be added to the config": { issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(v1.VaultIssuer{ - CABundle: []byte(testCertBundle), + gen.SetIssuerVault(cmapi.VaultIssuer{ + CABundle: []byte(testLeafCertificate), }), ), expectedErr: nil, checkFunc: func(cfg *vault.Config) error { testCA := x509.NewCertPool() - testCA.AppendCertsFromPEM([]byte(testCertBundle)) + testCA.AppendCertsFromPEM([]byte(testLeafCertificate)) subs := cfg.HttpClient.Transport.(*http.Transport).TLSClientConfig.RootCAs.Subjects() err := fmt.Errorf("got unexpected root CAs in config, exp=%s got=%s", @@ -780,7 +901,7 @@ func TestNewConfig(t *testing.T) { type requestTokenWithAppRoleRefT struct { client Client - appRole *v1.VaultAppRole + appRole *cmapi.VaultAppRole fakeLister *listers.FakeSecretLister @@ -789,7 +910,7 @@ type requestTokenWithAppRoleRefT struct { } func TestRequestTokenWithAppRoleRef(t *testing.T) { - basicAppRoleRef := &v1.VaultAppRole{ + basicAppRoleRef := &cmapi.VaultAppRole{ RoleId: "test-role-id", SecretRef: cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ From 2f70e9d4dbcf35252663010422a1e2036f9870fb Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Fri, 6 Nov 2020 03:40:25 +0900 Subject: [PATCH 2/6] vault: extract PEM extraction to a dedicated function Signed-off-by: Sorah Fukumori --- pkg/internal/vault/vault.go | 42 +++++++++------- pkg/internal/vault/vault_test.go | 86 +++++++++++++++++++++----------- 2 files changed, 80 insertions(+), 48 deletions(-) diff --git a/pkg/internal/vault/vault.go b/pkg/internal/vault/vault.go index df06491c9..b1f6be3cb 100644 --- a/pkg/internal/vault/vault.go +++ b/pkg/internal/vault/vault.go @@ -132,25 +132,7 @@ func (v *Vault) Sign(csrPEM []byte, duration time.Duration) (cert []byte, ca []b return nil, nil, fmt.Errorf("failed to decode response returned by vault: %s", err) } - parsedBundle, err := certutil.ParsePKIMap(vaultResult.Data) - if err != nil { - return nil, nil, fmt.Errorf("failed to decode response returned by vault: %s", err) - } - - bundle, err := parsedBundle.ToCertBundle() - if err != nil { - return nil, nil, fmt.Errorf("unable to convert certificate bundle to PEM bundle: %s", err.Error()) - } - - var caPem []byte - if len(bundle.CAChain) > 0 { - caPem = []byte(bundle.CAChain[len(bundle.CAChain)-1]) - } - - crtPems := []string{bundle.Certificate} - crtPems = append(crtPems, bundle.CAChain[0:len(bundle.CAChain)-1]...) - - return []byte(strings.Join(crtPems, "\n")), caPem, nil + return extractCertificatesFromVaultCertificateSecret(&vaultResult) } func (v *Vault) setToken(client Client) error { @@ -358,3 +340,25 @@ func (v *Vault) requestTokenWithKubernetesAuth(client Client, kubernetesAuth *v1 func (v *Vault) Sys() *vault.Sys { return v.client.Sys() } + +func extractCertificatesFromVaultCertificateSecret(secret *certutil.Secret) (cert []byte, ca []byte, err error) { + parsedBundle, err := certutil.ParsePKIMap(secret.Data) + if err != nil { + return nil, nil, fmt.Errorf("failed to decode response returned by vault: %s", err) + } + + bundle, err := parsedBundle.ToCertBundle() + if err != nil { + return nil, nil, fmt.Errorf("unable to convert certificate bundle to PEM bundle: %s", err.Error()) + } + + var caPem []byte + if len(bundle.CAChain) > 0 { + caPem = []byte(bundle.CAChain[len(bundle.CAChain)-1]) + } + + crtPems := []string{bundle.Certificate} + crtPems = append(crtPems, bundle.CAChain[0:len(bundle.CAChain)-1]...) + + return []byte(strings.Join(crtPems, "\n")), caPem, nil +} diff --git a/pkg/internal/vault/vault_test.go b/pkg/internal/vault/vault_test.go index d1cc3fe1f..7ca3e304d 100644 --- a/pkg/internal/vault/vault_test.go +++ b/pkg/internal/vault/vault_test.go @@ -183,29 +183,32 @@ type testSignT struct { expectedCA string } -func bundlePEM(csrPEM []byte, caPEM ...string) ([]byte, error) { - bundle := certutil.Secret{ +func signedCertificateSecret(issuingCaPEM string, caPEM ...string) *certutil.Secret { + secret := &certutil.Secret{ Data: map[string]interface{}{ "certificate": testLeafCertificate, }, } - if len(caPEM) > 0 { - bundle.Data["ca_chain"] = caPEM - } - return jsonutil.EncodeJSON(&bundle) + + secret.Data["issuing_ca"] = issuingCaPEM + + chain := []string{issuingCaPEM} + chain = append(chain, caPEM...) + secret.Data["ca_chain"] = chain + + return secret +} + +func bundlePEM(issuingCaPEM string, caPEM ...string) ([]byte, error) { + secret := signedCertificateSecret(issuingCaPEM, caPEM...) + return jsonutil.EncodeJSON(&secret) } func TestSign(t *testing.T) { privatekey := generateRSAPrivateKey(t) csrPEM := generateCSR(t, privatekey) - bundleData, err := bundlePEM(csrPEM, testIntermediateCa) - if err != nil { - t.Errorf("failed to encode bundle for testing: %s", err) - t.FailNow() - } - - bundleDataWithIntermediate, err := bundlePEM(csrPEM, testIntermediateCa, testRootCa) + bundleData, err := bundlePEM(testIntermediateCa) if err != nil { t.Errorf("failed to encode bundle for testing: %s", err) t.FailNow() @@ -257,22 +260,6 @@ func TestSign(t *testing.T) { expectedCert: testLeafCertificate, expectedCA: testIntermediateCa, }, - - "vault issuer with a CA chain (issuer working as a intermediate CA)": { - csrPEM: csrPEM, - issuer: gen.Issuer("vault-issuer", - gen.SetIssuerVault(cmapi.VaultIssuer{ - Namespace: "test", - }), - ), - fakeClient: vaultfake.NewFakeClient().WithRawRequest(&vault.Response{ - Response: &http.Response{ - Body: ioutil.NopCloser(bytes.NewReader(bundleDataWithIntermediate))}, - }, nil), - expectedErr: nil, - expectedCert: strings.Join([]string{testLeafCertificate, testIntermediateCa}, "\n"), - expectedCA: testRootCa, - }, } for name, test := range tests { @@ -316,6 +303,47 @@ func TestSign(t *testing.T) { } } +type testExtractCertificatesFromVaultCertT struct { + secret *certutil.Secret + expectedCert []string + expectedCA string +} + +func TestExtractCertificatesFromVaultCertificateSecret(t *testing.T) { + tests := map[string]testExtractCertificatesFromVaultCertT{ + "when a Vault engine is a root CA": { + secret: signedCertificateSecret(testIntermediateCa), + expectedCert: []string{testLeafCertificate}, + expectedCA: testIntermediateCa, + }, + "when a Vault engine is an intermediate CA, and its parent is a root CA": { + secret: signedCertificateSecret(testIntermediateCa, testRootCa), + expectedCert: []string{testLeafCertificate, testIntermediateCa}, + expectedCA: testRootCa, + }, + "when a Vault engine is an intermediate CA, and its parent is a intermediate CA": { + secret: signedCertificateSecret(testIntermediateCa, testIntermediateCa, testRootCa), + expectedCert: []string{testLeafCertificate, testIntermediateCa, testIntermediateCa}, + expectedCA: testRootCa, + }, + } + + for name, test := range tests { + cert, ca, err := extractCertificatesFromVaultCertificateSecret(test.secret) + + if err != nil { + t.Errorf("%s: failed to extract certificate: %s", name, err) + } + expectedCert := strings.Join(test.expectedCert, "\n") + if expectedCert != string(cert) { + t.Errorf("%s: unexpected leaf certificate, exp=%s, got=%s", name, expectedCert, cert) + } + if test.expectedCA != string(ca) { + t.Errorf("%s: unexpected root certificate, exp=%s, got=%s", name, test.expectedCA, cert) + } + } +} + type testSetTokenT struct { expectedToken string expectedErr error From 69a0816ca10daf531145c9c92da43d99c79a5995 Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Fri, 6 Nov 2020 04:41:55 +0900 Subject: [PATCH 3/6] vault: Handle missing ca_chain just in case certutil should ensure CAChain always set... https://github.com/hashicorp/vault/blob/3298836f6a90d95ff97af33cb2abd1f7269847df/sdk/helper/certutil/types.go Signed-off-by: Sorah Fukumori --- pkg/internal/vault/vault.go | 4 +++- pkg/internal/vault/vault_test.go | 11 ++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/internal/vault/vault.go b/pkg/internal/vault/vault.go index b1f6be3cb..c6bdddaf1 100644 --- a/pkg/internal/vault/vault.go +++ b/pkg/internal/vault/vault.go @@ -358,7 +358,9 @@ func extractCertificatesFromVaultCertificateSecret(secret *certutil.Secret) (cer } crtPems := []string{bundle.Certificate} - crtPems = append(crtPems, bundle.CAChain[0:len(bundle.CAChain)-1]...) + if len(bundle.CAChain) > 0 { + crtPems = append(crtPems, bundle.CAChain[0:len(bundle.CAChain)-1]...) + } return []byte(strings.Join(crtPems, "\n")), caPem, nil } diff --git a/pkg/internal/vault/vault_test.go b/pkg/internal/vault/vault_test.go index 7ca3e304d..92a647341 100644 --- a/pkg/internal/vault/vault_test.go +++ b/pkg/internal/vault/vault_test.go @@ -192,9 +192,14 @@ func signedCertificateSecret(issuingCaPEM string, caPEM ...string) *certutil.Sec secret.Data["issuing_ca"] = issuingCaPEM - chain := []string{issuingCaPEM} - chain = append(chain, caPEM...) - secret.Data["ca_chain"] = chain + // Vault returns ca_chain only when a certificate chain is set along with a CA certificate to Vault PKI mount + // See https://github.com/hashicorp/vault/blob/v1.5.0/builtin/logical/pki/path_issue_sign.go#L256 + // See https://github.com/hashicorp/vault/blob/v1.5.5/sdk/helper/certutil/types.go#L627 + if len(caPEM) > 0 { + chain := []string{issuingCaPEM} + chain = append(chain, caPEM...) + secret.Data["ca_chain"] = chain + } return secret } From 25fc672b80dfdc0e60f32ea534db6b0023fb588a Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Fri, 6 Nov 2020 04:47:59 +0900 Subject: [PATCH 4/6] vault: use issuing_ca field when no ca_chain available Signed-off-by: Sorah Fukumori --- pkg/internal/vault/vault.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/internal/vault/vault.go b/pkg/internal/vault/vault.go index c6bdddaf1..848b431ea 100644 --- a/pkg/internal/vault/vault.go +++ b/pkg/internal/vault/vault.go @@ -355,6 +355,8 @@ func extractCertificatesFromVaultCertificateSecret(secret *certutil.Secret) (cer var caPem []byte if len(bundle.CAChain) > 0 { caPem = []byte(bundle.CAChain[len(bundle.CAChain)-1]) + } else { + caPem = []byte(bundle.IssuingCA) } crtPems := []string{bundle.Certificate} From 90c4f9e56167fd0470e84dce8e85f521c967d7cf Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Sat, 28 Nov 2020 18:06:11 +0900 Subject: [PATCH 5/6] Avoid named return variables Co-authored-by: Maartje Eyskens Signed-off-by: Sorah Fukumori --- pkg/internal/vault/vault.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/internal/vault/vault.go b/pkg/internal/vault/vault.go index 848b431ea..1d7af478a 100644 --- a/pkg/internal/vault/vault.go +++ b/pkg/internal/vault/vault.go @@ -341,7 +341,7 @@ func (v *Vault) Sys() *vault.Sys { return v.client.Sys() } -func extractCertificatesFromVaultCertificateSecret(secret *certutil.Secret) (cert []byte, ca []byte, err error) { +func extractCertificatesFromVaultCertificateSecret(secret *certutil.Secret) ([]byte, []byte, error) { parsedBundle, err := certutil.ParsePKIMap(secret.Data) if err != nil { return nil, nil, fmt.Errorf("failed to decode response returned by vault: %s", err) From f768afd0a30a2addf330f24d6e3e58ed5b6596c9 Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Sat, 28 Nov 2020 18:12:57 +0900 Subject: [PATCH 6/6] vault: change condition precise Signed-off-by: Sorah Fukumori --- pkg/internal/vault/vault.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/internal/vault/vault.go b/pkg/internal/vault/vault.go index 1d7af478a..4389af3af 100644 --- a/pkg/internal/vault/vault.go +++ b/pkg/internal/vault/vault.go @@ -360,7 +360,7 @@ func extractCertificatesFromVaultCertificateSecret(secret *certutil.Secret) ([]b } crtPems := []string{bundle.Certificate} - if len(bundle.CAChain) > 0 { + if len(bundle.CAChain) > 1 { crtPems = append(crtPems, bundle.CAChain[0:len(bundle.CAChain)-1]...) }