Repository: sigstore/fulcio
Pull request #1268 (2023-07-09 19:46:41)

always use non-TLS credentials to connect over unix domain socket
Fixes: #1267

<!--
Thanks for opening a pull request! Please do not just delete this text.  The three fields below are mandatory.

Please remember to:
- This PR requires an issue. If it is a new feature, the issue should proceed the PR and will have allowed sufficent time for discussions to take place. Please use
issue tags such as "Closes #XYZ" or "Resolves sigstore/repo-name#XYZ".
  [documentation](https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)
- ensure your commits are signed-off, as sigstore uses the [DCO](https://en.wikipedia.org/wiki/Developer_Certificate_of_Origin) using `git commit -s`, or `git commit -s --amend` if you want to amend already existing commits
- lastly, ensure there are no merge commits!

Thank you :)
-->

#### Summary
<!--
 Explain the **motivation** for making this change. What existing problem does the pull request solve? How can reviewers test this PR?
-->

#### Release Note
<!--
Add a release note for each of the following conditions:

* Config changes (additions, deletions, updates)
* API additions—new endpoint, new response fields, or newly accepted request parameters
* Database changes (any)
* Websocket additions or changes
* Anything noteworthy to an administrator running private sigstore instances (err on the side of over-communicating)
* New features and improvements, including behavioural changes, UI changes and CLI changes
* Bug fixes and fixes of previous known issues
* Deprecation warnings, breaking changes, or compatibility notes

If no release notes are required write NONE. Use past-tense.

-->

#### Documentation
<!--

Does this change require an update to documentation? How will users implement your new feature?

Please reference a PR within https://docs.sigstore.dev

-->


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/sigstore/fulcio/pull/1268?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) Report
> Merging [#1268](https://app.codecov.io/gh/sigstore/fulcio/pull/1268?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) (2dea6a9) into [main](https://app.codecov.io/gh/sigstore/fulcio/commit/381531879578eddf1edf167c9915d15b682cbf85?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) (3815318) will **increase** coverage by `1.70%`.
> The diff coverage is `66.66%`.

```diff
@@            Coverage Diff             @@
##             main    #1268      +/-   ##
==========================================
+ Coverage   55.86%   57.56%   +1.70%     
==========================================
  Files          50       50              
  Lines        2991     2993       +2     
==========================================
+ Hits         1671     1723      +52     
+ Misses       1167     1117      -50     
  Partials      153      153              
```


| [Impacted Files](https://app.codecov.io/gh/sigstore/fulcio/pull/1268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) | Coverage Δ | |
|---|---|---|
| [cmd/app/http.go](https://app.codecov.io/gh/sigstore/fulcio/pull/1268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-Y21kL2FwcC9odHRwLmdv) | `60.52% <66.66%> (+9.17%)` | :arrow_up: |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/sigstore/fulcio/pull/1268/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore)



Patch:
diff --git a/cmd/app/http.go b/cmd/app/http.go
index 87249fd..ad096d2 100644
--- a/cmd/app/http.go
+++ b/cmd/app/http.go
@@ -73,7 +73,9 @@ func createHTTPServer(ctx context.Context, serverEndpoint string, grpcServer, le
 
 	if legacyGRPCServer != nil {
 		endpoint := fmt.Sprintf("unix:%v", legacyGRPCServer.grpcServerEndpoint)
-		if err := legacy_gw.RegisterCAHandlerFromEndpoint(ctx, mux, endpoint, opts); err != nil {
+		// we are connecting over a unix domain socket, therefore we won't ever need TLS
+		unixDomainSocketOpts := []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())}
+		if err := legacy_gw.RegisterCAHandlerFromEndpoint(ctx, mux, endpoint, unixDomainSocketOpts); err != nil {
 			log.Logger.Fatal(err)
 		}
 	}


Test patch:
diff --git a/cmd/app/http_test.go b/cmd/app/http_test.go
index f2c6870..7495ee4 100644
--- a/cmd/app/http_test.go
+++ b/cmd/app/http_test.go
@@ -24,6 +24,8 @@ import (
 	"net"
 	"net/http"
 	"net/url"
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -70,6 +72,56 @@ func setupHTTPServer(t *testing.T) (httpServer, string) {
 
 }
 
+// setup with GRPC TLS enabled
+func setupHTTPServerWithGRPCTLS(t *testing.T) (httpServer, string) {
+	t.Helper()
+	httpListen, err := net.Listen("tcp", ":0")
+	if err != nil {
+		t.Error(err)
+	}
+
+	tlsPKIDir := t.TempDir()
+	certPath := filepath.Join(tlsPKIDir, "cert.pem")
+	os.WriteFile(certPath, []byte(certPEM), 0644)
+	keyPath := filepath.Join(tlsPKIDir, "key.pem")
+	os.WriteFile(keyPath, []byte(keyPEM), 0644)
+
+	viper.Set("grpc-tls-certificate", certPath)
+	viper.Set("grpc-tls-key", keyPath)
+
+	viper.Set("grpc-host", "")
+	viper.Set("grpc-port", 0)
+	grpcServer, err := createGRPCServer(nil, nil, &TrivialCertificateAuthority{}, nil)
+	if err != nil {
+		t.Error(err)
+	}
+	grpcServer.startTCPListener()
+	conn, err := grpc.Dial(grpcServer.grpcServerEndpoint, grpc.WithTransportCredentials(insecure.NewCredentials()))
+	defer func() {
+		if conn != nil {
+			_ = conn.Close()
+		}
+	}()
+	if err != nil {
+		t.Error(err)
+	}
+	legacyGRPCServer, err := createLegacyGRPCServer(nil, grpcServer.caService)
+	if err != nil {
+		t.Fatal(err)
+	}
+	legacyGRPCServer.startUnixListener()
+
+	httpHost := httpListen.Addr().String()
+	httpServer := createHTTPServer(context.Background(), httpHost, grpcServer, legacyGRPCServer)
+	go func() {
+		_ = httpServer.Serve(httpListen)
+		grpcServer.GracefulStop()
+	}()
+
+	return httpServer, fmt.Sprintf("http://%s", httpHost)
+
+}
+
 func TestHTTPCORSSupport(t *testing.T) {
 	httpServer, host := setupHTTPServer(t)
 	defer httpServer.Close()
@@ -109,6 +161,23 @@ func TestHTTPDoesntLeakGRPCHeaders(t *testing.T) {
 	}
 }
 
+func TestIssue1267(t *testing.T) {
+	httpServer, host := setupHTTPServerWithGRPCTLS(t)
+	defer httpServer.Close()
+
+	url, _ := url.Parse(host + "/api/v1/rootCert")
+	req := http.Request{
+		Method: "GET",
+		URL:    url,
+	}
+
+	resp, err := http.DefaultClient.Do(&req)
+	if err != nil || resp.StatusCode != http.StatusOK {
+		t.Errorf("unexpected response: %v, %v", resp, err)
+	}
+	defer resp.Body.Close()
+}
+
 // Trivial CA service that returns junk
 type TrivialCertificateAuthority struct {
 }