Merge pull request #31545 from vespa-engine/mpolden/cert-sanity-check

Detect missing or mismatching certificate

10 files changed, 133 insertions, 62 deletions

diff --git a/client/go/internal/cli/cmd/cert.go b/client/go/internal/cli/cmd/cert.go
index 14e4861cec3..24b414443a0 100644
--- a/client/go/internal/cli/cmd/cert.go
+++ b/client/go/internal/cli/cmd/cert.go
@@ -160,10 +160,10 @@ func doCertAdd(cli *CLI, overwriteCertificate bool, args []string) error {
 	if pkg.HasCertificate() && !overwriteCertificate {
 		return errHint(fmt.Errorf("application package '%s' already contains a certificate", pkg.Path), "Use -f flag to force overwriting")
 	}
-	return maybeCopyCertificate(true, false, cli, target, pkg)
+	return requireCertificate(true, false, cli, target, pkg)
 }
 
-func maybeCopyCertificate(force, ignoreZip bool, cli *CLI, target vespa.Target, pkg vespa.ApplicationPackage) error {
+func requireCertificate(force, ignoreZip bool, cli *CLI, target vespa.Target, pkg vespa.ApplicationPackage) error {
 	if pkg.IsZip() {
 		if ignoreZip {
 			cli.printWarning("Cannot verify existence of "+color.CyanString("security/clients.pem")+" since '"+pkg.Path+"' is compressed",
@@ -175,10 +175,28 @@ func maybeCopyCertificate(force, ignoreZip bool, cli *CLI, target vespa.Target,
 			return errHint(fmt.Errorf("cannot add certificate to compressed application package: '%s'", pkg.Path), hint)
 		}
 	}
+	tlsOptions, err := cli.config.readTLSOptions(target.Deployment().Application, target.Type())
+	if err != nil {
+		return err
+	}
+	if len(tlsOptions.CertificatePEM) == 0 {
+		return errHint(fmt.Errorf("no certificate exists for %s", target.Deployment().Application.String()), "Try (re)creating the certificate with 'vespa auth cert'")
+	}
 	if force {
-		return copyCertificate(cli, target, pkg)
+		return copyCertificate(tlsOptions, cli, pkg)
 	}
 	if pkg.HasCertificate() {
+		matches, err := pkg.HasMatchingCertificate(tlsOptions.CertificatePEM)
+		if err != nil {
+			return err
+		}
+		if !matches {
+			return errHint(fmt.Errorf("certificate in %s does not match the stored key pair for %s",
+				filepath.Join("security", "clients.pem"),
+				target.Deployment().Application.String()),
+				"If this application was deployed using a different application ID in the past, the matching key pair may be stored under a different ID in "+cli.config.homeDir,
+				"Specify the matching application with --application, or add the current certificate to the package using --add-cert")
+		}
 		return nil
 	}
 	if cli.isTerminal() {
@@ -188,7 +206,7 @@ func maybeCopyCertificate(force, ignoreZip bool, cli *CLI, target vespa.Target,
 			return err
 		}
 		if ok {
-			return copyCertificate(cli, target, pkg)
+			return copyCertificate(tlsOptions, cli, pkg)
 		}
 	}
 	return errHint(fmt.Errorf("deployment to Vespa Cloud requires certificate in application package"),
@@ -196,15 +214,7 @@ func maybeCopyCertificate(force, ignoreZip bool, cli *CLI, target vespa.Target,
 		"Pass --add-cert to use the certificate of the current application")
 }
 
-func copyCertificate(cli *CLI, target vespa.Target, pkg vespa.ApplicationPackage) error {
-	tlsOptions, err := cli.config.readTLSOptions(target.Deployment().Application, target.Type())
-	if err != nil {
-		return err
-	}
-	hint := "Try generating the certificate with 'vespa auth cert'"
-	if tlsOptions.CertificateFile == "" {
-		return errHint(fmt.Errorf("no certificate exists for "+target.Deployment().Application.String()), hint)
-	}
+func copyCertificate(tlsOptions vespa.TLSOptions, cli *CLI, pkg vespa.ApplicationPackage) error {
 	data, err := os.ReadFile(tlsOptions.CertificateFile)
 	if err != nil {
 		return errHint(fmt.Errorf("could not read certificate file: %w", err))
diff --git a/client/go/internal/cli/cmd/config.go b/client/go/internal/cli/cmd/config.go
index fc6d69aab61..032abfd598a 100644
--- a/client/go/internal/cli/cmd/config.go
+++ b/client/go/internal/cli/cmd/config.go
@@ -429,51 +429,76 @@ func (c *Config) privateKeyPath(app vespa.ApplicationID, targetType string) (cre
 	return c.credentialsFile(app, targetType, false)
 }
 
-func (c *Config) readTLSOptions(app vespa.ApplicationID, targetType string) (vespa.TLSOptions, error) {
-	_, trustAll := c.environment["VESPA_CLI_DATA_PLANE_TRUST_ALL"]
-	cert, certOk := c.environment["VESPA_CLI_DATA_PLANE_CERT"]
-	key, keyOk := c.environment["VESPA_CLI_DATA_PLANE_KEY"]
-	caCertText, caCertOk := c.environment["VESPA_CLI_DATA_PLANE_CA_CERT"]
-	options := vespa.TLSOptions{TrustAll: trustAll}
-	// CA certificate
+func (c *Config) caCertificatePEM() ([]byte, string, error) {
+	envVar := "VESPA_CLI_DATA_PLANE_CA_CERT"
+	caCertText, caCertOk := c.environment[envVar]
 	if caCertOk {
-		options.CACertificate = []byte(caCertText)
-	} else if caCertFile := c.caCertificatePath(); caCertFile != "" {
+		return []byte(caCertText), envVar, nil
+	}
+	if caCertFile := c.caCertificatePath(); caCertFile != "" {
 		b, err := os.ReadFile(caCertFile)
 		if err != nil {
-			return options, err
+			return nil, "", err
+		}
+		return b, caCertFile, nil
+	}
+	return nil, "", nil
+}
+
+func (c *Config) credentialsPEM(envVar string, credentialsFile credentialsFile) ([]byte, string, error) {
+	if pem, ok := c.environment[envVar]; ok {
+		return []byte(pem), envVar, nil
+	}
+	pem, err := os.ReadFile(credentialsFile.path)
+	if err != nil {
+		if os.IsNotExist(err) && credentialsFile.optional {
+			return nil, "", nil
 		}
-		options.CACertificate = b
-		options.CACertificateFile = caCertFile
+		return nil, "", err
 	}
-	// Certificate and private key
-	if certOk && keyOk {
-		kp, err := tls.X509KeyPair([]byte(cert), []byte(key))
+	return []byte(pem), credentialsFile.path, nil
+}
+
+func (c *Config) readTLSOptions(app vespa.ApplicationID, targetType string) (vespa.TLSOptions, error) {
+	var options vespa.TLSOptions
+	// Certificate
+	if certPath, err := c.certificatePath(app, targetType); err == nil {
+		certPEM, certFile, err := c.credentialsPEM("VESPA_CLI_DATA_PLANE_CERT", certPath)
 		if err != nil {
 			return vespa.TLSOptions{}, err
 		}
-		options.KeyPair = []tls.Certificate{kp}
+		options.CertificatePEM = certPEM
+		options.CertificateFile = certFile
 	} else {
-		keyFile, err := c.privateKeyPath(app, targetType)
+		return vespa.TLSOptions{}, err
+	}
+	// Private key
+	if keyPath, err := c.privateKeyPath(app, targetType); err == nil {
+		keyPEM, keyFile, err := c.credentialsPEM("VESPA_CLI_DATA_PLANE_KEY", keyPath)
 		if err != nil {
 			return vespa.TLSOptions{}, err
 		}
-		certFile, err := c.certificatePath(app, targetType)
+		options.PrivateKeyPEM = keyPEM
+		options.PrivateKeyFile = keyFile
+	} else {
+		return vespa.TLSOptions{}, err
+
+	}
+	// CA certificate
+	_, options.TrustAll = c.environment["VESPA_CLI_DATA_PLANE_TRUST_ALL"]
+	caCertificate, caCertificateFile, err := c.caCertificatePEM()
+	if err != nil {
+		return vespa.TLSOptions{}, err
+	}
+	options.CACertificatePEM = caCertificate
+	options.CACertificateFile = caCertificateFile
+	// Key pair
+	if len(options.CertificatePEM) > 0 && len(options.PrivateKeyPEM) > 0 {
+		kp, err := tls.X509KeyPair(options.CertificatePEM, options.PrivateKeyPEM)
 		if err != nil {
 			return vespa.TLSOptions{}, err
 		}
-		kp, err := tls.LoadX509KeyPair(certFile.path, keyFile.path)
-		allowMissing := os.IsNotExist(err) && keyFile.optional && certFile.optional
-		if err == nil {
-			options.KeyPair = []tls.Certificate{kp}
-			options.PrivateKeyFile = keyFile.path
-			options.CertificateFile = certFile.path
-		} else if err != nil && !allowMissing {
-			return vespa.TLSOptions{}, err
-		}
-	}
-	// If we found a key pair, parse it and check expiry
-	if options.KeyPair != nil {
+		options.KeyPair = []tls.Certificate{kp}
 		cert, err := x509.ParseCertificate(options.KeyPair[0].Certificate[0])
 		if err != nil {
 			return vespa.TLSOptions{}, err
@@ -513,7 +538,7 @@ func (c *Config) authConfigPath() string {
 	return filepath.Join(c.homeDir, "auth.json")
 }
 
-func (c *Config) readAPIKey(cli *CLI, system vespa.System, tenantName string) ([]byte, error) {
+func (c *Config) readAPIKey(cli *CLI, tenantName string) ([]byte, error) {
 	if override, ok := c.apiKeyFromEnv(); ok {
 		return override, nil
 	}
diff --git a/client/go/internal/cli/cmd/config_test.go b/client/go/internal/cli/cmd/config_test.go
index b13f8365f5f..6c9321b3219 100644
--- a/client/go/internal/cli/cmd/config_test.go
+++ b/client/go/internal/cli/cmd/config_test.go
@@ -156,19 +156,19 @@ func assertConfigCommandErr(t *testing.T, configHome, expected string, args ...s
 
 func TestReadAPIKey(t *testing.T) {
 	cli, _, _ := newTestCLI(t)
-	key, err := cli.config.readAPIKey(cli, vespa.PublicSystem, "t1")
+	key, err := cli.config.readAPIKey(cli, "t1")
 	assert.Nil(t, key)
 	require.NotNil(t, err)
 
 	// From default path when it exists
 	require.Nil(t, os.WriteFile(filepath.Join(cli.config.homeDir, "t1.api-key.pem"), []byte("foo"), 0600))
-	key, err = cli.config.readAPIKey(cli, vespa.PublicSystem, "t1")
+	key, err = cli.config.readAPIKey(cli, "t1")
 	require.Nil(t, err)
 	assert.Equal(t, []byte("foo"), key)
 
 	// Cloud CI never reads key from disk as it's not expected to have any
 	cli, _, _ = newTestCLI(t, "VESPA_CLI_CLOUD_CI=true")
-	key, err = cli.config.readAPIKey(cli, vespa.PublicSystem, "t1")
+	key, err = cli.config.readAPIKey(cli, "t1")
 	require.Nil(t, err)
 	assert.Nil(t, key)
 
@@ -176,20 +176,20 @@ func TestReadAPIKey(t *testing.T) {
 	keyFile := filepath.Join(t.TempDir(), "key")
 	require.Nil(t, os.WriteFile(keyFile, []byte("bar"), 0600))
 	cli, _, _ = newTestCLI(t, "VESPA_CLI_API_KEY_FILE="+keyFile)
-	key, err = cli.config.readAPIKey(cli, vespa.PublicSystem, "t1")
+	key, err = cli.config.readAPIKey(cli, "t1")
 	require.Nil(t, err)
 	assert.Equal(t, []byte("bar"), key)
 
 	// From key specified in environment
 	cli, _, _ = newTestCLI(t, "VESPA_CLI_API_KEY=baz")
-	key, err = cli.config.readAPIKey(cli, vespa.PublicSystem, "t1")
+	key, err = cli.config.readAPIKey(cli, "t1")
 	require.Nil(t, err)
 	assert.Equal(t, []byte("baz"), key)
 
 	// Prefer Auth0 if we have auth config
 	cli, _, _ = newTestCLI(t)
 	require.Nil(t, os.WriteFile(filepath.Join(cli.config.homeDir, "auth.json"), []byte("foo"), 0600))
-	key, err = cli.config.readAPIKey(cli, vespa.PublicSystem, "t1")
+	key, err = cli.config.readAPIKey(cli, "t1")
 	require.Nil(t, err)
 	assert.Nil(t, key)
 }
@@ -209,9 +209,14 @@ func TestConfigReadTLSOptions(t *testing.T) {
 	assertTLSOptions(t, homeDir, app,
 		vespa.TargetLocal,
 		vespa.TLSOptions{
-			TrustAll:      true,
-			CACertificate: []byte("cacert"),
-			KeyPair:       []tls.Certificate{keyPair},
+			TrustAll:          true,
+			KeyPair:           []tls.Certificate{keyPair},
+			CACertificatePEM:  []byte("cacert"),
+			CertificatePEM:    pemCert,
+			PrivateKeyPEM:     pemKey,
+			CACertificateFile: "VESPA_CLI_DATA_PLANE_CA_CERT",
+			CertificateFile:   "VESPA_CLI_DATA_PLANE_CERT",
+			PrivateKeyFile:    "VESPA_CLI_DATA_PLANE_KEY",
 		},
 		"VESPA_CLI_DATA_PLANE_TRUST_ALL=true",
 		"VESPA_CLI_DATA_PLANE_CA_CERT=cacert",
@@ -230,7 +235,9 @@ func TestConfigReadTLSOptions(t *testing.T) {
 		vespa.TargetLocal,
 		vespa.TLSOptions{
 			KeyPair:           []tls.Certificate{keyPair},
-			CACertificate:     []byte("cacert"),
+			CACertificatePEM:  []byte("cacert"),
+			CertificatePEM:    pemCert,
+			PrivateKeyPEM:     pemKey,
 			CACertificateFile: caCertFile,
 			CertificateFile:   certFile,
 			PrivateKeyFile:    keyFile,
@@ -249,6 +256,8 @@ func TestConfigReadTLSOptions(t *testing.T) {
 		vespa.TargetLocal,
 		vespa.TLSOptions{
 			KeyPair:         []tls.Certificate{keyPair},
+			CertificatePEM:  pemCert,
+			PrivateKeyPEM:   pemKey,
 			CertificateFile: defaultCertFile,
 			PrivateKeyFile:  defaultKeyFile,
 		},
diff --git a/client/go/internal/cli/cmd/deploy.go b/client/go/internal/cli/cmd/deploy.go
index 718e2b27b2b..c9f08655759 100644
--- a/client/go/internal/cli/cmd/deploy.go
+++ b/client/go/internal/cli/cmd/deploy.go
@@ -68,7 +68,7 @@ $ vespa deploy -t cloud -z perf.aws-us-east-1c`,
 				opts.Version = version
 			}
 			if target.Type() == vespa.TargetCloud {
-				if err := maybeCopyCertificate(copyCert, true, cli, target, pkg); err != nil {
+				if err := requireCertificate(copyCert, true, cli, target, pkg); err != nil {
 					return err
 				}
 			}
diff --git a/client/go/internal/cli/cmd/deploy_test.go b/client/go/internal/cli/cmd/deploy_test.go
index 8d1c202e554..b8a684cf1cf 100644
--- a/client/go/internal/cli/cmd/deploy_test.go
+++ b/client/go/internal/cli/cmd/deploy_test.go
@@ -62,6 +62,21 @@ Hint: Pass --add-cert to use the certificate of the current application
 	buf.WriteString("y\n")
 	require.Nil(t, cli.Run("deploy", "--add-cert=false", "--wait=0", pkgDir2))
 	assert.Contains(t, stdout.String(), "Success: Triggered deployment")
+
+	// Missing application certificate is detected
+	stderr.Reset()
+	require.NotNil(t, cli.Run("deploy", "--application=t1.a2.i2", pkgDir2))
+	assert.Equal(t, "Error: no certificate exists for t1.a2.i2\nHint: Try (re)creating the certificate with 'vespa auth cert'\n", stderr.String())
+
+	// Mismatching certificate is detected
+	stdout.Reset()
+	stderr.Reset()
+	assert.Nil(t, cli.Run("auth", "cert", "--application=t1.a1.i1", "-f", "--no-add"))
+	require.NotNil(t, cli.Run("deploy", "--application=t1.a1.i1", pkgDir2))
+	assert.Equal(t, `Error: certificate in security/clients.pem does not match the stored key pair for t1.a1.i1
+Hint: If this application was deployed using a different application ID in the past, the matching key pair may be stored under a different ID in `+
+		cli.config.homeDir+"\nHint: Specify the matching application with --application, or add the current certificate to the package using --add-cert\n",
+		stderr.String())
 }
 
 func TestDeployCloudFastWait(t *testing.T) {
diff --git a/client/go/internal/cli/cmd/feed.go b/client/go/internal/cli/cmd/feed.go
index f69de6b3038..e696333a3f1 100644
--- a/client/go/internal/cli/cmd/feed.go
+++ b/client/go/internal/cli/cmd/feed.go
@@ -143,7 +143,7 @@ func createServices(n int, timeout time.Duration, cli *CLI, waiter *Waiter) ([]h
 		// Create a separate HTTP client for each service
 		client := cli.httpClientFactory(timeout)
 		// Feeding should always use HTTP/2
-		httputil.ForceHTTP2(client, service.TLSOptions.KeyPair, service.TLSOptions.CACertificate, service.TLSOptions.TrustAll)
+		httputil.ForceHTTP2(client, service.TLSOptions.KeyPair, service.TLSOptions.CACertificatePEM, service.TLSOptions.TrustAll)
 		service.SetClient(client)
 		services = append(services, service)
 	}
diff --git a/client/go/internal/cli/cmd/prod.go b/client/go/internal/cli/cmd/prod.go
index 08d4086719d..139e4690ed2 100644
--- a/client/go/internal/cli/cmd/prod.go
+++ b/client/go/internal/cli/cmd/prod.go
@@ -154,7 +154,7 @@ $ vespa prod deploy`,
 			if err := verifyTests(cli, pkg); err != nil {
 				return err
 			}
-			if err := maybeCopyCertificate(options.copyCert, true, cli, target, pkg); err != nil {
+			if err := requireCertificate(options.copyCert, true, cli, target, pkg); err != nil {
 				return err
 			}
 			deployment := vespa.DeploymentOptions{ApplicationPackage: pkg, Target: target}
diff --git a/client/go/internal/cli/cmd/root.go b/client/go/internal/cli/cmd/root.go
index 6d0c747954a..26e84773b9f 100644
--- a/client/go/internal/cli/cmd/root.go
+++ b/client/go/internal/cli/cmd/root.go
@@ -463,7 +463,7 @@ func (c *CLI) createCustomTarget(targetType, customURL string) (vespa.Target, er
 }
 
 func (c *CLI) cloudApiAuthenticator(deployment vespa.Deployment, system vespa.System) (vespa.Authenticator, error) {
-	apiKey, err := c.config.readAPIKey(c, system, deployment.Application.Tenant)
+	apiKey, err := c.config.readAPIKey(c, deployment.Application.Tenant)
 	if err != nil {
 		return nil, err
 	}
diff --git a/client/go/internal/vespa/application.go b/client/go/internal/vespa/application.go
index 618d23b6bab..d499006c982 100644
--- a/client/go/internal/vespa/application.go
+++ b/client/go/internal/vespa/application.go
@@ -3,6 +3,7 @@ package vespa
 
 import (
 	"archive/zip"
+	"bytes"
 	"errors"
 	"fmt"
 	"io"
@@ -21,6 +22,14 @@ type ApplicationPackage struct {
 
 func (ap *ApplicationPackage) HasCertificate() bool { return ap.hasFile("security", "clients.pem") }
 
+func (ap *ApplicationPackage) HasMatchingCertificate(certificatePEM []byte) (bool, error) {
+	clientsPEM, err := os.ReadFile(filepath.Join(ap.Path, "security", "clients.pem"))
+	if err != nil {
+		return false, err
+	}
+	return bytes.Equal(clientsPEM, certificatePEM), nil
+}
+
 func (ap *ApplicationPackage) HasDeploymentSpec() bool { return ap.hasFile("deployment.xml", "") }
 
 func (ap *ApplicationPackage) hasFile(pathSegment ...string) bool {
diff --git a/client/go/internal/vespa/target.go b/client/go/internal/vespa/target.go
index 960917b75d6..5270b5669f9 100644
--- a/client/go/internal/vespa/target.go
+++ b/client/go/internal/vespa/target.go
@@ -126,9 +126,12 @@ type Target interface {
 
 // TLSOptions holds the client certificate to use for cloud API or service requests.
 type TLSOptions struct {
-	CACertificate []byte
-	KeyPair       []tls.Certificate
-	TrustAll      bool
+	KeyPair  []tls.Certificate
+	TrustAll bool
+
+	CACertificatePEM []byte
+	CertificatePEM   []byte
+	PrivateKeyPEM    []byte
 
 	CACertificateFile string
 	CertificateFile   string
@@ -149,7 +152,7 @@ type LogOptions struct {
 func (s *Service) Do(request *http.Request, timeout time.Duration) (*http.Response, error) {
 	if !s.customClient {
 		// Do not override TLS config if a custom client has been configured
-		httputil.ConfigureTLS(s.httpClient, s.TLSOptions.KeyPair, s.TLSOptions.CACertificate, s.TLSOptions.TrustAll)
+		httputil.ConfigureTLS(s.httpClient, s.TLSOptions.KeyPair, s.TLSOptions.CACertificatePEM, s.TLSOptions.TrustAll)
 	}
 	if s.auth != nil {
 		if err := s.auth.Authenticate(request); err != nil {