From 4b6a6a1cd5c8df83b3c51a03ecab975b82057489 Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Mon, 8 Oct 2018 11:37:36 -0700 Subject: [PATCH] Allow inverted key/cert order in combined PEM file certificate.FileStore only handles (cert, key) combined PEM files. This PR allows (key, cert), which is what "openssl req -out foo.pem -keyout foo.pem" generates. --- .../k8s.io/client-go/util/certificate/BUILD | 1 - .../util/certificate/certificate_store.go | 25 +--- .../certificate/certificate_store_test.go | 128 ++++-------------- 3 files changed, 26 insertions(+), 128 deletions(-) diff --git a/staging/src/k8s.io/client-go/util/certificate/BUILD b/staging/src/k8s.io/client-go/util/certificate/BUILD index 51e48a596e1..609fb2c2834 100644 --- a/staging/src/k8s.io/client-go/util/certificate/BUILD +++ b/staging/src/k8s.io/client-go/util/certificate/BUILD @@ -24,7 +24,6 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/watch:go_default_library", "//staging/src/k8s.io/client-go/kubernetes/typed/certificates/v1beta1:go_default_library", - "//staging/src/k8s.io/client-go/util/cert:go_default_library", ], ) diff --git a/staging/src/k8s.io/client-go/util/certificate/certificate_store.go b/staging/src/k8s.io/client-go/util/certificate/certificate_store.go index f54bd6586f3..81a9e7647c2 100644 --- a/staging/src/k8s.io/client-go/util/certificate/certificate_store.go +++ b/staging/src/k8s.io/client-go/util/certificate/certificate_store.go @@ -21,7 +21,6 @@ import ( "crypto/x509" "encoding/pem" "fmt" - "io/ioutil" "os" "path/filepath" "strings" @@ -171,11 +170,9 @@ func (s *fileStore) Current() (*tls.Certificate, error) { } func loadFile(pairFile string) (*tls.Certificate, error) { - certBlock, keyBlock, err := loadCertKeyBlocks(pairFile) - if err != nil { - return nil, err - } - cert, err := tls.X509KeyPair(pem.EncodeToMemory(certBlock), pem.EncodeToMemory(keyBlock)) + // LoadX509KeyPair knows how to parse combined cert and private key from + // the same file. + cert, err := tls.LoadX509KeyPair(pairFile, pairFile) if err != nil { return nil, fmt.Errorf("could not convert data from %q into cert/key pair: %v", pairFile, err) } @@ -187,22 +184,6 @@ func loadFile(pairFile string) (*tls.Certificate, error) { return &cert, nil } -func loadCertKeyBlocks(pairFile string) (cert *pem.Block, key *pem.Block, err error) { - data, err := ioutil.ReadFile(pairFile) - if err != nil { - return nil, nil, fmt.Errorf("could not load cert/key pair from %q: %v", pairFile, err) - } - certBlock, rest := pem.Decode(data) - if certBlock == nil { - return nil, nil, fmt.Errorf("could not decode the first block from %q from expected PEM format", pairFile) - } - keyBlock, _ := pem.Decode(rest) - if keyBlock == nil { - return nil, nil, fmt.Errorf("could not decode the second block from %q from expected PEM format", pairFile) - } - return certBlock, keyBlock, nil -} - func (s *fileStore) Update(certData, keyData []byte) (*tls.Certificate, error) { ts := time.Now().Format("2006-01-02-15-04-05") pemFilename := s.filename(ts) diff --git a/staging/src/k8s.io/client-go/util/certificate/certificate_store_test.go b/staging/src/k8s.io/client-go/util/certificate/certificate_store_test.go index 5d4d860d0b4..ba4c55647c4 100644 --- a/staging/src/k8s.io/client-go/util/certificate/certificate_store_test.go +++ b/staging/src/k8s.io/client-go/util/certificate/certificate_store_test.go @@ -17,12 +17,11 @@ limitations under the License. package certificate import ( + "bytes" "io/ioutil" "os" "path/filepath" "testing" - - "k8s.io/client-go/util/cert" ) func TestUpdateSymlinkExistingFileError(t *testing.T) { @@ -178,96 +177,6 @@ func TestUpdateSymlinkReplaceExistingSymlink(t *testing.T) { } } -func TestLoadCertKeyBlocksNoFile(t *testing.T) { - dir, err := ioutil.TempDir("", "k8s-test-load-cert-key-blocks") - if err != nil { - t.Fatalf("Unable to create the test directory %q: %v", dir, err) - } - defer func() { - if err := os.RemoveAll(dir); err != nil { - t.Errorf("Unable to clean up test directory %q: %v", dir, err) - } - }() - - pairFile := filepath.Join(dir, "kubelet-pair.pem") - - if _, _, err := loadCertKeyBlocks(pairFile); err == nil { - t.Errorf("Got no error, but expected %q not found.", pairFile) - } -} - -func TestLoadCertKeyBlocksEmptyFile(t *testing.T) { - dir, err := ioutil.TempDir("", "k8s-test-load-cert-key-blocks") - if err != nil { - t.Fatalf("Unable to create the test directory %q: %v", dir, err) - } - defer func() { - if err := os.RemoveAll(dir); err != nil { - t.Errorf("Unable to clean up test directory %q: %v", dir, err) - } - }() - - pairFile := filepath.Join(dir, "kubelet-pair.pem") - if err := ioutil.WriteFile(pairFile, nil, 0600); err != nil { - t.Fatalf("Unable to create the file %q: %v", pairFile, err) - } - - if _, _, err := loadCertKeyBlocks(pairFile); err == nil { - t.Errorf("Got no error, but expected %q not found.", pairFile) - } -} - -func TestLoadCertKeyBlocksPartialFile(t *testing.T) { - dir, err := ioutil.TempDir("", "k8s-test-load-cert-key-blocks") - if err != nil { - t.Fatalf("Unable to create the test directory %q: %v", dir, err) - } - defer func() { - if err := os.RemoveAll(dir); err != nil { - t.Errorf("Unable to clean up test directory %q: %v", dir, err) - } - }() - - pairFile := filepath.Join(dir, "kubelet-pair.pem") - if err := ioutil.WriteFile(pairFile, storeCertData.certificatePEM, 0600); err != nil { - t.Fatalf("Unable to create the file %q: %v", pairFile, err) - } - - if _, _, err := loadCertKeyBlocks(pairFile); err == nil { - t.Errorf("Got no error, but expected %q invalid.", pairFile) - } -} - -func TestLoadCertKeyBlocks(t *testing.T) { - dir, err := ioutil.TempDir("", "k8s-test-load-cert-key-blocks") - if err != nil { - t.Fatalf("Unable to create the test directory %q: %v", dir, err) - } - defer func() { - if err := os.RemoveAll(dir); err != nil { - t.Errorf("Unable to clean up test directory %q: %v", dir, err) - } - }() - - pairFile := filepath.Join(dir, "kubelet-pair.pem") - data := append(storeCertData.certificatePEM, []byte("\n")...) - data = append(data, storeCertData.keyPEM...) - if err := ioutil.WriteFile(pairFile, data, 0600); err != nil { - t.Fatalf("Unable to create the file %q: %v", pairFile, err) - } - - certBlock, keyBlock, err := loadCertKeyBlocks(pairFile) - if err != nil { - t.Errorf("Got %v, but expected no error.", pairFile) - } - if certBlock.Type != cert.CertificateBlockType { - t.Errorf("Got %q loaded from the pair file, expected a %q.", certBlock.Type, cert.CertificateBlockType) - } - if keyBlock.Type != cert.RSAPrivateKeyBlockType { - t.Errorf("Got %q loaded from the pair file, expected a %q.", keyBlock.Type, cert.RSAPrivateKeyBlockType) - } -} - func TestLoadFile(t *testing.T) { dir, err := ioutil.TempDir("", "k8s-test-load-cert-key-blocks") if err != nil { @@ -280,21 +189,30 @@ func TestLoadFile(t *testing.T) { }() pairFile := filepath.Join(dir, "kubelet-pair.pem") - data := append(storeCertData.certificatePEM, []byte("\n")...) - data = append(data, storeCertData.keyPEM...) - if err := ioutil.WriteFile(pairFile, data, 0600); err != nil { - t.Fatalf("Unable to create the file %q: %v", pairFile, err) - } - cert, err := loadFile(pairFile) - if err != nil { - t.Fatalf("Could not load certificate from disk: %v", err) + tests := []struct { + desc string + data []byte + }{ + {desc: "cert and key", data: bytes.Join([][]byte{storeCertData.certificatePEM, storeCertData.keyPEM}, []byte("\n"))}, + {desc: "key and cert", data: bytes.Join([][]byte{storeCertData.keyPEM, storeCertData.certificatePEM}, []byte("\n"))}, } - if cert == nil { - t.Fatalf("There was no error, but no certificate data was returned.") - } - if cert.Leaf == nil { - t.Fatalf("Got an empty leaf, expected private data.") + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + if err := ioutil.WriteFile(pairFile, tt.data, 0600); err != nil { + t.Fatalf("Unable to create the file %q: %v", pairFile, err) + } + cert, err := loadFile(pairFile) + if err != nil { + t.Fatalf("Could not load certificate from disk: %v", err) + } + if cert == nil { + t.Fatalf("There was no error, but no certificate data was returned.") + } + if cert.Leaf == nil { + t.Fatalf("Got an empty leaf, expected private data.") + } + }) } }