Merge pull request #2235 from kubevirt-bot/cherry-pick-2183-to-releas…

…e-0.16

[release-0.16] Don't fail virt-api start when client-ca-file doesn't exist

pkg/virt-api/api.go

@@ -25,11 +25,10 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
-	"os"
 	"path/filepath"
 	"sync"
 
-	restful "github.com/emicklei/go-restful"
+	"github.com/emicklei/go-restful"
 	restfulspec "github.com/emicklei/go-restful-openapi"
 	"github.com/go-openapi/spec"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
@@ -112,11 +111,10 @@ type virtAPIApp struct {
 	signingCertBytes           []byte
 	certBytes                  []byte
 	keyBytes                   []byte
-	clientCABytes              []byte
 	requestHeaderClientCABytes []byte
 	certFile                   string
 	keyFile                    string
-	clientCAFile               string
+	caFile                     string
 	signingCertFile            string
 	namespace                  string
 	tlsConfig                  *tls.Config
@@ -389,18 +387,14 @@ func (app *virtAPIApp) getClientCert() error {
 		return err
 	}
 
-	clientCA, ok := authConfigMap.Data["client-ca-file"]
-	if !ok {
-		return fmt.Errorf("client-ca-file value not found in auth config map.")
-	}
-	app.clientCABytes = []byte(clientCA)
-
-	// request-header-ca-file doesn't always exist in all deployments.
-	// set it if the value is set though.
+	// The request-header CA is mandatory. It can be retrieved from the configmap as we do here, or it must be provided
+	// via flag on start of this apiserver. Since we don't do the latter, the former is mandatory for us
+	// see https://github.com/kubernetes-incubator/apiserver-builder-alpha/blob/master/docs/concepts/auth.md#requestheader-authentication
 	requestHeaderClientCA, ok := authConfigMap.Data["requestheader-client-ca-file"]
-	if ok {
-		app.requestHeaderClientCABytes = []byte(requestHeaderClientCA)
+	if !ok {
+		return fmt.Errorf("requestheader-client-ca-file not found in extension-apiserver-authentication ConfigMap")
 	}
+	app.requestHeaderClientCABytes = []byte(requestHeaderClientCA)
 
 	// This config map also contains information about what
 	// headers our authorizor should inspect
@@ -923,27 +917,13 @@ func (app *virtAPIApp) setupTLS(fs Filesystem) error {
 	app.keyFile = filepath.Join(app.certsDirectory, "/key.pem")
 	app.certFile = filepath.Join(app.certsDirectory, "/cert.pem")
 	app.signingCertFile = filepath.Join(app.certsDirectory, "/signingCert.pem")
-	app.clientCAFile = filepath.Join(app.certsDirectory, "/clientCA.crt")
+	app.caFile = filepath.Join(app.certsDirectory, "/clientCA.crt")
 
 	// Write the certs to disk
-	err := fs.WriteFile(app.clientCAFile, app.clientCABytes, 0600)
+	err := fs.WriteFile(app.caFile, app.requestHeaderClientCABytes, 0600)
 	if err != nil {
 		return err
 	}
-
-	if len(app.requestHeaderClientCABytes) != 0 {
-		f, err := fs.OpenFile(app.clientCAFile, os.O_APPEND|os.O_WRONLY, 0600)
-		if err != nil {
-			return err
-		}
-		defer f.Close()
-
-		_, err = f.Write(app.requestHeaderClientCABytes)
-		if err != nil {
-			return err
-		}
-	}
-
 	err = fs.WriteFile(app.keyFile, app.keyBytes, 0600)
 	if err != nil {
 		return err
@@ -959,7 +939,7 @@ func (app *virtAPIApp) setupTLS(fs Filesystem) error {
 
 	// create the client CA pool.
 	// This ensures we're talking to the k8s api server
-	pool, err := cert.NewPool(app.clientCAFile)
+	pool, err := cert.NewPool(app.caFile)
 	if err != nil {
 		return err
 	}

pkg/virt-api/api_test.go

@@ -50,14 +50,13 @@ const namespaceKubevirt = "kubevirt"
 
 var _ = Describe("Virt-api", func() {
 	var app virtAPIApp
-	var tmpDir, keyFile, certFile, signingCertFile, clientCAFile string
+	var tmpDir, keyFile, certFile, signingCertFile, caFile string
 	var goodPemCertificate1, goodPemCertificate2, badPemCertificate string
 	var server *ghttp.Server
 	var backend *httptest.Server
 	var ctrl *gomock.Controller
 	var authorizorMock *rest.MockVirtApiAuthorizor
 	var filesystemMock *MockFilesystem
-	var fileMock *MockFile
 	var expectedValidatingWebhooks *admissionregistrationv1beta1.ValidatingWebhookConfiguration
 	var expectedMutatingWebhooks *admissionregistrationv1beta1.MutatingWebhookConfiguration
 	var restrictiveMode os.FileMode
@@ -75,7 +74,7 @@ var _ = Describe("Virt-api", func() {
 		app.certsDirectory = tmpDir
 		keyFile = filepath.Join(app.certsDirectory, "/key.pem")
 		certFile = filepath.Join(app.certsDirectory, "/cert.pem")
-		clientCAFile = filepath.Join(app.certsDirectory, "/clientCA.crt")
+		caFile = filepath.Join(app.certsDirectory, "/clientCA.crt")
 		signingCertFile = filepath.Join(app.certsDirectory, "/signingCert.pem")
 		restrictiveMode = 0600
 
@@ -87,7 +86,6 @@ var _ = Describe("Virt-api", func() {
 		ctrl = gomock.NewController(GinkgoT())
 		authorizorMock = rest.NewMockVirtApiAuthorizor(ctrl)
 		filesystemMock = NewMockFilesystem(ctrl)
-		fileMock = NewMockFile(ctrl)
 
 		// Reset go-restful
 		http.DefaultServeMux = new(http.ServeMux)
@@ -267,7 +265,7 @@ xw==
 			Expect(app.keyBytes).To(Equal(keyBytes))
 		}, 5)
 
-		It("should return error if client CA doesn't exist", func() {
+		It("should return error if extension-apiserver-authentication ConfigMap doesn't exist", func() {
 			server.AppendHandlers(
 				ghttp.CombineHandlers(
 					ghttp.VerifyRequest("GET", "/api/v1/namespaces/kube-system/configmaps/extension-apiserver-authentication"),
@@ -280,11 +278,11 @@ xw==
 
 		}, 5)
 
-		It("should retrieve client CA", func() {
+		It("should retrieve requestheader CA only", func() {
 
 			configMap := &k8sv1.ConfigMap{}
 			configMap.Data = make(map[string]string)
-			configMap.Data["client-ca-file"] = "fakedata"
+			configMap.Data["requestheader-client-ca-file"] = "morefakedata"
 			server.AppendHandlers(
 				ghttp.CombineHandlers(
 					ghttp.VerifyRequest("GET", "/api/v1/namespaces/kube-system/configmaps/extension-apiserver-authentication"),
@@ -294,13 +292,28 @@ xw==
 
 			err := app.getClientCert()
 			Expect(err).ToNot(HaveOccurred())
-			Expect(app.clientCABytes).To(Equal([]byte("fakedata")))
+			Expect(app.requestHeaderClientCABytes).To(Equal([]byte("morefakedata")))
+		}, 5)
+
+		It("should fail without requestheader CA", func() {
+
+			configMap := &k8sv1.ConfigMap{}
+			configMap.Data = make(map[string]string)
+			server.AppendHandlers(
+				ghttp.CombineHandlers(
+					ghttp.VerifyRequest("GET", "/api/v1/namespaces/kube-system/configmaps/extension-apiserver-authentication"),
+					ghttp.RespondWithJSONEncoded(http.StatusOK, configMap),
+				),
+			)
+
+			err := app.getClientCert()
+			Expect(err).To(HaveOccurred())
 		}, 5)
 
 		It("should auto detect correct request headers from cert configmap", func() {
 			configMap := &k8sv1.ConfigMap{}
 			configMap.Data = make(map[string]string)
-			configMap.Data["client-ca-file"] = "fakedata"
+			configMap.Data["requestheader-client-ca-file"] = "morefakedata"
 			configMap.Data["requestheader-username-headers"] = "[\"fakeheader1\"]"
 			configMap.Data["requestheader-group-headers"] = "[\"fakeheader2\"]"
 			configMap.Data["requestheader-extra-headers-prefix"] = "[\"fakeheader3-\"]"
@@ -566,18 +579,19 @@ xw==
 			Expect(err).To(HaveOccurred())
 		}, 5)
 
-		It("should fail setupTLS at clientCAFile write error", func() {
-			clientCAFile := filepath.Join(app.certsDirectory, "/clientCA.crt")
+		It("should fail setupTLS without any data available at key write error", func() {
 			filesystemMock.EXPECT().
-				WriteFile(clientCAFile, app.clientCABytes, restrictiveMode).
-				Return(errors.New("fake error writing " + clientCAFile))
+				WriteFile(caFile, app.requestHeaderClientCABytes, restrictiveMode)
+			filesystemMock.EXPECT().
+				WriteFile(keyFile, app.keyBytes, restrictiveMode).
+				Return(errors.New("fake error writing " + caFile))
 			err := app.setupTLS(filesystemMock)
 			Expect(err).To(HaveOccurred())
 		}, 5)
 
 		It("should fail setupTLS at keyBytes write error", func() {
 			filesystemMock.EXPECT().
-				WriteFile(clientCAFile, app.clientCABytes, restrictiveMode)
+				WriteFile(caFile, app.requestHeaderClientCABytes, restrictiveMode)
 			filesystemMock.EXPECT().
 				WriteFile(keyFile, app.keyBytes, restrictiveMode).
 				Return(errors.New("fake error writing " + keyFile))
@@ -587,7 +601,7 @@ xw==
 
 		It("should fail setupTLS at certFile write error", func() {
 			filesystemMock.EXPECT().
-				WriteFile(clientCAFile, app.clientCABytes, restrictiveMode)
+				WriteFile(caFile, app.requestHeaderClientCABytes, restrictiveMode)
 			filesystemMock.EXPECT().
 				WriteFile(keyFile, app.keyBytes, restrictiveMode)
 			filesystemMock.EXPECT().
@@ -599,7 +613,7 @@ xw==
 
 		It("should fail setupTLS at signingCertBytes write error", func() {
 			filesystemMock.EXPECT().
-				WriteFile(clientCAFile, app.clientCABytes, restrictiveMode)
+				WriteFile(caFile, app.requestHeaderClientCABytes, restrictiveMode)
 			filesystemMock.EXPECT().
 				WriteFile(keyFile, app.keyBytes, restrictiveMode)
 			filesystemMock.EXPECT().
@@ -613,7 +627,7 @@ xw==
 
 		It("should fail setupTLS at new pool error", func() {
 			filesystemMock.EXPECT().
-				WriteFile(clientCAFile, app.clientCABytes, restrictiveMode)
+				WriteFile(caFile, app.requestHeaderClientCABytes, restrictiveMode)
 			filesystemMock.EXPECT().
 				WriteFile(keyFile, app.keyBytes, restrictiveMode)
 			filesystemMock.EXPECT().
@@ -624,65 +638,34 @@ xw==
 			Expect(err).To(HaveOccurred())
 		}, 5)
 
-		It("should fail when client CA from request at open error", func() {
+		It("should fail setupTLS at requestheader CA write error", func() {
 			app.requestHeaderClientCABytes = []byte(goodPemCertificate1)
 			filesystemMock.EXPECT().
-				WriteFile(clientCAFile, app.clientCABytes, restrictiveMode)
-			filesystemMock.EXPECT().
-				OpenFile(clientCAFile, os.O_APPEND|os.O_WRONLY, restrictiveMode).
-				Return(nil, errors.New("fake error opening "+clientCAFile))
-			err := app.setupTLS(filesystemMock)
-			Expect(err).To(HaveOccurred())
-		}, 5)
-
-		It("should fail when client CA from request at write error", func() {
-			app.requestHeaderClientCABytes = []byte(goodPemCertificate1)
-			filesystemMock.EXPECT().
-				WriteFile(clientCAFile, app.clientCABytes, restrictiveMode)
-			filesystemMock.EXPECT().
-				OpenFile(clientCAFile, os.O_APPEND|os.O_WRONLY, restrictiveMode).
-				Return(fileMock, nil)
-			fileMock.EXPECT().
-				Write(app.requestHeaderClientCABytes).
-				Return(0, errors.New("fake error writing request client CA"))
-			fileMock.EXPECT().Close()
+				WriteFile(caFile, app.requestHeaderClientCABytes, restrictiveMode).
+				Return(errors.New("fake error opening " + caFile))
 			err := app.setupTLS(filesystemMock)
 			Expect(err).To(HaveOccurred())
 		}, 5)
 
-		It("should pass setupTLS at good client CA from config", func() {
-			app.clientCABytes = []byte(goodPemCertificate1)
-			err := app.setupTLS(IOUtil{})
-			Expect(err).ToNot(HaveOccurred())
-		}, 5)
-
-		It("should fail setupTLS at bad client CA from config", func() {
-			app.clientCABytes = []byte(badPemCertificate)
-			err := app.setupTLS(IOUtil{})
-			Expect(len(app.requestHeaderClientCABytes)).To(Equal(0))
-			Expect(err).To(HaveOccurred())
-		}, 5)
-
-		It("should pass setupTLS at good client CA from request", func() {
+		It("should pass setupTLS at good requestheader CA", func() {
 			app.requestHeaderClientCABytes = []byte(goodPemCertificate1)
 			err := app.setupTLS(IOUtil{})
 			Expect(err).ToNot(HaveOccurred())
 		}, 5)
 
-		It("should fail setupTLS at bad client CA from request", func() {
+		It("should fail setupTLS at bad requestheader CA", func() {
 			app.requestHeaderClientCABytes = []byte(badPemCertificate)
 			err := app.setupTLS(IOUtil{})
 			Expect(err).To(HaveOccurred())
 		}, 5)
 
-		It("should concatenate at setupTLS client CA from request and config", func() {
-			app.clientCABytes = []byte(goodPemCertificate1)
+		It("should write requestheader CA only at setupTLS", func() {
 			app.requestHeaderClientCABytes = []byte(goodPemCertificate2)
 			err := app.setupTLS(IOUtil{})
 			Expect(err).ToNot(HaveOccurred())
-			clientCABytes, err := ioutil.ReadFile(clientCAFile)
+			clientCABytes, err := ioutil.ReadFile(caFile)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(clientCABytes).To(Equal([]byte(goodPemCertificate1 + goodPemCertificate2)))
+			Expect(clientCABytes).To(Equal([]byte(goodPemCertificate2)))
 		}, 5)
 
 		It("should have default values for flags", func() {

pkg/virt-api/rest/authorizer.go

@@ -207,7 +207,7 @@ func isInfoOrHealthEndpoint(req *restful.Request) bool {
 func isAuthenticated(req *restful.Request) bool {
 	// Peer cert is required for authentication.
 	// If the peer's cert is provided, we are guaranteed
-	// it has been validated against our client CA pool
+	// it has been validated against our CA pool containing the requestheader CA
 	if req.Request == nil || req.Request.TLS == nil || len(req.Request.TLS.PeerCertificates) == 0 {
 		return false
 	}