Friendlier error message for provider signing errors (#1712)

Signed-off-by: Janos <86970079+janosdebugs@users.noreply.github.com>

internal/getproviders/package_authentication.go

@@ -10,6 +10,7 @@ import (
 	"bytes"
 	"crypto/sha256"
 	"encoding/hex"
+	"errors"
 	"fmt"
 
 	"log"
@@ -385,20 +386,23 @@ func NewSignatureAuthentication(meta PackageMeta, document, signature []byte, ke
 	}
 }
 
-func (s signatureAuthentication) shouldEnforceGPGValidation() (bool, error) {
+// ErrUnknownIssuer indicates an error when no valid signature for a provider could be found.
+var ErrUnknownIssuer = fmt.Errorf("authentication signature from unknown issuer")
+
+func (s signatureAuthentication) shouldEnforceGPGValidation() bool {
 	// we should enforce validation for all provider sources that are not the default provider registry
 	if s.ProviderSource != nil && s.ProviderSource.Hostname != tfaddr.DefaultProviderRegistryHost {
-		return true, nil
+		return true
 	}
 
 	// if we have been provided keys, we should enforce GPG validation
 	if len(s.Keys) > 0 {
-		return true, nil
+		return true
 	}
 
 	// otherwise if the environment variable is set to true, we should enforce GPG validation
 	enforceEnvVar, exists := os.LookupEnv(enforceGPGValidationEnvName)
-	return exists && enforceEnvVar == "true", nil
+	return exists && enforceEnvVar == "true"
 }
 func (s signatureAuthentication) shouldEnforceGPGExpiration() bool {
 	// otherwise if the environment variable is set to true, we should enforce GPG expiration
@@ -407,10 +411,7 @@ func (s signatureAuthentication) shouldEnforceGPGExpiration() bool {
 }
 
 func (s signatureAuthentication) AuthenticatePackage(location PackageLocation) (*PackageAuthenticationResult, error) {
-	shouldValidate, err := s.shouldEnforceGPGValidation()
+	shouldValidate := s.shouldEnforceGPGValidation()
-	if err != nil {
-		return nil, fmt.Errorf("error determining if GPG validation should be enforced for pacakage %s: %w", location.String(), err)
-	}
 
 	if !shouldValidate {
 		// As this is a temporary measure, we will log a warning to the user making it very clear what is happening
@@ -430,7 +431,7 @@ func (s signatureAuthentication) AuthenticatePackage(location PackageLocation) (
 
 	_, keyID, err := s.findSigningKey()
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("the provider is not signed with a valid signing key; please contact the provider author (%w)", err)
 	}
 
 	// We have a valid signature.
@@ -496,8 +497,8 @@ func (s signatureAuthentication) findSigningKey() (*SigningKey, string, error) {
 		}
 
 		entity, err := openpgp.CheckDetachedSignature(keyring, bytes.NewReader(s.Document), bytes.NewReader(s.Signature), nil)
-		if !s.shouldEnforceGPGExpiration() && (err == openpgpErrors.ErrSignatureExpired || err == openpgpErrors.ErrKeyExpired) {
+		if !s.shouldEnforceGPGExpiration() && (errors.Is(err, openpgpErrors.ErrKeyExpired) || errors.Is(err, openpgpErrors.ErrSignatureExpired)) {
-			// Internally openpgp will *only* return the Expired errors if all other checks have succeded
+			// Internally openpgp will *only* return the Expired errors if all other checks have succeeded
 			// This is currently the best way to work around expired provider keys
 			fmt.Printf("[WARN] Provider %s/%s (%v) gpg key expired, this will fail in future versions of OpenTofu\n", s.Meta.Provider.Namespace, s.Meta.Provider.Type, s.Meta.Provider.Hostname)
 			err = nil
@@ -505,7 +506,7 @@ func (s signatureAuthentication) findSigningKey() (*SigningKey, string, error) {
 
 		// If the signature issuer does not match the key, keep trying the
 		// rest of the provided keys.
-		if err == openpgpErrors.ErrUnknownIssuer {
+		if errors.Is(err, openpgpErrors.ErrUnknownIssuer) {
 			continue
 		}
 
@@ -525,7 +526,7 @@ func (s signatureAuthentication) findSigningKey() (*SigningKey, string, error) {
 
 	// If none of the provided keys issued the signature, this package is
 	// unsigned. This is currently a terminal authentication error.
-	return nil, "", fmt.Errorf("authentication signature from unknown issuer")
+	return nil, "", ErrUnknownIssuer
 }
 
 // entityString extracts the key ID and identity name(s) from an openpgp.Entity

internal/getproviders/package_authentication_test.go

@@ -463,9 +463,10 @@ func TestNewSignatureAuthentication_expired(t *testing.T) {
 // to OpenPGP failures from malformed keys or signatures.
 func TestSignatureAuthentication_failure(t *testing.T) {
 	tests := map[string]struct {
-		signature string
+		signature    string
-		keys      []SigningKey
+		keys         []SigningKey
-		err       string
+		errorType    any
+		errorMessage string
 	}{
 		"invalid key": {
 			testHashicorpSignatureGoodBase64,
@@ -474,7 +475,8 @@ func TestSignatureAuthentication_failure(t *testing.T) {
 					ASCIIArmor: "invalid PGP armor value",
 				},
 			},
-			"error decoding signing key: openpgp: invalid argument: no armored data found",
+			openpgpErrors.InvalidArgumentError(""),
+			"no armored data found",
 		},
 		"invalid signature": {
 			testSignatureBadBase64,
@@ -483,7 +485,8 @@ func TestSignatureAuthentication_failure(t *testing.T) {
 					ASCIIArmor: testAuthorKeyArmor,
 				},
 			},
-			"error checking signature: openpgp: invalid data: signature subpacket truncated",
+			openpgpErrors.InvalidArgumentError(""),
+			"signature subpacket truncated",
 		},
 		"no keys match signature": {
 			testAuthorSignatureGoodBase64,
@@ -492,11 +495,13 @@ func TestSignatureAuthentication_failure(t *testing.T) {
 					ASCIIArmor: TestingPublicKey,
 				},
 			},
-			"authentication signature from unknown issuer",
+			nil,
+			ErrUnknownIssuer.Error(),
 		},
 	}
 
 	for name, test := range tests {
+		test := test
 		t.Run(name, func(t *testing.T) {
 			// Location is unused
 			location := PackageLocalArchive("testdata/my-package.zip")
@@ -512,8 +517,21 @@ func TestSignatureAuthentication_failure(t *testing.T) {
 			if result != nil {
 				t.Errorf("wrong result: got %#v, want nil", result)
 			}
-			if gotErr := err.Error(); gotErr != test.err {
+			if test.errorType != nil {
-				t.Errorf("wrong err: got %s, want %s", gotErr, test.err)
+				if err == nil {
+					t.Errorf("expected error of type %v, got nil", test.errorType)
+				}
+				if !errors.As(err, &test.errorType) {
+					t.Errorf("wrong error type: got %v, want %v", err, test.errorType)
+				}
+			}
+			if test.errorMessage != "" {
+				if err == nil {
+					t.Errorf("expected error of type %v, got nil", test.errorType)
+				}
+				if !strings.Contains(err.Error(), test.errorMessage) {
+					t.Errorf("wrong error message: %s (expected an error message containing %s)", err.Error(), test.errorMessage)
+				}
 			}
 		})
 	}
@@ -879,10 +897,7 @@ func TestShouldEnforceGPGValidation(t *testing.T) {
 				t.Setenv(enforceGPGValidationEnvName, tt.envVarValue)
 			}
 
-			actual, err := sigAuth.shouldEnforceGPGValidation()
+			actual := sigAuth.shouldEnforceGPGValidation()
-			if err != nil {
-				t.Fatal(err)
-			}
 			if actual != tt.expected {
 				t.Errorf("expected %t, actual %t", tt.expected, actual)
 			}