From fb576bdcd82091d1c9b42f362ce43f841a944c12 Mon Sep 17 00:00:00 2001
From: Andy Zhao <andyzhao@google.com>
Date: Mon, 23 Nov 2020 12:17:18 -0800
Subject: [PATCH 01/11] fix(google-api-go-generator): add temporary patch for
 compute mtls endpoint

---
 google-api-go-generator/gen.go | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/google-api-go-generator/gen.go b/google-api-go-generator/gen.go
index 33aeaf42859..e934f836122 100644
--- a/google-api-go-generator/gen.go
+++ b/google-api-go-generator/gen.go
@@ -499,6 +499,11 @@ func (a *API) mtlsAPIBaseURL() string {
 	if a.doc.MTLSRootURL != "" {
 		return resolveRelative(a.doc.MTLSRootURL, a.doc.ServicePath)
 	}
+	// TODO(andyrzhao): Remove the workaround below when MTLSRootURL becomes available in
+	// compute discovery doc, after compute migrates to OP discovery doc gen (ETA 2021).
+	if a.doc.MTLSRootURL == "" && a.doc.RootURL == "https://compute.googleapis.com/" {
+		return resolveRelative("https://compute.mtls.googleapis.com/", a.doc.ServicePath)
+	}
 	return ""
 }
 

From 502ec8b5c0b451f81e9d65c85ea94f8ce159ec7e Mon Sep 17 00:00:00 2001
From: Andy Zhao <andyzhao@google.com>
Date: Mon, 1 Feb 2021 14:34:52 -0800
Subject: [PATCH 02/11] fix(transport): expand OS environment variables in cert
 provider command

---
 transport/cert/default_cert.go | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/transport/cert/default_cert.go b/transport/cert/default_cert.go
index 141ae457936..04aefec0afa 100644
--- a/transport/cert/default_cert.go
+++ b/transport/cert/default_cert.go
@@ -110,6 +110,10 @@ func (s *secureConnectSource) getClientCertificate(info *tls.CertificateRequestI
 	if defaultCert.cachedCert != nil && !isCertificateExpired(defaultCert.cachedCert) {
 		return defaultCert.cachedCert, nil
 	}
+	// Expand OS environment variables in the cert provider command such as "$HOME".
+	for i := 0; i < len(s.metadata.Cmd); i++ {
+		s.metadata.Cmd[i] = os.ExpandEnv(s.metadata.Cmd[i])
+	}
 	command := s.metadata.Cmd
 	data, err := exec.Command(command[0], command[1:]...).Output()
 	if err != nil {

From 14ab00fb479556c2ab2844bb91b1efbc55983790 Mon Sep 17 00:00:00 2001
From: Andy Zhao <andyzhao@google.com>
Date: Fri, 3 Jun 2022 16:38:01 -0700
Subject: [PATCH 03/11] feat(transport): Integrate with enterprise certificate
 proxy

---
 go.mod                                        |   1 +
 go.sum                                        |   2 +
 transport/cert/default_cert.go                | 120 ++----------------
 transport/cert/default_cert_test.go           | 107 ----------------
 transport/cert/enterprise_cert.go             |  49 +++++++
 transport/cert/enterprise_cert_test.go        |  47 +++++++
 transport/cert/internal/test_signer.go        | 100 +++++++++++++++
 transport/cert/secureconnect_cert.go          | 117 +++++++++++++++++
 transport/cert/secureconnect_cert_test.go     | 118 +++++++++++++++++
 .../cert/testdata/context_aware_metadata.json |   3 +
 .../context_aware_metadata_invalid_pem.json   |   3 +
 ...ontext_aware_metadata_nonexpiring_pem.json |   3 +
 .../enterprise_certificate_config.json        |   8 ++
 ...rprise_certificate_config_invalid_pem.json |   8 ++
 transport/cert/testdata/invalid.pem           |   6 +
 ...onexpiringtestcert.pem => nonexpiring.pem} |   0
 transport/cert/testdata/signer.sh             |   7 +
 transport/cert/testdata/signer_invalid_pem.sh |   7 +
 18 files changed, 493 insertions(+), 213 deletions(-)
 delete mode 100644 transport/cert/default_cert_test.go
 create mode 100644 transport/cert/enterprise_cert.go
 create mode 100644 transport/cert/enterprise_cert_test.go
 create mode 100644 transport/cert/internal/test_signer.go
 create mode 100644 transport/cert/secureconnect_cert.go
 create mode 100644 transport/cert/secureconnect_cert_test.go
 create mode 100644 transport/cert/testdata/context_aware_metadata.json
 create mode 100644 transport/cert/testdata/context_aware_metadata_invalid_pem.json
 create mode 100644 transport/cert/testdata/context_aware_metadata_nonexpiring_pem.json
 create mode 100644 transport/cert/testdata/enterprise_certificate_config.json
 create mode 100644 transport/cert/testdata/enterprise_certificate_config_invalid_pem.json
 create mode 100644 transport/cert/testdata/invalid.pem
 rename transport/cert/testdata/{nonexpiringtestcert.pem => nonexpiring.pem} (100%)
 create mode 100755 transport/cert/testdata/signer.sh
 create mode 100755 transport/cert/testdata/signer_invalid_pem.sh

diff --git a/go.mod b/go.mod
index 32d063e20e8..48885b7d9be 100644
--- a/go.mod
+++ b/go.mod
@@ -5,6 +5,7 @@ go 1.15
 require (
 	cloud.google.com/go/compute v1.6.1
 	github.com/google/go-cmp v0.5.8
+	github.com/googleapis/enterprise-certificate-proxy v0.0.0-20220520183353-fd19c99a87aa // indirect
 	github.com/googleapis/gax-go/v2 v2.4.0
 	go.opencensus.io v0.23.0
 	golang.org/x/net v0.0.0-20220526153639-5463443f8c37
diff --git a/go.sum b/go.sum
index d6ae8de057a..9e43012a103 100644
--- a/go.sum
+++ b/go.sum
@@ -157,6 +157,8 @@ github.com/google/pprof v0.0.0-20210609004039-a478d1d731e9/go.mod h1:kpwsk12EmLe
 github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE=
 github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI=
 github.com/google/uuid v1.1.2/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
+github.com/googleapis/enterprise-certificate-proxy v0.0.0-20220520183353-fd19c99a87aa h1:7MYGT2XEMam7Mtzv1yDUYXANedWvwk3HKkR3MyGowy8=
+github.com/googleapis/enterprise-certificate-proxy v0.0.0-20220520183353-fd19c99a87aa/go.mod h1:17drOmN3MwGY7t0e+Ei9b45FFGA3fBs3x36SsCg1hq8=
 github.com/googleapis/gax-go/v2 v2.0.4/go.mod h1:0Wqv26UfaUD9n4G6kQubkQ+KchISgw+vpHVxEJEs9eg=
 github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5mhpdKc/us6bOk=
 github.com/googleapis/gax-go/v2 v2.1.0/go.mod h1:Q3nei7sK6ybPYH7twZdmQpAd1MKb7pfu6SK+H1/DsU0=
diff --git a/transport/cert/default_cert.go b/transport/cert/default_cert.go
index 04aefec0afa..18db2babecc 100644
--- a/transport/cert/default_cert.go
+++ b/transport/cert/default_cert.go
@@ -1,4 +1,4 @@
-// Copyright 2020 Google LLC.
+// Copyright 2022 Google LLC.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
@@ -14,32 +14,18 @@ package cert
 
 import (
 	"crypto/tls"
-	"crypto/x509"
-	"encoding/json"
-	"errors"
-	"fmt"
-	"io/ioutil"
-	"os"
-	"os/exec"
-	"os/user"
-	"path/filepath"
 	"sync"
-	"time"
-)
-
-const (
-	metadataPath = ".secureConnect"
-	metadataFile = "context_aware_metadata.json"
 )
 
 // defaultCertData holds all the variables pertaining to
 // the default certficate source created by DefaultSource.
+//
+// A singleton model is used to allow the source to be reused
+// by the transport layer.
 type defaultCertData struct {
-	once            sync.Once
-	source          Source
-	err             error
-	cachedCertMutex sync.Mutex
-	cachedCert      *tls.Certificate
+	once   sync.Once
+	source Source
+	err    error
 }
 
 var (
@@ -49,93 +35,15 @@ var (
 // Source is a function that can be passed into crypto/tls.Config.GetClientCertificate.
 type Source func(*tls.CertificateRequestInfo) (*tls.Certificate, error)
 
-// DefaultSource returns a certificate source that execs the command specified
-// in the file at ~/.secureConnect/context_aware_metadata.json
-//
-// If that file does not exist, a nil source is returned.
+// DefaultSource returns a certificate source using the preferred EnterpriseCertificateProxySource.
+// If EnterpriseCertificateProxySource is not available, fall back to the legacy SecureConnectSource.
+// If neither of these are successful, a nil source is returned.
 func DefaultSource() (Source, error) {
 	defaultCert.once.Do(func() {
-		defaultCert.source, defaultCert.err = newSecureConnectSource()
+		defaultCert.source, defaultCert.err = NewEnterpriseCertificateProxySource("")
+		if defaultCert.source == nil && defaultCert.err == nil {
+			defaultCert.source, defaultCert.err = NewSecureConnectSource("")
+		}
 	})
 	return defaultCert.source, defaultCert.err
 }
-
-type secureConnectSource struct {
-	metadata secureConnectMetadata
-}
-
-type secureConnectMetadata struct {
-	Cmd []string `json:"cert_provider_command"`
-}
-
-// newSecureConnectSource creates a secureConnectSource by reading the well-known file.
-func newSecureConnectSource() (Source, error) {
-	user, err := user.Current()
-	if err != nil {
-		// Ignore.
-		return nil, nil
-	}
-	filename := filepath.Join(user.HomeDir, metadataPath, metadataFile)
-	file, err := ioutil.ReadFile(filename)
-	if os.IsNotExist(err) {
-		// Ignore.
-		return nil, nil
-	}
-	if err != nil {
-		return nil, err
-	}
-
-	var metadata secureConnectMetadata
-	if err := json.Unmarshal(file, &metadata); err != nil {
-		return nil, fmt.Errorf("cert: could not parse JSON in %q: %v", filename, err)
-	}
-	if err := validateMetadata(metadata); err != nil {
-		return nil, fmt.Errorf("cert: invalid config in %q: %v", filename, err)
-	}
-	return (&secureConnectSource{
-		metadata: metadata,
-	}).getClientCertificate, nil
-}
-
-func validateMetadata(metadata secureConnectMetadata) error {
-	if len(metadata.Cmd) == 0 {
-		return errors.New("empty cert_provider_command")
-	}
-	return nil
-}
-
-func (s *secureConnectSource) getClientCertificate(info *tls.CertificateRequestInfo) (*tls.Certificate, error) {
-	defaultCert.cachedCertMutex.Lock()
-	defer defaultCert.cachedCertMutex.Unlock()
-	if defaultCert.cachedCert != nil && !isCertificateExpired(defaultCert.cachedCert) {
-		return defaultCert.cachedCert, nil
-	}
-	// Expand OS environment variables in the cert provider command such as "$HOME".
-	for i := 0; i < len(s.metadata.Cmd); i++ {
-		s.metadata.Cmd[i] = os.ExpandEnv(s.metadata.Cmd[i])
-	}
-	command := s.metadata.Cmd
-	data, err := exec.Command(command[0], command[1:]...).Output()
-	if err != nil {
-		// TODO(cbro): read stderr for error message? Might contain sensitive info.
-		return nil, err
-	}
-	cert, err := tls.X509KeyPair(data, data)
-	if err != nil {
-		return nil, err
-	}
-	defaultCert.cachedCert = &cert
-	return &cert, nil
-}
-
-// isCertificateExpired returns true if the given cert is expired or invalid.
-func isCertificateExpired(cert *tls.Certificate) bool {
-	if len(cert.Certificate) == 0 {
-		return true
-	}
-	parsed, err := x509.ParseCertificate(cert.Certificate[0])
-	if err != nil {
-		return true
-	}
-	return time.Now().After(parsed.NotAfter)
-}
diff --git a/transport/cert/default_cert_test.go b/transport/cert/default_cert_test.go
deleted file mode 100644
index 2d7e333f332..00000000000
--- a/transport/cert/default_cert_test.go
+++ /dev/null
@@ -1,107 +0,0 @@
-// Copyright 2020 Google LLC.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package cert
-
-import (
-	"bytes"
-	"testing"
-)
-
-func TestGetClientCertificateSuccess(t *testing.T) {
-	defaultCert.cachedCert = nil
-	source := secureConnectSource{metadata: secureConnectMetadata{Cmd: []string{"cat", "testdata/testcert.pem"}}}
-	cert, err := source.getClientCertificate(nil)
-	if err != nil {
-		t.Error(err)
-	}
-	if cert.Certificate == nil {
-		t.Error("getClientCertificate: want non-nil Certificate, got nil")
-	}
-	if cert.PrivateKey == nil {
-		t.Error("getClientCertificate: want non-nil PrivateKey, got nil")
-	}
-}
-
-func TestGetClientCertificateFailure(t *testing.T) {
-	defaultCert.cachedCert = nil
-	source := secureConnectSource{metadata: secureConnectMetadata{Cmd: []string{"cat"}}}
-	_, err := source.getClientCertificate(nil)
-	if err == nil {
-		t.Error("Expecting error.")
-	}
-	if got, want := err.Error(), "tls: failed to find any PEM data in certificate input"; got != want {
-		t.Errorf("getClientCertificate: want %v err, got %v", want, got)
-	}
-}
-
-func TestValidateMetadataSuccess(t *testing.T) {
-	metadata := secureConnectMetadata{Cmd: []string{"cat", "testdata/testcert.pem"}}
-	err := validateMetadata(metadata)
-	if err != nil {
-		t.Error(err)
-	}
-}
-
-func TestValidateMetadataFailure(t *testing.T) {
-	metadata := secureConnectMetadata{Cmd: []string{}}
-	err := validateMetadata(metadata)
-	if err == nil {
-		t.Error("validateMetadata: want non-nil err, got nil")
-	}
-	if want, got := "empty cert_provider_command", err.Error(); want != got {
-		t.Errorf("validateMetadata: want %v err, got %v", want, got)
-	}
-}
-
-func TestIsCertificateExpiredTrue(t *testing.T) {
-	defaultCert.cachedCert = nil
-	source := secureConnectSource{metadata: secureConnectMetadata{Cmd: []string{"cat", "testdata/testcert.pem"}}}
-	cert, err := source.getClientCertificate(nil)
-	if err != nil {
-		t.Error(err)
-	}
-	if !isCertificateExpired(cert) {
-		t.Error("isCertificateExpired: want true, got false")
-	}
-}
-
-func TestIsCertificateExpiredFalse(t *testing.T) {
-	defaultCert.cachedCert = nil
-	source := secureConnectSource{metadata: secureConnectMetadata{Cmd: []string{"cat", "testdata/nonexpiringtestcert.pem"}}}
-	cert, err := source.getClientCertificate(nil)
-	if err != nil {
-		t.Error(err)
-	}
-	if isCertificateExpired(cert) {
-		t.Error("isCertificateExpired: want false, got true")
-	}
-}
-
-func TestCertificateCaching(t *testing.T) {
-	defaultCert.cachedCert = nil
-	source := secureConnectSource{metadata: secureConnectMetadata{Cmd: []string{"cat", "testdata/nonexpiringtestcert.pem"}}}
-	cert, err := source.getClientCertificate(nil)
-	if err != nil {
-		t.Error(err)
-	}
-	if cert == nil {
-		t.Error("getClientCertificate: want non-nil cert, got nil")
-	}
-	if defaultCert.cachedCert == nil {
-		t.Error("getClientCertificate: want non-nil defaultSourceCachedCert, got nil")
-	}
-
-	source = secureConnectSource{metadata: secureConnectMetadata{Cmd: []string{"cat", "testdata/testcert.pem"}}}
-	cert, err = source.getClientCertificate(nil)
-	if err != nil {
-		t.Error(err)
-	}
-	if !bytes.Equal(cert.Certificate[0], defaultCert.cachedCert.Certificate[0]) {
-		t.Error("getClientCertificate: want cached Certificate, got different Certificate")
-	}
-	if cert.PrivateKey != defaultCert.cachedCert.PrivateKey {
-		t.Error("getClientCertificate: want cached PrivateKey, got different PrivateKey")
-	}
-}
diff --git a/transport/cert/enterprise_cert.go b/transport/cert/enterprise_cert.go
new file mode 100644
index 00000000000..17d08538cbb
--- /dev/null
+++ b/transport/cert/enterprise_cert.go
@@ -0,0 +1,49 @@
+// Copyright 2022 Google LLC.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+package cert
+
+import (
+	"crypto/tls"
+	"errors"
+	"os"
+
+	"github.com/googleapis/enterprise-certificate-proxy/client"
+)
+
+type ecpSource struct {
+	key *client.Key
+}
+
+// NewEnterpriseCertificateProxySource creates a certificate source
+// using the enterprise-certificate-proxy client, which delegates
+// certifcate related operations to an OS-specific "signer binary"
+// that communicates with the native keystore (ex. keychain on MacOS).
+//
+// The configFilePath points to a config file containing relevant parameters
+// such as the certificate issuer and the location of the signer binary.
+// If configFilePath is empty, the client will attempt to load the config from
+// a well-known gcloud location.
+//
+// Return nil for Source and Error if config file is missing.
+func NewEnterpriseCertificateProxySource(configFilePath string) (Source, error) {
+	key, err := client.Cred(configFilePath)
+	if errors.Is(err, os.ErrNotExist) {
+		// Ignore.
+		return nil, nil
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	return (&ecpSource{
+		key: key,
+	}).getClientCertificate, nil
+}
+
+func (s *ecpSource) getClientCertificate(info *tls.CertificateRequestInfo) (*tls.Certificate, error) {
+	var cert tls.Certificate
+	cert.PrivateKey = s.key
+	cert.Certificate = s.key.CertificateChain()
+	return &cert, nil
+}
diff --git a/transport/cert/enterprise_cert_test.go b/transport/cert/enterprise_cert_test.go
new file mode 100644
index 00000000000..471c5f143ef
--- /dev/null
+++ b/transport/cert/enterprise_cert_test.go
@@ -0,0 +1,47 @@
+// Copyright 2022 Google LLC.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+package cert
+
+import (
+	"testing"
+)
+
+func TestEnterpriseCertificateProxySource_ConfigMissing(t *testing.T) {
+	source, err := NewEnterpriseCertificateProxySource("missing.json")
+	if err != nil {
+		t.Fatal("NewEnterpriseCertificateProxySource: expected nil error returned when config is missing.")
+	}
+	if source != nil {
+		t.Error("NewEnterpriseCertificateProxySource: expected nil source returned when config is missing.")
+	}
+}
+
+// This test launches a mock signer binary "test_signer.go" that uses a valid pem file.
+func TestEnterpriseCertificateProxySource_GetClientCertificateSuccess(t *testing.T) {
+	source, err := NewEnterpriseCertificateProxySource("testdata/enterprise_certificate_config.json")
+	if err != nil {
+		t.Fatal(err)
+	}
+	cert, err := source(nil)
+	if err != nil {
+		t.Error(err)
+	}
+	if cert.Certificate == nil {
+		t.Error("getClientCertificate: want non-nil Certificate, got nil")
+	}
+	if cert.PrivateKey == nil {
+		t.Error("getClientCertificate: want non-nil PrivateKey, got nil")
+	}
+}
+
+// This test launches a mock signer binary "test_signer.go" that uses an invalid pem file.
+func TestEnterpriseCertificateProxySource_InitializationFailure(t *testing.T) {
+	_, err := NewEnterpriseCertificateProxySource("testdata/enterprise_certificate_config_invalid_pem.json")
+	if err == nil {
+		t.Fatal("Expecting error.")
+	}
+	if got, want := err.Error(), "failed to retrieve certificate chain: unexpected EOF"; got != want {
+		t.Errorf("NewEnterpriseCertificateProxySource: want err %v, got %v", want, got)
+	}
+}
diff --git a/transport/cert/internal/test_signer.go b/transport/cert/internal/test_signer.go
new file mode 100644
index 00000000000..0984cdcc390
--- /dev/null
+++ b/transport/cert/internal/test_signer.go
@@ -0,0 +1,100 @@
+// Copyright 2022 Google LLC.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+//
+// test_signer.go is a net/rpc server that listens on stdin/stdout, exposing
+// mock methods for testing enterprise certificate proxy flow.
+package main
+
+import (
+	"crypto"
+	"crypto/tls"
+	"crypto/x509"
+	"io"
+	"io/ioutil"
+	"log"
+	"net/rpc"
+	"os"
+	"time"
+)
+
+type SignArgs struct {
+	Digest []byte
+	Opts   crypto.SignerOpts
+}
+
+// A EnterpriseCertSigner exports RPC methods for signing.
+type EnterpriseCertSigner struct {
+	cert *tls.Certificate
+}
+
+// A Transport wraps a pair of unidirectional streams as an io.ReadWriteCloser.
+type Transport struct {
+	io.ReadCloser
+	io.WriteCloser
+}
+
+// Close closes t's underlying ReadCloser and WriteCloser.
+func (t *Transport) Close() error {
+	rerr := t.ReadCloser.Close()
+	werr := t.WriteCloser.Close()
+	if rerr != nil {
+		return rerr
+	}
+	return werr
+}
+
+// CertificateChain returns the credential as a raw X509 cert chain. This
+// contains the public key.
+func (k *EnterpriseCertSigner) CertificateChain(ignored struct{}, certificateChain *[][]byte) error {
+	*certificateChain = k.cert.Certificate
+	return nil
+}
+
+// Public returns the first public key for this Key, in ASN.1 DER form.
+func (k *EnterpriseCertSigner) Public(ignored struct{}, publicKey *[]byte) (err error) {
+	if len(k.cert.Certificate) == 0 {
+		return nil
+	}
+	cert, err := x509.ParseCertificate(k.cert.Certificate[0])
+	*publicKey, err = x509.MarshalPKIXPublicKey(cert.PublicKey)
+	return nil
+}
+
+// Sign signs a message by encrypting a message digest.
+func (k *EnterpriseCertSigner) Sign(args SignArgs, resp *[]byte) (err error) {
+	return nil
+}
+
+func main() {
+	enterpriseCertSigner := new(EnterpriseCertSigner)
+
+	data, err := ioutil.ReadFile(os.Args[1])
+	if err != nil {
+		log.Fatalf("Error reading certificate: %v", err)
+	}
+	cert, err := tls.X509KeyPair(data, data)
+	if err != nil {
+		log.Fatalf("Error creating X509 certificate: %v", err)
+	}
+
+	enterpriseCertSigner.cert = &cert
+
+	if err := rpc.Register(enterpriseCertSigner); err != nil {
+		log.Fatalf("Error registering net/rpc: %v", err)
+	}
+
+	// If the parent process dies, we should exit.
+	// We can detect this by periodically checking if the PID of the parent
+	// process is 1 (https://stackoverflow.com/a/2035683).
+	go func() {
+		for {
+			if os.Getppid() == 1 {
+				log.Fatalln("Parent process died, exiting...")
+			}
+			time.Sleep(time.Second)
+		}
+	}()
+
+	rpc.ServeConn(&Transport{os.Stdin, os.Stdout})
+}
diff --git a/transport/cert/secureconnect_cert.go b/transport/cert/secureconnect_cert.go
new file mode 100644
index 00000000000..3e7b35c3ad8
--- /dev/null
+++ b/transport/cert/secureconnect_cert.go
@@ -0,0 +1,117 @@
+// Copyright 2020 Google LLC.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+package cert
+
+import (
+	"crypto/tls"
+	"crypto/x509"
+	"encoding/json"
+	"errors"
+	"fmt"
+	"io/ioutil"
+	"os"
+	"os/exec"
+	"os/user"
+	"path/filepath"
+	"sync"
+	"time"
+)
+
+const (
+	metadataPath = ".secureConnect"
+	metadataFile = "context_aware_metadata.json"
+)
+
+type secureConnectSource struct {
+	metadata secureConnectMetadata
+
+	// Cache the cert to avoid executing helper command repeatedly.
+	cachedCertMutex sync.Mutex
+	cachedCert      *tls.Certificate
+}
+
+type secureConnectMetadata struct {
+	Cmd []string `json:"cert_provider_command"`
+}
+
+// NewSecureConnectSource creates a certificate source using
+// the Secure Connect Helper and its associated metadata file.
+//
+// The configFilePath points to the location of the context aware metadata file.
+// If configFilePath is empty, use the default context aware metadata location.
+//
+// Return nil for Source and Error if config file is missing.
+func NewSecureConnectSource(configFilePath string) (Source, error) {
+	if configFilePath == "" {
+		user, err := user.Current()
+		if err != nil {
+			// Ignore.
+			return nil, nil
+		}
+		configFilePath = filepath.Join(user.HomeDir, metadataPath, metadataFile)
+	}
+
+	file, err := ioutil.ReadFile(configFilePath)
+	if errors.Is(err, os.ErrNotExist) {
+		// Ignore.
+		return nil, nil
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	var metadata secureConnectMetadata
+	if err := json.Unmarshal(file, &metadata); err != nil {
+		return nil, fmt.Errorf("cert: could not parse JSON in %q: %v", configFilePath, err)
+	}
+	if err := validateMetadata(metadata); err != nil {
+		return nil, fmt.Errorf("cert: invalid config in %q: %v", configFilePath, err)
+	}
+	return (&secureConnectSource{
+		metadata: metadata,
+	}).getClientCertificate, nil
+}
+
+func validateMetadata(metadata secureConnectMetadata) error {
+	if len(metadata.Cmd) == 0 {
+		return errors.New("empty cert_provider_command")
+	}
+	return nil
+}
+
+func (s *secureConnectSource) getClientCertificate(info *tls.CertificateRequestInfo) (*tls.Certificate, error) {
+	s.cachedCertMutex.Lock()
+	defer s.cachedCertMutex.Unlock()
+	if s.cachedCert != nil && !isCertificateExpired(s.cachedCert) {
+		return s.cachedCert, nil
+	}
+	// Expand OS environment variables in the cert provider command such as "$HOME".
+	for i := 0; i < len(s.metadata.Cmd); i++ {
+		s.metadata.Cmd[i] = os.ExpandEnv(s.metadata.Cmd[i])
+	}
+	command := s.metadata.Cmd
+	data, err := exec.Command(command[0], command[1:]...).Output()
+	if err != nil {
+		// TODO(cbro): read stderr for error message? Might contain sensitive info.
+		return nil, err
+	}
+	cert, err := tls.X509KeyPair(data, data)
+	if err != nil {
+		return nil, err
+	}
+	s.cachedCert = &cert
+	return &cert, nil
+}
+
+// isCertificateExpired returns true if the given cert is expired or invalid.
+func isCertificateExpired(cert *tls.Certificate) bool {
+	if len(cert.Certificate) == 0 {
+		return true
+	}
+	parsed, err := x509.ParseCertificate(cert.Certificate[0])
+	if err != nil {
+		return true
+	}
+	return time.Now().After(parsed.NotAfter)
+}
diff --git a/transport/cert/secureconnect_cert_test.go b/transport/cert/secureconnect_cert_test.go
new file mode 100644
index 00000000000..e83d337e859
--- /dev/null
+++ b/transport/cert/secureconnect_cert_test.go
@@ -0,0 +1,118 @@
+// Copyright 2020 Google LLC.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package cert
+
+import (
+	"bytes"
+	"testing"
+)
+
+func TestSecureConnectSource_ConfigMissing(t *testing.T) {
+	source, err := NewSecureConnectSource("missing.json")
+	if err != nil {
+		t.Fatal("NewSecureConnectSource: expected nil error returned when config is missing.")
+	}
+	if source != nil {
+		t.Error("NewSecureConnectSource: expected nil source and error returned when config is missing.")
+	}
+}
+
+func TestSecureConnectSource_GetClientCertificateSuccess(t *testing.T) {
+	source, err := NewSecureConnectSource("testdata/context_aware_metadata.json")
+	if err != nil {
+		t.Fatal(err)
+	}
+	cert, err := source(nil)
+	if err != nil {
+		t.Error(err)
+	}
+	if cert.Certificate == nil {
+		t.Error("getClientCertificate: want non-nil Certificate, got nil")
+	}
+	if cert.PrivateKey == nil {
+		t.Error("getClientCertificate: want non-nil PrivateKey, got nil")
+	}
+}
+
+func TestSecureConnectSource_GetClientCertificateFailure(t *testing.T) {
+	source, err := NewSecureConnectSource("testdata/context_aware_metadata_invalid_pem.json")
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, err = source(nil)
+	if err == nil {
+		t.Error("Expecting error.")
+	}
+	if got, want := err.Error(), "x509: malformed certificate"; got != want {
+		t.Errorf("getClientCertificate: want %v err, got %v", want, got)
+	}
+}
+
+func TestSecureConnectSource_ValidateMetadataSuccess(t *testing.T) {
+	metadata := secureConnectMetadata{Cmd: []string{"cat", "testdata/testcert.pem"}}
+	err := validateMetadata(metadata)
+	if err != nil {
+		t.Error(err)
+	}
+}
+
+func TestSecureConnectSource_ValidateMetadataFailure(t *testing.T) {
+	metadata := secureConnectMetadata{Cmd: []string{}}
+	err := validateMetadata(metadata)
+	if err == nil {
+		t.Error("validateMetadata: want non-nil err, got nil")
+	}
+	if want, got := "empty cert_provider_command", err.Error(); want != got {
+		t.Errorf("validateMetadata: want %v err, got %v", want, got)
+	}
+}
+
+func TestSecureConnectSource_IsCertificateExpiredTrue(t *testing.T) {
+	source, err := NewSecureConnectSource("testdata/context_aware_metadata.json")
+	if err != nil {
+		t.Fatal(err)
+	}
+	cert, err := source(nil)
+	if err != nil {
+		t.Error(err)
+	}
+	if !isCertificateExpired(cert) {
+		t.Error("isCertificateExpired: want true, got false")
+	}
+}
+
+func TestSecureConnectSource_IsCertificateExpiredFalse(t *testing.T) {
+	source, err := NewSecureConnectSource("testdata/context_aware_metadata_nonexpiring_pem.json")
+	if err != nil {
+		t.Fatal(err)
+	}
+	cert, err := source(nil)
+	if err != nil {
+		t.Error(err)
+	}
+	if isCertificateExpired(cert) {
+		t.Error("isCertificateExpired: want false, got true")
+	}
+}
+
+func TestCertificateCaching(t *testing.T) {
+	source := secureConnectSource{metadata: secureConnectMetadata{Cmd: []string{"cat", "testdata/nonexpiring.pem"}}}
+	cert, err := source.getClientCertificate(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if cert == nil {
+		t.Error("getClientCertificate: want non-nil cert, got nil")
+	}
+	if source.cachedCert == nil {
+		t.Error("getClientCertificate: want non-nil cachedCert, got nil")
+	}
+	if !bytes.Equal(cert.Certificate[0], source.cachedCert.Certificate[0]) {
+		t.Error("Cached certificate is different.")
+	}
+	if cert.PrivateKey != source.cachedCert.PrivateKey {
+		t.Error("Cached PrivateKey is different.")
+	}
+}
diff --git a/transport/cert/testdata/context_aware_metadata.json b/transport/cert/testdata/context_aware_metadata.json
new file mode 100644
index 00000000000..19b700eac90
--- /dev/null
+++ b/transport/cert/testdata/context_aware_metadata.json
@@ -0,0 +1,3 @@
+{
+	"cert_provider_command":["cat", "testdata/testcert.pem"]
+}
\ No newline at end of file
diff --git a/transport/cert/testdata/context_aware_metadata_invalid_pem.json b/transport/cert/testdata/context_aware_metadata_invalid_pem.json
new file mode 100644
index 00000000000..bc7727ea720
--- /dev/null
+++ b/transport/cert/testdata/context_aware_metadata_invalid_pem.json
@@ -0,0 +1,3 @@
+{
+	"cert_provider_command":["cat", "testdata/invalid.pem"]
+}
\ No newline at end of file
diff --git a/transport/cert/testdata/context_aware_metadata_nonexpiring_pem.json b/transport/cert/testdata/context_aware_metadata_nonexpiring_pem.json
new file mode 100644
index 00000000000..4a073c1809e
--- /dev/null
+++ b/transport/cert/testdata/context_aware_metadata_nonexpiring_pem.json
@@ -0,0 +1,3 @@
+{
+	"cert_provider_command":["cat", "testdata/nonexpiring.pem"]
+}
\ No newline at end of file
diff --git a/transport/cert/testdata/enterprise_certificate_config.json b/transport/cert/testdata/enterprise_certificate_config.json
new file mode 100644
index 00000000000..be9f9a3e806
--- /dev/null
+++ b/transport/cert/testdata/enterprise_certificate_config.json
@@ -0,0 +1,8 @@
+{
+  "cert_info": {
+    "issuer": "Test Issuer"
+  },
+  "libs": {
+    "signer_binary": "./testdata/signer.sh"
+  }
+}
diff --git a/transport/cert/testdata/enterprise_certificate_config_invalid_pem.json b/transport/cert/testdata/enterprise_certificate_config_invalid_pem.json
new file mode 100644
index 00000000000..5fc2dcfc8f9
--- /dev/null
+++ b/transport/cert/testdata/enterprise_certificate_config_invalid_pem.json
@@ -0,0 +1,8 @@
+{
+  "cert_info": {
+    "issuer": "Test Issuer"
+  },
+  "libs": {
+    "signer_binary": "./testdata/signer_invalid_pem.sh"
+  }
+}
diff --git a/transport/cert/testdata/invalid.pem b/transport/cert/testdata/invalid.pem
new file mode 100644
index 00000000000..be8c6472488
--- /dev/null
+++ b/transport/cert/testdata/invalid.pem
@@ -0,0 +1,6 @@
+-----BEGIN CERTIFICATE-----
+MIIB0zCCAX2gAwIBAgIJAI/M7BYjwB+uMA0GCSqGSIb3DQEBBQUAMEUxCzAJBgNV
+-----END CERTIFICATE-----
+-----BEGIN PRIVATE KEY-----
+MIIBOwIBAAJBANLJhPHhITqQbPklG3ibCVxwGMRfp/v4XqhfdQHdcVfHap6NQ5Wo
+-----END PRIVATE KEY-----
\ No newline at end of file
diff --git a/transport/cert/testdata/nonexpiringtestcert.pem b/transport/cert/testdata/nonexpiring.pem
similarity index 100%
rename from transport/cert/testdata/nonexpiringtestcert.pem
rename to transport/cert/testdata/nonexpiring.pem
diff --git a/transport/cert/testdata/signer.sh b/transport/cert/testdata/signer.sh
new file mode 100755
index 00000000000..9a023aee14b
--- /dev/null
+++ b/transport/cert/testdata/signer.sh
@@ -0,0 +1,7 @@
+#!/bin/bash  
+
+# Copyright 2022 Google LLC.
+# Use of this source code is governed by a BSD-style
+# license that can be found in the LICENSE file.
+
+go run ./internal/test_signer.go testdata/testcert.pem
\ No newline at end of file
diff --git a/transport/cert/testdata/signer_invalid_pem.sh b/transport/cert/testdata/signer_invalid_pem.sh
new file mode 100755
index 00000000000..abf2952154f
--- /dev/null
+++ b/transport/cert/testdata/signer_invalid_pem.sh
@@ -0,0 +1,7 @@
+#!/bin/bash  
+
+# Copyright 2022 Google LLC.
+# Use of this source code is governed by a BSD-style
+# license that can be found in the LICENSE file.
+
+go run ./internal/test_signer.go testdata/invalid.pem
\ No newline at end of file

From a3609806092ad18babaf53f2d954b882c0387d25 Mon Sep 17 00:00:00 2001
From: Andy Zhao <andyzhao@google.com>
Date: Fri, 3 Jun 2022 16:58:01 -0700
Subject: [PATCH 04/11] feat(transport): Minor comment updates

---
 transport/cert/default_cert.go            | 4 +++-
 transport/cert/secureconnect_cert.go      | 2 +-
 transport/cert/secureconnect_cert_test.go | 3 +--
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/transport/cert/default_cert.go b/transport/cert/default_cert.go
index 18db2babecc..3caaf120a09 100644
--- a/transport/cert/default_cert.go
+++ b/transport/cert/default_cert.go
@@ -37,7 +37,9 @@ type Source func(*tls.CertificateRequestInfo) (*tls.Certificate, error)
 
 // DefaultSource returns a certificate source using the preferred EnterpriseCertificateProxySource.
 // If EnterpriseCertificateProxySource is not available, fall back to the legacy SecureConnectSource.
-// If neither of these are successful, a nil source is returned.
+//
+// If neither source is available (due to missing configurations), a nil Source and a nil Error are
+// returned to indicate that a default certificate source is unavailable.
 func DefaultSource() (Source, error) {
 	defaultCert.once.Do(func() {
 		defaultCert.source, defaultCert.err = NewEnterpriseCertificateProxySource("")
diff --git a/transport/cert/secureconnect_cert.go b/transport/cert/secureconnect_cert.go
index 3e7b35c3ad8..f157b2f5489 100644
--- a/transport/cert/secureconnect_cert.go
+++ b/transport/cert/secureconnect_cert.go
@@ -1,4 +1,4 @@
-// Copyright 2020 Google LLC.
+// Copyright 2022 Google LLC.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 package cert
diff --git a/transport/cert/secureconnect_cert_test.go b/transport/cert/secureconnect_cert_test.go
index e83d337e859..1b934f010d6 100644
--- a/transport/cert/secureconnect_cert_test.go
+++ b/transport/cert/secureconnect_cert_test.go
@@ -1,7 +1,6 @@
-// Copyright 2020 Google LLC.
+// Copyright 2022 Google LLC.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
-
 package cert
 
 import (

From b608fd17288151be48e2609da1e0d458f30863d5 Mon Sep 17 00:00:00 2001
From: Andy Zhao <andyzhao@google.com>
Date: Sun, 5 Jun 2022 21:37:32 -0700
Subject: [PATCH 05/11] feat(transport): Minor test update

---
 transport/cert/secureconnect_cert_test.go | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/transport/cert/secureconnect_cert_test.go b/transport/cert/secureconnect_cert_test.go
index 1b934f010d6..c7457c05b1e 100644
--- a/transport/cert/secureconnect_cert_test.go
+++ b/transport/cert/secureconnect_cert_test.go
@@ -44,9 +44,6 @@ func TestSecureConnectSource_GetClientCertificateFailure(t *testing.T) {
 	if err == nil {
 		t.Error("Expecting error.")
 	}
-	if got, want := err.Error(), "x509: malformed certificate"; got != want {
-		t.Errorf("getClientCertificate: want %v err, got %v", want, got)
-	}
 }
 
 func TestSecureConnectSource_ValidateMetadataSuccess(t *testing.T) {

From 8fc15d438b1acde6b9f246f9af389696809c5057 Mon Sep 17 00:00:00 2001
From: Andy Zhao <andyzhao@google.com>
Date: Mon, 6 Jun 2022 09:50:43 -0700
Subject: [PATCH 06/11] feat(transport): Go mod tidy

---
 go.mod | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/go.mod b/go.mod
index 48885b7d9be..fd17f307229 100644
--- a/go.mod
+++ b/go.mod
@@ -5,7 +5,7 @@ go 1.15
 require (
 	cloud.google.com/go/compute v1.6.1
 	github.com/google/go-cmp v0.5.8
-	github.com/googleapis/enterprise-certificate-proxy v0.0.0-20220520183353-fd19c99a87aa // indirect
+	github.com/googleapis/enterprise-certificate-proxy v0.0.0-20220520183353-fd19c99a87aa
 	github.com/googleapis/gax-go/v2 v2.4.0
 	go.opencensus.io v0.23.0
 	golang.org/x/net v0.0.0-20220526153639-5463443f8c37

From 030d395b39c5044692bc7de7f8a54bb0640f8dc0 Mon Sep 17 00:00:00 2001
From: Andy Zhao <andyzhao@google.com>
Date: Mon, 6 Jun 2022 10:52:17 -0700
Subject: [PATCH 07/11] feat(transport): Update comments

---
 transport/cert/enterprise_cert.go      | 17 ++++++++++++++---
 transport/cert/internal/test_signer.go |  5 +++--
 transport/cert/secureconnect_cert.go   | 19 +++++++++++++++----
 3 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/transport/cert/enterprise_cert.go b/transport/cert/enterprise_cert.go
index 17d08538cbb..5422abd3591 100644
--- a/transport/cert/enterprise_cert.go
+++ b/transport/cert/enterprise_cert.go
@@ -1,6 +1,15 @@
 // Copyright 2022 Google LLC.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
+
+// Package cert contains certificate tools for Google API clients.
+// This package is intended to be used with crypto/tls.Config.GetClientCertificate.
+//
+// The certificates can be used to satisfy Google's Endpoint Validation.
+// See https://cloud.google.com/endpoint-verification/docs/overview
+//
+// This package is not intended for use by end developers. Use the
+// google.golang.org/api/option package to configure API clients.
 package cert
 
 import (
@@ -16,7 +25,7 @@ type ecpSource struct {
 }
 
 // NewEnterpriseCertificateProxySource creates a certificate source
-// using the enterprise-certificate-proxy client, which delegates
+// using the Enterprise Certificate Proxy client, which delegates
 // certifcate related operations to an OS-specific "signer binary"
 // that communicates with the native keystore (ex. keychain on MacOS).
 //
@@ -25,11 +34,13 @@ type ecpSource struct {
 // If configFilePath is empty, the client will attempt to load the config from
 // a well-known gcloud location.
 //
-// Return nil for Source and Error if config file is missing.
+// Return nil for Source and Error if config file is missing, which
+// means Enterprise Certificate Proxy is not supported.
 func NewEnterpriseCertificateProxySource(configFilePath string) (Source, error) {
 	key, err := client.Cred(configFilePath)
 	if errors.Is(err, os.ErrNotExist) {
-		// Ignore.
+		// Config file missing means Enterprise Certificate Proxy is not supported,
+		// so this is not a real error.
 		return nil, nil
 	}
 	if err != nil {
diff --git a/transport/cert/internal/test_signer.go b/transport/cert/internal/test_signer.go
index 0984cdcc390..6b253bfe4e6 100644
--- a/transport/cert/internal/test_signer.go
+++ b/transport/cert/internal/test_signer.go
@@ -18,17 +18,18 @@ import (
 	"time"
 )
 
+// SignArgs encapsulate the parameters for the Sign method.
 type SignArgs struct {
 	Digest []byte
 	Opts   crypto.SignerOpts
 }
 
-// A EnterpriseCertSigner exports RPC methods for signing.
+// EnterpriseCertSigner exports RPC methods for signing.
 type EnterpriseCertSigner struct {
 	cert *tls.Certificate
 }
 
-// A Transport wraps a pair of unidirectional streams as an io.ReadWriteCloser.
+// Transport wraps a pair of unidirectional streams as an io.ReadWriteCloser.
 type Transport struct {
 	io.ReadCloser
 	io.WriteCloser
diff --git a/transport/cert/secureconnect_cert.go b/transport/cert/secureconnect_cert.go
index f157b2f5489..adb8a7570ee 100644
--- a/transport/cert/secureconnect_cert.go
+++ b/transport/cert/secureconnect_cert.go
@@ -1,6 +1,15 @@
 // Copyright 2022 Google LLC.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
+
+// Package cert contains certificate tools for Google API clients.
+// This package is intended to be used with crypto/tls.Config.GetClientCertificate.
+//
+// The certificates can be used to satisfy Google's Endpoint Validation.
+// See https://cloud.google.com/endpoint-verification/docs/overview
+//
+// This package is not intended for use by end developers. Use the
+// google.golang.org/api/option package to configure API clients.
 package cert
 
 import (
@@ -41,12 +50,14 @@ type secureConnectMetadata struct {
 // The configFilePath points to the location of the context aware metadata file.
 // If configFilePath is empty, use the default context aware metadata location.
 //
-// Return nil for Source and Error if config file is missing.
+// Return nil for Source and Error if config file is missing, which
+// means Secure Connect is not supported.
 func NewSecureConnectSource(configFilePath string) (Source, error) {
 	if configFilePath == "" {
 		user, err := user.Current()
 		if err != nil {
-			// Ignore.
+			// Error locating the default config means Secure Connect is not supported,
+			// so this is not a real error.
 			return nil, nil
 		}
 		configFilePath = filepath.Join(user.HomeDir, metadataPath, metadataFile)
@@ -54,7 +65,8 @@ func NewSecureConnectSource(configFilePath string) (Source, error) {
 
 	file, err := ioutil.ReadFile(configFilePath)
 	if errors.Is(err, os.ErrNotExist) {
-		// Ignore.
+		// Config file missing means Secure Connect is not supported,
+		// so this is not a real error.
 		return nil, nil
 	}
 	if err != nil {
@@ -93,7 +105,6 @@ func (s *secureConnectSource) getClientCertificate(info *tls.CertificateRequestI
 	command := s.metadata.Cmd
 	data, err := exec.Command(command[0], command[1:]...).Output()
 	if err != nil {
-		// TODO(cbro): read stderr for error message? Might contain sensitive info.
 		return nil, err
 	}
 	cert, err := tls.X509KeyPair(data, data)

From 30bc6f300dfd6a80282dd0e2b13d046306d0446d Mon Sep 17 00:00:00 2001
From: Andy Zhao <andyzhao@google.com>
Date: Mon, 6 Jun 2022 11:11:49 -0700
Subject: [PATCH 08/11] feat(transport): Update tests

---
 transport/cert/internal/test_signer.go    | 5 ++++-
 transport/cert/secureconnect_cert_test.go | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/transport/cert/internal/test_signer.go b/transport/cert/internal/test_signer.go
index 6b253bfe4e6..d40f851593b 100644
--- a/transport/cert/internal/test_signer.go
+++ b/transport/cert/internal/test_signer.go
@@ -58,8 +58,11 @@ func (k *EnterpriseCertSigner) Public(ignored struct{}, publicKey *[]byte) (err
 		return nil
 	}
 	cert, err := x509.ParseCertificate(k.cert.Certificate[0])
+	if err != nil {
+		return err
+	}
 	*publicKey, err = x509.MarshalPKIXPublicKey(cert.PublicKey)
-	return nil
+	return err
 }
 
 // Sign signs a message by encrypting a message digest.
diff --git a/transport/cert/secureconnect_cert_test.go b/transport/cert/secureconnect_cert_test.go
index c7457c05b1e..b1ddc4a8f4f 100644
--- a/transport/cert/secureconnect_cert_test.go
+++ b/transport/cert/secureconnect_cert_test.go
@@ -100,10 +100,10 @@ func TestCertificateCaching(t *testing.T) {
 		t.Fatal(err)
 	}
 	if cert == nil {
-		t.Error("getClientCertificate: want non-nil cert, got nil")
+		t.Fatal("getClientCertificate: want non-nil cert, got nil")
 	}
 	if source.cachedCert == nil {
-		t.Error("getClientCertificate: want non-nil cachedCert, got nil")
+		t.Fatal("getClientCertificate: want non-nil cachedCert, got nil")
 	}
 	if !bytes.Equal(cert.Certificate[0], source.cachedCert.Certificate[0]) {
 		t.Error("Cached certificate is different.")

From 84dad39f91689c88fddf81b0d3c38bf4b4a164b2 Mon Sep 17 00:00:00 2001
From: Andy Zhao <andyzhao@google.com>
Date: Tue, 7 Jun 2022 22:37:21 -0700
Subject: [PATCH 09/11] feat(transport): Address PR comments

---
 transport/cert/default_cert.go                |  11 ++++++++--
 transport/cert/enterprise_cert.go             |   8 ++-----
 transport/cert/enterprise_cert_test.go        |   9 ++++----
 transport/cert/secureconnect_cert.go          |  17 ++++++---------
 transport/cert/secureconnect_cert_test.go     |   7 +++---
 .../cert/testdata/context_aware_metadata.json |   2 +-
 .../context_aware_metadata_invalid_pem.json   |   2 +-
 ...ontext_aware_metadata_nonexpiring_pem.json |   2 +-
 transport/cert/testdata/invalid.pem           |   2 +-
 transport/cert/testdata/signer.sh             |   2 +-
 transport/cert/testdata/signer_invalid_pem.sh |   2 +-
 transport/cert/testdata/testcert.pem          |   2 +-
 transport/internal/ecp/.DS_Store              | Bin 0 -> 6148 bytes
 .../internal => internal/ecp}/test_signer.go  |  20 +++++++++---------
 14 files changed, 43 insertions(+), 43 deletions(-)
 create mode 100644 transport/internal/ecp/.DS_Store
 rename transport/{cert/internal => internal/ecp}/test_signer.go (81%)

diff --git a/transport/cert/default_cert.go b/transport/cert/default_cert.go
index 3caaf120a09..21d0251531c 100644
--- a/transport/cert/default_cert.go
+++ b/transport/cert/default_cert.go
@@ -1,4 +1,4 @@
-// Copyright 2022 Google LLC.
+// Copyright 2020 Google LLC.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
@@ -14,6 +14,7 @@ package cert
 
 import (
 	"crypto/tls"
+	"errors"
 	"sync"
 )
 
@@ -35,6 +36,9 @@ var (
 // Source is a function that can be passed into crypto/tls.Config.GetClientCertificate.
 type Source func(*tls.CertificateRequestInfo) (*tls.Certificate, error)
 
+// errSourceUnavailable is a sentinel error to indicate certificate source is unavailable.
+var errSourceUnavailable = errors.New("certificate source is unavailable")
+
 // DefaultSource returns a certificate source using the preferred EnterpriseCertificateProxySource.
 // If EnterpriseCertificateProxySource is not available, fall back to the legacy SecureConnectSource.
 //
@@ -43,8 +47,11 @@ type Source func(*tls.CertificateRequestInfo) (*tls.Certificate, error)
 func DefaultSource() (Source, error) {
 	defaultCert.once.Do(func() {
 		defaultCert.source, defaultCert.err = NewEnterpriseCertificateProxySource("")
-		if defaultCert.source == nil && defaultCert.err == nil {
+		if errors.Is(defaultCert.err, errSourceUnavailable) {
 			defaultCert.source, defaultCert.err = NewSecureConnectSource("")
+			if errors.Is(defaultCert.err, errSourceUnavailable) {
+				defaultCert.source, defaultCert.err = nil, nil
+			}
 		}
 	})
 	return defaultCert.source, defaultCert.err
diff --git a/transport/cert/enterprise_cert.go b/transport/cert/enterprise_cert.go
index 5422abd3591..41d9e8ea92e 100644
--- a/transport/cert/enterprise_cert.go
+++ b/transport/cert/enterprise_cert.go
@@ -33,15 +33,11 @@ type ecpSource struct {
 // such as the certificate issuer and the location of the signer binary.
 // If configFilePath is empty, the client will attempt to load the config from
 // a well-known gcloud location.
-//
-// Return nil for Source and Error if config file is missing, which
-// means Enterprise Certificate Proxy is not supported.
 func NewEnterpriseCertificateProxySource(configFilePath string) (Source, error) {
 	key, err := client.Cred(configFilePath)
 	if errors.Is(err, os.ErrNotExist) {
-		// Config file missing means Enterprise Certificate Proxy is not supported,
-		// so this is not a real error.
-		return nil, nil
+		// Config file missing means Enterprise Certificate Proxy is not supported.
+		return nil, errSourceUnavailable
 	}
 	if err != nil {
 		return nil, err
diff --git a/transport/cert/enterprise_cert_test.go b/transport/cert/enterprise_cert_test.go
index 471c5f143ef..9563a3d0d32 100644
--- a/transport/cert/enterprise_cert_test.go
+++ b/transport/cert/enterprise_cert_test.go
@@ -4,13 +4,14 @@
 package cert
 
 import (
+	"errors"
 	"testing"
 )
 
 func TestEnterpriseCertificateProxySource_ConfigMissing(t *testing.T) {
 	source, err := NewEnterpriseCertificateProxySource("missing.json")
-	if err != nil {
-		t.Fatal("NewEnterpriseCertificateProxySource: expected nil error returned when config is missing.")
+	if !errors.Is(err, errSourceUnavailable) {
+		t.Fatal("NewEnterpriseCertificateProxySource: expected errSourceUnavailable returned when config is missing.")
 	}
 	if source != nil {
 		t.Error("NewEnterpriseCertificateProxySource: expected nil source returned when config is missing.")
@@ -25,7 +26,7 @@ func TestEnterpriseCertificateProxySource_GetClientCertificateSuccess(t *testing
 	}
 	cert, err := source(nil)
 	if err != nil {
-		t.Error(err)
+		t.Fatal(err)
 	}
 	if cert.Certificate == nil {
 		t.Error("getClientCertificate: want non-nil Certificate, got nil")
@@ -41,7 +42,7 @@ func TestEnterpriseCertificateProxySource_InitializationFailure(t *testing.T) {
 	if err == nil {
 		t.Fatal("Expecting error.")
 	}
-	if got, want := err.Error(), "failed to retrieve certificate chain: unexpected EOF"; got != want {
+	if got, want := err.Error(), "failed to parse public key: asn1: syntax error: sequence truncated"; got != want {
 		t.Errorf("NewEnterpriseCertificateProxySource: want err %v, got %v", want, got)
 	}
 }
diff --git a/transport/cert/secureconnect_cert.go b/transport/cert/secureconnect_cert.go
index adb8a7570ee..401a705216a 100644
--- a/transport/cert/secureconnect_cert.go
+++ b/transport/cert/secureconnect_cert.go
@@ -49,25 +49,20 @@ type secureConnectMetadata struct {
 //
 // The configFilePath points to the location of the context aware metadata file.
 // If configFilePath is empty, use the default context aware metadata location.
-//
-// Return nil for Source and Error if config file is missing, which
-// means Secure Connect is not supported.
 func NewSecureConnectSource(configFilePath string) (Source, error) {
 	if configFilePath == "" {
 		user, err := user.Current()
 		if err != nil {
-			// Error locating the default config means Secure Connect is not supported,
-			// so this is not a real error.
-			return nil, nil
+			// Error locating the default config means Secure Connect is not supported.
+			return nil, errSourceUnavailable
 		}
 		configFilePath = filepath.Join(user.HomeDir, metadataPath, metadataFile)
 	}
 
 	file, err := ioutil.ReadFile(configFilePath)
 	if errors.Is(err, os.ErrNotExist) {
-		// Config file missing means Secure Connect is not supported,
-		// so this is not a real error.
-		return nil, nil
+		// Config file missing means Secure Connect is not supported.
+		return nil, errSourceUnavailable
 	}
 	if err != nil {
 		return nil, err
@@ -75,10 +70,10 @@ func NewSecureConnectSource(configFilePath string) (Source, error) {
 
 	var metadata secureConnectMetadata
 	if err := json.Unmarshal(file, &metadata); err != nil {
-		return nil, fmt.Errorf("cert: could not parse JSON in %q: %v", configFilePath, err)
+		return nil, fmt.Errorf("cert: could not parse JSON in %q: %w", configFilePath, err)
 	}
 	if err := validateMetadata(metadata); err != nil {
-		return nil, fmt.Errorf("cert: invalid config in %q: %v", configFilePath, err)
+		return nil, fmt.Errorf("cert: invalid config in %q: %w", configFilePath, err)
 	}
 	return (&secureConnectSource{
 		metadata: metadata,
diff --git a/transport/cert/secureconnect_cert_test.go b/transport/cert/secureconnect_cert_test.go
index b1ddc4a8f4f..0117a0a5496 100644
--- a/transport/cert/secureconnect_cert_test.go
+++ b/transport/cert/secureconnect_cert_test.go
@@ -5,16 +5,17 @@ package cert
 
 import (
 	"bytes"
+	"errors"
 	"testing"
 )
 
 func TestSecureConnectSource_ConfigMissing(t *testing.T) {
 	source, err := NewSecureConnectSource("missing.json")
-	if err != nil {
-		t.Fatal("NewSecureConnectSource: expected nil error returned when config is missing.")
+	if !errors.Is(err, errSourceUnavailable) {
+		t.Fatal("NewSecureConnectSource: expected errSourceUnavailable returned when config is missing.")
 	}
 	if source != nil {
-		t.Error("NewSecureConnectSource: expected nil source and error returned when config is missing.")
+		t.Error("NewSecureConnectSource: expected nil source returned when config is missing.")
 	}
 }
 
diff --git a/transport/cert/testdata/context_aware_metadata.json b/transport/cert/testdata/context_aware_metadata.json
index 19b700eac90..fbfdfb14272 100644
--- a/transport/cert/testdata/context_aware_metadata.json
+++ b/transport/cert/testdata/context_aware_metadata.json
@@ -1,3 +1,3 @@
 {
 	"cert_provider_command":["cat", "testdata/testcert.pem"]
-}
\ No newline at end of file
+}
diff --git a/transport/cert/testdata/context_aware_metadata_invalid_pem.json b/transport/cert/testdata/context_aware_metadata_invalid_pem.json
index bc7727ea720..d297c2135be 100644
--- a/transport/cert/testdata/context_aware_metadata_invalid_pem.json
+++ b/transport/cert/testdata/context_aware_metadata_invalid_pem.json
@@ -1,3 +1,3 @@
 {
 	"cert_provider_command":["cat", "testdata/invalid.pem"]
-}
\ No newline at end of file
+}
diff --git a/transport/cert/testdata/context_aware_metadata_nonexpiring_pem.json b/transport/cert/testdata/context_aware_metadata_nonexpiring_pem.json
index 4a073c1809e..5876cd34437 100644
--- a/transport/cert/testdata/context_aware_metadata_nonexpiring_pem.json
+++ b/transport/cert/testdata/context_aware_metadata_nonexpiring_pem.json
@@ -1,3 +1,3 @@
 {
 	"cert_provider_command":["cat", "testdata/nonexpiring.pem"]
-}
\ No newline at end of file
+}
diff --git a/transport/cert/testdata/invalid.pem b/transport/cert/testdata/invalid.pem
index be8c6472488..032255fbbdc 100644
--- a/transport/cert/testdata/invalid.pem
+++ b/transport/cert/testdata/invalid.pem
@@ -3,4 +3,4 @@ MIIB0zCCAX2gAwIBAgIJAI/M7BYjwB+uMA0GCSqGSIb3DQEBBQUAMEUxCzAJBgNV
 -----END CERTIFICATE-----
 -----BEGIN PRIVATE KEY-----
 MIIBOwIBAAJBANLJhPHhITqQbPklG3ibCVxwGMRfp/v4XqhfdQHdcVfHap6NQ5Wo
------END PRIVATE KEY-----
\ No newline at end of file
+-----END PRIVATE KEY-----
diff --git a/transport/cert/testdata/signer.sh b/transport/cert/testdata/signer.sh
index 9a023aee14b..8a7b2192526 100755
--- a/transport/cert/testdata/signer.sh
+++ b/transport/cert/testdata/signer.sh
@@ -4,4 +4,4 @@
 # Use of this source code is governed by a BSD-style
 # license that can be found in the LICENSE file.
 
-go run ./internal/test_signer.go testdata/testcert.pem
\ No newline at end of file
+go run ../internal/ecp/test_signer.go testdata/testcert.pem
diff --git a/transport/cert/testdata/signer_invalid_pem.sh b/transport/cert/testdata/signer_invalid_pem.sh
index abf2952154f..f97fb1489f9 100755
--- a/transport/cert/testdata/signer_invalid_pem.sh
+++ b/transport/cert/testdata/signer_invalid_pem.sh
@@ -4,4 +4,4 @@
 # Use of this source code is governed by a BSD-style
 # license that can be found in the LICENSE file.
 
-go run ./internal/test_signer.go testdata/invalid.pem
\ No newline at end of file
+go run ../internal/ecp/test_signer.go testdata/invalid.pem
diff --git a/transport/cert/testdata/testcert.pem b/transport/cert/testdata/testcert.pem
index d15c396ba18..3f45e909126 100644
--- a/transport/cert/testdata/testcert.pem
+++ b/transport/cert/testdata/testcert.pem
@@ -18,4 +18,4 @@ MQIhAPW+eyZo7ay3lMz1V01WVjNKK9QSn1MJlb06h/LuYv9FAiEA25WPedKgVyCW
 SmUwbPw8fnTcpqDWE3yTO3vKcebqMSsCIBF3UmVue8YU3jybC3NxuXq3wNm34R8T
 xVLHwDXh/6NJAiEAl2oHGGLz64BuAfjKrqwz7qMYr9HCLIe/YsoWq/olzScCIQDi
 D2lWusoe2/nEqfDVVWGWlyJ7yOmqaVm/iNUN9B2N2g==
------END PRIVATE KEY-----
\ No newline at end of file
+-----END PRIVATE KEY-----
diff --git a/transport/internal/ecp/.DS_Store b/transport/internal/ecp/.DS_Store
new file mode 100644
index 0000000000000000000000000000000000000000..440a2760323160c0b5dee4da6c9996d2e7b36e81
GIT binary patch
literal 6148
zcmeHK!Ait15S`Hq1uuK_n4{PJL0sw=><>t{qC!g+cD?7nybAu7Z!%+P+2TzRnSta@
zW-^oJL6Zy-@%FWEh&Du2qY1JoD<bAq*M&{K0<z@jkNcrZX;?|}S55Nm6`kmjj&z!T
z|70AVn%zE)huID$-<aCA8&U^fo!&lPn%nogdBQRE-}(>FXSrv~xatfz1I~am;0*i(
z1GuwA>T5;sodIXS8JHN5^C6%KM#HRFjt+FC1OQer7lAIdgv12HXqXk@fv|=GHI%Ky
zU=4>o*t}?%6*ZjLiVwDxzls+wt0R9X(}|;^_s)Pbuw>v;ha0*7@A1p57WvB%A2|cg
zz&~Svt9I9Ju_?P-&$cIbZ9scK6OnmY6bSUiBLD+AM{bH!{Xulhi-uWIvWUHe1N|XT
M2=UGt_yq<&0o!XWi2wiq

literal 0
HcmV?d00001

diff --git a/transport/cert/internal/test_signer.go b/transport/internal/ecp/test_signer.go
similarity index 81%
rename from transport/cert/internal/test_signer.go
rename to transport/internal/ecp/test_signer.go
index d40f851593b..aaa0ea6c11e 100644
--- a/transport/cert/internal/test_signer.go
+++ b/transport/internal/ecp/test_signer.go
@@ -29,16 +29,16 @@ type EnterpriseCertSigner struct {
 	cert *tls.Certificate
 }
 
-// Transport wraps a pair of unidirectional streams as an io.ReadWriteCloser.
-type Transport struct {
+// Connection wraps a pair of unidirectional streams as an io.ReadWriteCloser.
+type Connection struct {
 	io.ReadCloser
 	io.WriteCloser
 }
 
-// Close closes t's underlying ReadCloser and WriteCloser.
-func (t *Transport) Close() error {
-	rerr := t.ReadCloser.Close()
-	werr := t.WriteCloser.Close()
+// Close closes c's underlying ReadCloser and WriteCloser.
+func (c *Connection) Close() error {
+	rerr := c.ReadCloser.Close()
+	werr := c.WriteCloser.Close()
 	if rerr != nil {
 		return rerr
 	}
@@ -75,17 +75,17 @@ func main() {
 
 	data, err := ioutil.ReadFile(os.Args[1])
 	if err != nil {
-		log.Fatalf("Error reading certificate: %v", err)
+		log.Fatalf("Error reading certificate: %w", err)
 	}
 	cert, err := tls.X509KeyPair(data, data)
 	if err != nil {
-		log.Fatalf("Error creating X509 certificate: %v", err)
+		log.Printf("Error creating X509 certificate: %w", err)
 	}
 
 	enterpriseCertSigner.cert = &cert
 
 	if err := rpc.Register(enterpriseCertSigner); err != nil {
-		log.Fatalf("Error registering net/rpc: %v", err)
+		log.Fatalf("Error registering net/rpc: %w", err)
 	}
 
 	// If the parent process dies, we should exit.
@@ -100,5 +100,5 @@ func main() {
 		}
 	}()
 
-	rpc.ServeConn(&Transport{os.Stdin, os.Stdout})
+	rpc.ServeConn(&Connection{os.Stdin, os.Stdout})
 }

From 2bce4aa56aeeb5774717f9a6f3f73e50926c223c Mon Sep 17 00:00:00 2001
From: Andy Zhao <andyzhao@google.com>
Date: Wed, 8 Jun 2022 09:58:27 -0700
Subject: [PATCH 10/11] feat(transport): Fix test_signer.go

---
 transport/internal/ecp/.DS_Store      | Bin 6148 -> 0 bytes
 transport/internal/ecp/test_signer.go |   9 +++------
 2 files changed, 3 insertions(+), 6 deletions(-)
 delete mode 100644 transport/internal/ecp/.DS_Store

diff --git a/transport/internal/ecp/.DS_Store b/transport/internal/ecp/.DS_Store
deleted file mode 100644
index 440a2760323160c0b5dee4da6c9996d2e7b36e81..0000000000000000000000000000000000000000
GIT binary patch
literal 0
HcmV?d00001

literal 6148
zcmeHK!Ait15S`Hq1uuK_n4{PJL0sw=><>t{qC!g+cD?7nybAu7Z!%+P+2TzRnSta@
zW-^oJL6Zy-@%FWEh&Du2qY1JoD<bAq*M&{K0<z@jkNcrZX;?|}S55Nm6`kmjj&z!T
z|70AVn%zE)huID$-<aCA8&U^fo!&lPn%nogdBQRE-}(>FXSrv~xatfz1I~am;0*i(
z1GuwA>T5;sodIXS8JHN5^C6%KM#HRFjt+FC1OQer7lAIdgv12HXqXk@fv|=GHI%Ky
zU=4>o*t}?%6*ZjLiVwDxzls+wt0R9X(}|;^_s)Pbuw>v;ha0*7@A1p57WvB%A2|cg
zz&~Svt9I9Ju_?P-&$cIbZ9scK6OnmY6bSUiBLD+AM{bH!{Xulhi-uWIvWUHe1N|XT
M2=UGt_yq<&0o!XWi2wiq

diff --git a/transport/internal/ecp/test_signer.go b/transport/internal/ecp/test_signer.go
index aaa0ea6c11e..38425cf677b 100644
--- a/transport/internal/ecp/test_signer.go
+++ b/transport/internal/ecp/test_signer.go
@@ -75,17 +75,14 @@ func main() {
 
 	data, err := ioutil.ReadFile(os.Args[1])
 	if err != nil {
-		log.Fatalf("Error reading certificate: %w", err)
-	}
-	cert, err := tls.X509KeyPair(data, data)
-	if err != nil {
-		log.Printf("Error creating X509 certificate: %w", err)
+		log.Fatalf("Error reading certificate: %v", err)
 	}
+	cert, _ := tls.X509KeyPair(data, data)
 
 	enterpriseCertSigner.cert = &cert
 
 	if err := rpc.Register(enterpriseCertSigner); err != nil {
-		log.Fatalf("Error registering net/rpc: %w", err)
+		log.Fatalf("Error registering net/rpc: %v", err)
 	}
 
 	// If the parent process dies, we should exit.

From 6a1e1137bfa1475abf6a9a9fe9fbf89f52f9e601 Mon Sep 17 00:00:00 2001
From: Andy Zhao <andyzhao@google.com>
Date: Thu, 9 Jun 2022 10:30:58 -0700
Subject: [PATCH 11/11] feat(transport): Update tests to conform to go idiom

---
 transport/cert/enterprise_cert.go         |  8 +++---
 transport/cert/enterprise_cert_test.go    | 15 ++++------
 transport/cert/secureconnect_cert.go      |  8 +++---
 transport/cert/secureconnect_cert_test.go | 34 +++++++++++------------
 4 files changed, 31 insertions(+), 34 deletions(-)

diff --git a/transport/cert/enterprise_cert.go b/transport/cert/enterprise_cert.go
index 41d9e8ea92e..eaa52e07c08 100644
--- a/transport/cert/enterprise_cert.go
+++ b/transport/cert/enterprise_cert.go
@@ -35,11 +35,11 @@ type ecpSource struct {
 // a well-known gcloud location.
 func NewEnterpriseCertificateProxySource(configFilePath string) (Source, error) {
 	key, err := client.Cred(configFilePath)
-	if errors.Is(err, os.ErrNotExist) {
-		// Config file missing means Enterprise Certificate Proxy is not supported.
-		return nil, errSourceUnavailable
-	}
 	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			// Config file missing means Enterprise Certificate Proxy is not supported.
+			return nil, errSourceUnavailable
+		}
 		return nil, err
 	}
 
diff --git a/transport/cert/enterprise_cert_test.go b/transport/cert/enterprise_cert_test.go
index 9563a3d0d32..8f20887fd70 100644
--- a/transport/cert/enterprise_cert_test.go
+++ b/transport/cert/enterprise_cert_test.go
@@ -10,11 +10,11 @@ import (
 
 func TestEnterpriseCertificateProxySource_ConfigMissing(t *testing.T) {
 	source, err := NewEnterpriseCertificateProxySource("missing.json")
-	if !errors.Is(err, errSourceUnavailable) {
-		t.Fatal("NewEnterpriseCertificateProxySource: expected errSourceUnavailable returned when config is missing.")
+	if got, want := err, errSourceUnavailable; !errors.Is(err, errSourceUnavailable) {
+		t.Fatalf("NewEnterpriseCertificateProxySource: with missing config; got %v, want %v err", got, want)
 	}
 	if source != nil {
-		t.Error("NewEnterpriseCertificateProxySource: expected nil source returned when config is missing.")
+		t.Errorf("NewEnterpriseCertificateProxySource: with missing config; got %v, want nil source", source)
 	}
 }
 
@@ -29,10 +29,10 @@ func TestEnterpriseCertificateProxySource_GetClientCertificateSuccess(t *testing
 		t.Fatal(err)
 	}
 	if cert.Certificate == nil {
-		t.Error("getClientCertificate: want non-nil Certificate, got nil")
+		t.Error("getClientCertificate: got nil, want non-nil Certificate")
 	}
 	if cert.PrivateKey == nil {
-		t.Error("getClientCertificate: want non-nil PrivateKey, got nil")
+		t.Error("getClientCertificate: got nil, want non-nil PrivateKey")
 	}
 }
 
@@ -40,9 +40,6 @@ func TestEnterpriseCertificateProxySource_GetClientCertificateSuccess(t *testing
 func TestEnterpriseCertificateProxySource_InitializationFailure(t *testing.T) {
 	_, err := NewEnterpriseCertificateProxySource("testdata/enterprise_certificate_config_invalid_pem.json")
 	if err == nil {
-		t.Fatal("Expecting error.")
-	}
-	if got, want := err.Error(), "failed to parse public key: asn1: syntax error: sequence truncated"; got != want {
-		t.Errorf("NewEnterpriseCertificateProxySource: want err %v, got %v", want, got)
+		t.Error("NewEnterpriseCertificateProxySource: got nil, want non-nil err")
 	}
 }
diff --git a/transport/cert/secureconnect_cert.go b/transport/cert/secureconnect_cert.go
index 401a705216a..5913cab8017 100644
--- a/transport/cert/secureconnect_cert.go
+++ b/transport/cert/secureconnect_cert.go
@@ -60,11 +60,11 @@ func NewSecureConnectSource(configFilePath string) (Source, error) {
 	}
 
 	file, err := ioutil.ReadFile(configFilePath)
-	if errors.Is(err, os.ErrNotExist) {
-		// Config file missing means Secure Connect is not supported.
-		return nil, errSourceUnavailable
-	}
 	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			// Config file missing means Secure Connect is not supported.
+			return nil, errSourceUnavailable
+		}
 		return nil, err
 	}
 
diff --git a/transport/cert/secureconnect_cert_test.go b/transport/cert/secureconnect_cert_test.go
index 0117a0a5496..961477b5b46 100644
--- a/transport/cert/secureconnect_cert_test.go
+++ b/transport/cert/secureconnect_cert_test.go
@@ -11,11 +11,11 @@ import (
 
 func TestSecureConnectSource_ConfigMissing(t *testing.T) {
 	source, err := NewSecureConnectSource("missing.json")
-	if !errors.Is(err, errSourceUnavailable) {
-		t.Fatal("NewSecureConnectSource: expected errSourceUnavailable returned when config is missing.")
+	if got, want := err, errSourceUnavailable; !errors.Is(err, errSourceUnavailable) {
+		t.Fatalf("NewSecureConnectSource: with missing config; got %v, want %v err", got, want)
 	}
 	if source != nil {
-		t.Error("NewSecureConnectSource: expected nil source returned when config is missing.")
+		t.Errorf("NewSecureConnectSource: with missing config; got %v, want nil source", source)
 	}
 }
 
@@ -29,10 +29,10 @@ func TestSecureConnectSource_GetClientCertificateSuccess(t *testing.T) {
 		t.Error(err)
 	}
 	if cert.Certificate == nil {
-		t.Error("getClientCertificate: want non-nil Certificate, got nil")
+		t.Error("getClientCertificate: got nil, want non-nil Certificate")
 	}
 	if cert.PrivateKey == nil {
-		t.Error("getClientCertificate: want non-nil PrivateKey, got nil")
+		t.Error("getClientCertificate: got nil, want non-nil PrivateKey")
 	}
 }
 
@@ -43,7 +43,7 @@ func TestSecureConnectSource_GetClientCertificateFailure(t *testing.T) {
 	}
 	_, err = source(nil)
 	if err == nil {
-		t.Error("Expecting error.")
+		t.Error("getClientCertificate: got nil, want non-nil err")
 	}
 }
 
@@ -59,10 +59,10 @@ func TestSecureConnectSource_ValidateMetadataFailure(t *testing.T) {
 	metadata := secureConnectMetadata{Cmd: []string{}}
 	err := validateMetadata(metadata)
 	if err == nil {
-		t.Error("validateMetadata: want non-nil err, got nil")
+		t.Error("validateMetadata: got nil, want non-nil err")
 	}
-	if want, got := "empty cert_provider_command", err.Error(); want != got {
-		t.Errorf("validateMetadata: want %v err, got %v", want, got)
+	if got, want := err.Error(), "empty cert_provider_command"; got != want {
+		t.Errorf("validateMetadata: got %v, want %v err", got, want)
 	}
 }
 
@@ -76,7 +76,7 @@ func TestSecureConnectSource_IsCertificateExpiredTrue(t *testing.T) {
 		t.Error(err)
 	}
 	if !isCertificateExpired(cert) {
-		t.Error("isCertificateExpired: want true, got false")
+		t.Error("isCertificateExpired: got false, want true")
 	}
 }
 
@@ -90,7 +90,7 @@ func TestSecureConnectSource_IsCertificateExpiredFalse(t *testing.T) {
 		t.Error(err)
 	}
 	if isCertificateExpired(cert) {
-		t.Error("isCertificateExpired: want false, got true")
+		t.Error("isCertificateExpired: got true, want false")
 	}
 }
 
@@ -101,15 +101,15 @@ func TestCertificateCaching(t *testing.T) {
 		t.Fatal(err)
 	}
 	if cert == nil {
-		t.Fatal("getClientCertificate: want non-nil cert, got nil")
+		t.Fatal("getClientCertificate: got nil, want non-nil cert")
 	}
 	if source.cachedCert == nil {
-		t.Fatal("getClientCertificate: want non-nil cachedCert, got nil")
+		t.Fatal("getClientCertificate: got nil, want non-nil cachedCert")
 	}
-	if !bytes.Equal(cert.Certificate[0], source.cachedCert.Certificate[0]) {
-		t.Error("Cached certificate is different.")
+	if got, want := source.cachedCert.Certificate[0], cert.Certificate[0]; !bytes.Equal(got, want) {
+		t.Fatalf("getClientCertificate: got %v, want %v cached Certificate", got, want)
 	}
-	if cert.PrivateKey != source.cachedCert.PrivateKey {
-		t.Error("Cached PrivateKey is different.")
+	if got, want := source.cachedCert.PrivateKey, cert.PrivateKey; got != want {
+		t.Fatalf("getClientCertificate: got %v, want %v cached PrivateKey", got, want)
 	}
 }