fix: fix faulty logic for handling possible double-encoding of github…

… app private key (#3363)

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

internal/credentials/kubernetes/github/app.go

@@ -6,6 +6,7 @@ import (
 	"encoding/base64"
 	"errors"
 	"fmt"
+	"regexp"
 	"strconv"
 	"time"
 
@@ -24,6 +25,8 @@ const (
 	accessTokenUsername = "kargo"
 )
 
+var base64Regex = regexp.MustCompile(`^[a-zA-Z0-9+/]*={0,2}$`)
+
 type appCredentialHelper struct {
 	tokenCache *cache.Cache
 
@@ -137,16 +140,9 @@ func (a *appCredentialHelper) getAccessToken(
 	installationID int64,
 	encodedPrivateKey string,
 ) (string, error) {
-	decodedKey, err := base64.StdEncoding.DecodeString(encodedPrivateKey)
+	decodedKey, err := decodeKey(encodedPrivateKey)
 	if err != nil {
-		if corruptInputErr := new(base64.CorruptInputError); !errors.As(err, &corruptInputErr) {
-			return "", fmt.Errorf("error decoding private key: %w", err)
-		}
-
-		// If the key is not base64 encoded, it may be a raw key. Try using it
-		// as-is. We do this because initially, we required the PEM-encoded key
-		// to be base64 encoded (for reasons unknown today).
-		decodedKey = []byte(encodedPrivateKey)
+		return "", err
 	}
 	appTokenSource, err := githubauth.NewApplicationTokenSource(appID, decodedKey)
 	if err != nil {
@@ -159,3 +155,28 @@ func (a *appCredentialHelper) getAccessToken(
 	}
 	return token.AccessToken, nil
 }
+
+// decodeKey attempts to base64 decode a key. If successful, it returns the
+// result. If it fails, it attempts to infer whether the input was simply NOT
+// base64 encoded or whether it appears to have been base64 encoded but
+// corrupted -- due perhaps to a copy/paste error. This inference determines
+// whether to return the input as is or surface the decoding error. All other
+// errors are surfaced as is. This function is necessary because we initially
+// required the PEM-encoded key to be base64 encoded (for reasons unknown today)
+// and then we later dropped that requirement.
+func decodeKey(key string) ([]byte, error) {
+	decodedKey, err := base64.StdEncoding.DecodeString(key)
+	if err != nil {
+		if !errors.As(err, new(base64.CorruptInputError)) {
+			return nil, fmt.Errorf("error decoding private key: %w", err)
+		}
+		if base64Regex.MatchString(key) {
+			return nil, fmt.Errorf(
+				"probable corrupt base64 encoding of private key; base64 encoding "+
+					"this key is no longer required and is discouraged: %w", err,
+			)
+		}
+		return []byte(key), nil
+	}
+	return decodedKey, nil
+}

internal/credentials/kubernetes/github/app_test.go

@@ -2,6 +2,7 @@ package github
 
 import (
 	"context"
+	"encoding/base64"
 	"fmt"
 	"strconv"
 	"testing"
@@ -187,3 +188,40 @@ func TestAppCredentialHelper(t *testing.T) {
 		})
 	}
 }
+
+func TestGetAccessToken(t *testing.T) {
+	const key = "-----BEGIN PRIVATE KEY-----\nfakekey\n-----END PRIVATE KEY-----"
+	testCases := []struct {
+		name        string
+		key         string
+		expectedKey string
+		expectsErr  bool
+	}{
+		{
+			name:        "key is not base64 encoded",
+			key:         key,
+			expectedKey: key,
+		},
+		{
+			name:        "key is base64 encoded",
+			key:         base64.StdEncoding.EncodeToString([]byte(key)),
+			expectedKey: key,
+		},
+		{
+			name:       "key is a corrupted base64 encoding",
+			key:        "corrupted", // These are all base64 digits. :)
+			expectsErr: true,
+		},
+	}
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			key, err := decodeKey(testCase.key)
+			if testCase.expectsErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, []byte(testCase.expectedKey), key)
+		})
+	}
+}