Trim secret before signing JWT tokensid-fix-jwt-tokens

With this change we don't rely on the secret to either contain
a newline or not contain it.

Changelog: fixed

3 files changed, 98 insertions, 3 deletions

diff --git a/internal/gitlab/client/gitlabnet.go b/internal/gitlab/client/gitlabnet.go
index da4da1c93..e903bb208 100644
--- a/internal/gitlab/client/gitlabnet.go
+++ b/internal/gitlab/client/gitlabnet.go
@@ -135,9 +135,7 @@ func (c *GitlabNetClient) DoRequest(ctx context.Context, method, path string, da
 	if user != "" && password != "" {
 		request.SetBasicAuth(user, password)
 	}
-	secretBytes := []byte(c.secret)
-
-	encodedSecret := base64.StdEncoding.EncodeToString(secretBytes)
+	encodedSecret := base64.StdEncoding.EncodeToString([]byte(c.secret))
 	request.Header.Set(secretHeaderName, encodedSecret)
 
 	claims := jwt.RegisteredClaims{
@@ -145,6 +143,7 @@ func (c *GitlabNetClient) DoRequest(ctx context.Context, method, path string, da
 		IssuedAt:  jwt.NewNumericDate(time.Now()),
 		ExpiresAt: jwt.NewNumericDate(time.Now().Add(jwtTTL)),
 	}
+	secretBytes := []byte(strings.TrimSpace(c.secret))
 	tokenString, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString(secretBytes)
 	if err != nil {
 		return nil, err
diff --git a/internal/gitlab/client/gitlabnet_test.go b/internal/gitlab/client/gitlabnet_test.go
new file mode 100644
index 000000000..a6695ee5d
--- /dev/null
+++ b/internal/gitlab/client/gitlabnet_test.go
@@ -0,0 +1,73 @@
+package client
+
+import (
+	"fmt"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"time"
+
+	"github.com/golang-jwt/jwt/v4"
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
+)
+
+const secret = "it's a secret"
+
+func TestJWTAuthenticationHeader(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_, err := fmt.Fprint(w, r.Header.Get(apiSecretHeaderName))
+		require.NoError(t, err)
+	}))
+	defer server.Close()
+
+	tests := []struct {
+		secret string
+		method string
+	}{
+		{
+			secret: secret,
+			method: http.MethodGet,
+		},
+		{
+			secret: secret,
+			method: http.MethodPost,
+		},
+		{
+			secret: "\n\t " + secret + "\t \n",
+			method: http.MethodGet,
+		},
+		{
+			secret: "\n \t" + secret + "\n\t ",
+			method: http.MethodPost,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.method+" with "+tc.secret, func(t *testing.T) {
+			gitlabnet := &GitlabNetClient{
+				httpClient: &HTTPClient{Client: server.Client(), Host: server.URL},
+				secret:     tc.secret,
+			}
+
+			response, err := gitlabnet.DoRequest(testhelper.Context(t), tc.method, "/jwt_auth", nil)
+			require.NoError(t, err)
+			require.NotNil(t, response)
+			defer response.Body.Close()
+
+			responseBody, err := io.ReadAll(response.Body)
+			require.NoError(t, err)
+
+			claims := &jwt.RegisteredClaims{}
+			token, err := jwt.ParseWithClaims(string(responseBody), claims, func(token *jwt.Token) (interface{}, error) {
+				return []byte(secret), nil
+			})
+			require.NoError(t, err)
+			require.True(t, token.Valid)
+			require.Equal(t, "gitlab-shell", claims.Issuer)
+			require.WithinDuration(t, time.Now().Truncate(time.Second), claims.IssuedAt.Time, time.Second)
+			require.WithinDuration(t, time.Now().Truncate(time.Second).Add(time.Minute), claims.ExpiresAt.Time, time.Second)
+		})
+	}
+}
diff --git a/internal/gitlab/test_server.go b/internal/gitlab/test_server.go
index 3a6a78eb0..de610fe2a 100644
--- a/internal/gitlab/test_server.go
+++ b/internal/gitlab/test_server.go
@@ -16,6 +16,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/golang-jwt/jwt/v4"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
@@ -298,6 +299,10 @@ func handleAllowed(tb testing.TB, options TestServerOptions) func(w http.Respons
 			}
 		}
 
+		if !verifyJWT(r.Header.Get("Gitlab-Shell-Api-Request"), options.SecretToken) {
+			authenticated = false
+		}
+
 		if authenticated {
 			_, err := w.Write([]byte(`{"status":true}`))
 			require.NoError(tb, err)
@@ -378,6 +383,10 @@ func handlePreReceive(tb testing.TB, options TestServerOptions) func(w http.Resp
 			}
 		}
 
+		if !verifyJWT(r.Header.Get("Gitlab-Shell-Api-Request"), options.SecretToken) {
+			authenticated = false
+		}
+
 		if !authenticated {
 			http.Error(w, "unauthorized", http.StatusUnauthorized)
 			return
@@ -440,6 +449,11 @@ func handlePostReceive(options TestServerOptions) func(w http.ResponseWriter, r
 			}
 		}
 
+		if !verifyJWT(r.Header.Get("Gitlab-Shell-Api-Request"), options.SecretToken) {
+			http.Error(w, "jwt header is invalid", http.StatusUnauthorized)
+			return
+		}
+
 		if params.Identifier == "" {
 			http.Error(w, "identifier is empty", http.StatusUnauthorized)
 			return
@@ -557,6 +571,15 @@ func handleLfs(tb testing.TB, options TestServerOptions) func(w http.ResponseWri
 	}
 }
 
+func verifyJWT(header, secretToken string) bool {
+	claims := &jwt.RegisteredClaims{}
+	token, err := jwt.ParseWithClaims(header, claims, func(token *jwt.Token) (interface{}, error) {
+		return []byte(secretToken), nil
+	})
+
+	return err == nil && token.Valid && claims.Issuer == "gitlab-shell"
+}
+
 func startSocketHTTPServer(tb testing.TB, mux *http.ServeMux, tlsCfg *tls.Config) (string, func()) {
 	tempDir := testhelper.TempDir(tb)