Repository: buchgr/bazel-remote (go)

Issue:
Unauthenticated health probes and mTLS
With mTLS enabled, `/status` requires a valid client certificate for a successful response, preventing unauthenticated use of `/status` as a health check endpoint. (For example as an http probe in kubernetes.)

Workarounds such as just using a tcp probe, or a command probe that runs curl with a valid client certificate, do work.

Is there a recommended way of doing health checks when mTLS is enabled?

One solution could possibly be to have another endpoint, on another port, that only provides `/status` and does not require mTLS.

Pull request description:
always allow health check requests, even when using mTLS
Previously if bazel-remote was using mTLS authentication but without `--allow_unauthenticated_reads` then it would use the `tls.RequireAndVerifyClientCert` setting which prevented unauthenticated access to the health check service. We want to always allow access to the health check service regardless of authentication status.

Fixes #590.

Patch:
diff --git a/cache/disk/disk.go b/cache/disk/disk.go
index fc76e02..c955a07 100644
--- a/cache/disk/disk.go
+++ b/cache/disk/disk.go
@@ -255,7 +255,7 @@ func (c *diskCache) Put(ctx context.Context, kind cache.EntryKind, hash string,
 			os.Remove(blobFile)
 		} else if blobFile != "" {
 			// Mark the file as "complete".
-			err := os.Chmod(blobFile, tempfile.EndMode)
+			err := os.Chmod(blobFile, tempfile.FinalMode)
 			if err != nil {
 				log.Println("Failed to mark", blobFile, "as complete:", err)
 			}
@@ -583,7 +583,7 @@ func (c *diskCache) get(ctx context.Context, kind cache.EntryKind, hash string,
 			os.Remove(blobFile)
 		} else if blobFile != "" {
 			// Mark the file as "complete".
-			err := os.Chmod(blobFile, tempfile.EndMode)
+			err := os.Chmod(blobFile, tempfile.FinalMode)
 			if err != nil {
 				log.Println("Failed to mark", blobFile, "as complete:", err)
 			}
diff --git a/config/tls.go b/config/tls.go
index c233955..ea8ad5d 100644
--- a/config/tls.go
+++ b/config/tls.go
@@ -27,19 +27,16 @@ func (c *Config) setTLSConfig() error {
 			return fmt.Errorf("Error reading certificate/key pair: %w", err)
 		}
 
-		var cat tls.ClientAuthType = tls.RequireAndVerifyClientCert
-		if c.AllowUnauthenticatedReads {
-			// This allows us to handle some requests without a valid client
-			// certificate, but then we need to explicitly check for verified
-			// certs on requests that we require auth for.
-			// See server.checkGRPCClientCert and httpCache.hasValidClientCert.
-			cat = tls.VerifyClientCertIfGiven
-		}
-
 		c.TLSConfig = &tls.Config{
 			Certificates: []tls.Certificate{readCert},
 			ClientCAs:    caCertPool,
-			ClientAuth:   cat,
+
+			// This allows us to handle some requests without a valid client
+			// certificate (like the grpc health check service), but then we
+			// need to explicitly check for verified certs on requests that
+			// we require auth for.
+			// See server.checkGRPCClientCert and httpCache.hasValidClientCert.
+			ClientAuth: tls.VerifyClientCertIfGiven,
 		}
 
 		return nil
diff --git a/main.go b/main.go
index 4e48a27..f718659 100644
--- a/main.go
+++ b/main.go
@@ -237,17 +237,20 @@ func startHttpServer(c *config.Config, httpServer **http.Server,
 		WriteTimeout: c.HTTPWriteTimeout,
 	}
 
-	checkClientCertForWrites := c.TLSCaFile != "" && c.AllowUnauthenticatedReads
+	checkClientCertForReads := c.TLSCaFile != "" && !c.AllowUnauthenticatedReads
+	checkClientCertForWrites := c.TLSCaFile != ""
 	validateAC := !c.DisableHTTPACValidation
 	h := server.NewHTTPCache(diskCache, c.AccessLogger, c.ErrorLogger, validateAC,
-		c.EnableACKeyInstanceMangling, checkClientCertForWrites, gitCommit)
+		c.EnableACKeyInstanceMangling, checkClientCertForReads, checkClientCertForWrites, gitCommit)
 
 	cacheHandler := h.CacheHandler
+	var basicAuthenticator auth.BasicAuth
 	if c.HtpasswdFile != "" {
 		if c.AllowUnauthenticatedReads {
 			cacheHandler = unauthenticatedReadWrapper(cacheHandler, htpasswdSecrets, c.HTTPAddress)
 		} else {
-			cacheHandler = authWrapper(cacheHandler, htpasswdSecrets, c.HTTPAddress)
+			basicAuthenticator = auth.BasicAuth{Realm: c.HTTPAddress, Secrets: htpasswdSecrets}
+			cacheHandler = basicAuthWrapper(cacheHandler, &basicAuthenticator)
 		}
 	}
 
@@ -258,24 +261,43 @@ func startHttpServer(c *config.Config, httpServer **http.Server,
 		})
 	}
 
+	var statusHandler http.HandlerFunc = h.StatusPageHandler
+
+	if !c.AllowUnauthenticatedReads {
+		if c.TLSCaFile != "" {
+			statusHandler = h.VerifyClientCertHandler(statusHandler).ServeHTTP
+		} else if c.HtpasswdFile != "" {
+			statusHandler = basicAuthWrapper(statusHandler, &basicAuthenticator)
+		}
+	}
+
 	if c.EnableEndpointMetrics {
 		metricsMdlw := middleware.New(middleware.Config{
 			Recorder: httpmetrics.NewRecorder(httpmetrics.Config{
 				DurationBuckets: c.MetricsDurationBuckets,
 			}),
 		})
-		mux.Handle("/metrics", middlewarestd.Handler("metrics", metricsMdlw,
-			promhttp.Handler()))
-		mux.Handle("/status", middlewarestd.Handler("status", metricsMdlw,
-			http.HandlerFunc(h.StatusPageHandler)))
-		mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+
+		middlewareHandler := middlewarestd.Handler("metrics", metricsMdlw, promhttp.Handler())
+		if !c.AllowUnauthenticatedReads {
+			if c.TLSCaFile != "" {
+				middlewareHandler = h.VerifyClientCertHandler(middlewareHandler)
+			} else if c.HtpasswdFile != "" {
+				middlewareHandler = basicAuthWrapper(middlewareHandler.ServeHTTP, &basicAuthenticator)
+			}
+		}
+		mux.Handle("/metrics", middlewareHandler)
+
+		statusHandler = middlewarestd.Handler("status", metricsMdlw, http.HandlerFunc(h.StatusPageHandler)).ServeHTTP
+
+		cacheHandler = func(w http.ResponseWriter, r *http.Request) {
 			middlewarestd.Handler(r.Method, metricsMdlw, http.HandlerFunc(cacheHandler)).ServeHTTP(w, r)
-		})
-	} else {
-		mux.HandleFunc("/status", h.StatusPageHandler)
-		mux.HandleFunc("/", cacheHandler)
+		}
 	}
 
+	mux.HandleFunc("/status", statusHandler)
+	mux.HandleFunc("/", cacheHandler)
+
 	var ln net.Listener
 	var err error
 	if strings.HasPrefix(c.HTTPAddress, "unix://") {
@@ -403,8 +425,7 @@ func startGrpcServer(c *config.Config, grpcServer **grpc.Server,
 
 // A http.HandlerFunc wrapper which requires successful basic
 // authentication for all requests.
-func authWrapper(handler http.HandlerFunc, secrets auth.SecretProvider, addr string) http.HandlerFunc {
-	authenticator := &auth.BasicAuth{Realm: addr, Secrets: secrets}
+func basicAuthWrapper(handler http.HandlerFunc, authenticator *auth.BasicAuth) http.HandlerFunc {
 	return auth.JustCheck(authenticator, handler)
 }
 
diff --git a/server/grpc.go b/server/grpc.go
index 4bd1c20..2a43493 100644
--- a/server/grpc.go
+++ b/server/grpc.go
@@ -211,10 +211,7 @@ func GRPCmTLSUnaryServerInterceptor(allowUnauthenticatedReads bool) grpc.UnarySe
 }
 
 // Return a non-nil grpc error if a valid client certificate can't be
-// extracted from ctx.
-//
-// This is only used when mutual TLS authentication and unauthenticated
-// reads are enabled.
+// extracted from ctx. This is only used with mTLS authentication.
 func checkGRPCClientCert(ctx context.Context) error {
 
 	p, ok := peer.FromContext(ctx)
diff --git a/server/http.go b/server/http.go
index 53330e4..81db487 100644
--- a/server/http.go
+++ b/server/http.go
@@ -35,6 +35,7 @@ var decoder, _ = zstd.NewReader(nil) // TODO: raise WithDecoderConcurrency ?
 type HTTPCache interface {
 	CacheHandler(w http.ResponseWriter, r *http.Request)
 	StatusPageHandler(w http.ResponseWriter, r *http.Request)
+	VerifyClientCertHandler(wrapMe http.Handler) http.Handler
 }
 
 type httpCache struct {
@@ -44,6 +45,7 @@ type httpCache struct {
 	validateAC               bool
 	mangleACKeys             bool
 	gitCommit                string
+	checkClientCertForReads  bool
 	checkClientCertForWrites bool
 }
 
@@ -62,7 +64,7 @@ type statusPageData struct {
 // accessLogger will print one line for each HTTP request to stdout.
 // errorLogger will print unexpected server errors. Inexistent files and malformed URLs will not
 // be reported.
-func NewHTTPCache(cache disk.Cache, accessLogger cache.Logger, errorLogger cache.Logger, validateAC bool, mangleACKeys bool, checkClientCertForWrites bool, commit string) HTTPCache {
+func NewHTTPCache(cache disk.Cache, accessLogger cache.Logger, errorLogger cache.Logger, validateAC bool, mangleACKeys bool, checkClientCertForReads bool, checkClientCertForWrites bool, commit string) HTTPCache {
 
 	_, _, numItems, _ := cache.Stats()
 
@@ -74,6 +76,7 @@ func NewHTTPCache(cache disk.Cache, accessLogger cache.Logger, errorLogger cache
 		errorLogger:              errorLogger,
 		validateAC:               validateAC,
 		mangleACKeys:             mangleACKeys,
+		checkClientCertForReads:  checkClientCertForReads,
 		checkClientCertForWrites: checkClientCertForWrites,
 	}
 
@@ -215,6 +218,11 @@ func (h *httpCache) CacheHandler(w http.ResponseWriter, r *http.Request) {
 
 	switch m := r.Method; m {
 	case http.MethodGet:
+		if h.checkClientCertForReads && !h.hasValidClientCert(w, r) {
+			http.Error(w, "Authentication required for access", http.StatusUnauthorized)
+			h.logResponse(http.StatusUnauthorized, r)
+			return
+		}
 
 		if h.validateAC && kind == cache.AC {
 			h.handleGetValidAC(w, r, hash)
@@ -417,6 +425,11 @@ func (h *httpCache) CacheHandler(w http.ResponseWriter, r *http.Request) {
 		}
 
 	case http.MethodHead:
+		if h.checkClientCertForReads && !h.hasValidClientCert(w, r) {
+			http.Error(w, "Authentication required for access", http.StatusUnauthorized)
+			h.logResponse(http.StatusUnauthorized, r)
+			return
+		}
 
 		if h.validateAC && kind == cache.AC {
 			h.handleContainsValidAC(w, r, hash)
@@ -530,3 +543,16 @@ func (h *httpCache) hasValidClientCert(w http.ResponseWriter, r *http.Request) b
 
 	return true
 }
+
+// VerifyClientCertHandler returns a http.Handler which wraps another Handler,
+// but only calls the inner Handler if the request has a valid client cert.
+// This is only used when mutual TLS authentication is enabled.
+func (h *httpCache) VerifyClientCertHandler(wrapMe http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if !h.hasValidClientCert(w, r) {
+			return
+		}
+
+		wrapMe.ServeHTTP(w, r)
+	})
+}
diff --git a/utils/grpcreadclient/grpcreadclient.go b/utils/grpcreadclient/grpcreadclient.go
index 8d4b994..5d0310e 100644
--- a/utils/grpcreadclient/grpcreadclient.go
+++ b/utils/grpcreadclient/grpcreadclient.go
@@ -253,6 +253,13 @@ func checkCacheReadOps(conn *grpc.ClientConn, shouldWork bool) error {
 		return err
 	}
 
+	healthClient := grpc_health_v1.NewHealthClient(conn)
+
+	err = checkHealth(healthClient) // This should always work.
+	if err != nil {
+		return err
+	}
+
 	return nil
 }
 
@@ -280,11 +287,11 @@ func checkBatchReadBlobs(casClient pb.ContentAddressableStorageClient, shouldWor
 	}
 
 	if brResp.Responses[0] == nil {
-		return fmt.Errorf("Error: found nil reponse")
+		return fmt.Errorf("Error: found nil response")
 	}
 
 	if brResp.Responses[0].Status.Code != int32(codes.OK) {
-		return fmt.Errorf("Error: unexpected reponse: %s",
+		return fmt.Errorf("Error: unexpected response: %s",
 			brResp.Responses[0].Status.GetMessage())
 	}
 
@@ -444,13 +451,6 @@ func checkCacheWriteOps(conn *grpc.ClientConn, shouldWork bool) error {
 		return err
 	}
 
-	healthClient := grpc_health_v1.NewHealthClient(conn)
-
-	err = checkHealth(healthClient) // This should always work.
-	if err != nil {
-		return err
-	}
-
 	return nil
 }
 
@@ -514,15 +514,15 @@ func checkBatchUpdateBlobs(casClient pb.ContentAddressableStorageClient, shouldW
 
 	rs := resp.GetResponses()
 	if len(rs) != 1 {
-		return fmt.Errorf("Expected BatchUpdateBlobs to have 1 reponse, found %d", len(rs))
+		return fmt.Errorf("Expected BatchUpdateBlobs to have 1 response, found %d", len(rs))
 	}
 
 	if rs[0].Digest.Hash != ur.Digest.Hash {
-		return fmt.Errorf("Unexpected digest in reponse")
+		return fmt.Errorf("Unexpected digest in response")
 	}
 
 	if rs[0].Digest.SizeBytes != ur.Digest.SizeBytes {
-		return fmt.Errorf("Unexpected digest in reponse")
+		return fmt.Errorf("Unexpected digest in response")
 	}
 
 	if rs[0].Status != nil && rs[0].Status.Code != int32(codes.OK) {
@@ -612,6 +612,7 @@ func checkHealth(healthClient grpc_health_v1.HealthClient) error {
 	if resp.Status != grpc_health_v1.HealthCheckResponse_SERVING {
 		return fmt.Errorf("Expected health check to return SERVING status, got: %s", resp.Status.String())
 	}
+	fmt.Println("Health check succeeded, as expected")
 
 	return nil
 }
diff --git a/utils/tempfile/tempfile.go b/utils/tempfile/tempfile.go
index 6fbbba1..feef61c 100644
--- a/utils/tempfile/tempfile.go
+++ b/utils/tempfile/tempfile.go
@@ -35,8 +35,13 @@ func (c *Creator) ranqd1() string {
 
 const flags = os.O_RDWR | os.O_CREATE | os.O_EXCL
 
-const EndMode = 0666
-const wipMode = EndMode | os.ModeSetgid
+// The final permissions of the cache files, once they have finished
+// being uploaded.
+const FinalMode = 0664
+
+// The permissions to use for cache files that are still being uploaded.
+// The setgid bit will be unset when the upload has finished.
+const wipMode = FinalMode | os.ModeSetgid
 
 var errNoTempfile = errors.New("Failed to create a temp file")
 
@@ -48,7 +53,7 @@ var errNoTempfile = errors.New("Failed to create a temp file")
 // wrong.
 //
 // Once the file has been successfully written by the caller, it
-// should be chmod'ed to `EndMode` to mark it as complete.
+// should be chmod'ed to `FinalMode` to mark it as complete.
 func (c *Creator) Create(base string, legacy bool) (*os.File, string, error) {
 	var err error
 	var f *os.File


Test patch:
diff --git a/.bazelci/tls-tests.sh b/.bazelci/tls-tests.sh
index 20c7f7b..b2d106d 100755
--- a/.bazelci/tls-tests.sh
+++ b/.bazelci/tls-tests.sh
@@ -37,7 +37,7 @@ generate_keys() {
 
 	# Add subjectAltName aka "SAN", which replaces CN.
 	# Required for Go >= 1.15.
-	cat << EOF > domain.ext
+	cat << EOF > "$tmpdir/domain.ext"
 authorityKeyIdentifier=keyid,issuer
 basicConstraints=CA:FALSE
 keyUsage = digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment
@@ -48,7 +48,7 @@ EOF
 
 	# Self-signed server certificate:
 	openssl x509 -req -passin pass:1111 -days 358000 -in "$tmpdir/server.csr" \
-		-extfile domain.ext \
+		-extfile "$tmpdir/domain.ext" \
 		-CA "$tmpdir/ca.crt" -CAkey "$tmpdir/ca.key" -set_serial 01 -out "$tmpdir/server.crt"
 
 	# Remove passphrase from server key:
diff --git a/server/http_test.go b/server/http_test.go
index 47c4107..207f5bf 100644
--- a/server/http_test.go
+++ b/server/http_test.go
@@ -37,7 +37,7 @@ func TestDownloadFile(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, false, "")
+	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, false, false, "")
 
 	rr := httptest.NewRecorder()
 	handler := http.HandlerFunc(h.CacheHandler)
@@ -105,7 +105,7 @@ func TestUploadFilesConcurrently(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, false, "")
+	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, false, false, "")
 	handler := http.HandlerFunc(h.CacheHandler)
 
 	var wg sync.WaitGroup
@@ -169,7 +169,7 @@ func TestUploadSameFileConcurrently(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, false, "")
+	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, false, false, "")
 	handler := http.HandlerFunc(h.CacheHandler)
 
 	var wg sync.WaitGroup
@@ -210,7 +210,7 @@ func TestUploadCorruptedFile(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, false, "")
+	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, false, false, "")
 	rr := httptest.NewRecorder()
 	handler := http.HandlerFunc(h.CacheHandler)
 	handler.ServeHTTP(rr, r)
@@ -252,8 +252,9 @@ func TestUploadEmptyActionResult(t *testing.T) {
 	}
 	validate := true
 	mangle := false
-	unauthenticatedReads := false
-	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), validate, mangle, unauthenticatedReads, "")
+	checkClientCertForReads := false
+	checkClientCertForWrites := false
+	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), validate, mangle, checkClientCertForReads, checkClientCertForWrites, "")
 	rr := httptest.NewRecorder()
 	handler := http.HandlerFunc(h.CacheHandler)
 	handler.ServeHTTP(rr, r)
@@ -313,8 +314,9 @@ func testEmptyBlobAvailable(t *testing.T, method string) {
 	}
 	validate := true
 	mangle := false
-	unauthenticatedReads := false
-	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), validate, mangle, unauthenticatedReads, "")
+	checkClientCertForReads := false
+	checkClientCertForWrites := false
+	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), validate, mangle, checkClientCertForReads, checkClientCertForWrites, "")
 	rr := httptest.NewRecorder()
 	handler := http.HandlerFunc(h.CacheHandler)
 	handler.ServeHTTP(rr, r)
@@ -337,7 +339,7 @@ func TestStatusPage(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, false, "")
+	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, false, false, "")
 	rr := httptest.NewRecorder()
 	handler := http.HandlerFunc(h.StatusPageHandler)
 	handler.ServeHTTP(rr, r)
@@ -481,7 +483,7 @@ func TestRemoteReturnsNotFound(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	h := NewHTTPCache(emptyCache, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, false, "")
+	h := NewHTTPCache(emptyCache, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, false, false, "")
 	// create a fake http.Request
 	_, hash := testutils.RandomDataAndHash(1024)
 	url, _ := url.Parse(fmt.Sprintf("http://localhost:8080/ac/%s", hash))