If one gpg check is working, upgrade should continue (#3426) (#3443)

(cherry picked from commit 7be7f62)

Co-authored-by: Pierre HILBERT <[email protected]>

changelog/fragments/1694700201-gpg-unreachable-url-fix.yaml

@@ -0,0 +1,32 @@
+# Kind can be one of:
+# - breaking-change: a change to previously-documented behavior
+# - deprecation: functionality that is being removed in a later release
+# - bug-fix: fixes a problem in a previous version
+# - enhancement: extends functionality but does not break or fix existing behavior
+# - feature: new functionality
+# - known-issue: problems that we are aware of in a given version
+# - security: impacts on the security of a product or a user’s deployment.
+# - upgrade: important information for someone upgrading from a prior version
+# - other: does not fit into any of the other categories
+kind: bug-fix
+
+# Change summary; a 80ish characters long description of the change.
+summary: Fix gpg verification, if one is successful upgrade should continue.
+
+# Long description; in case the summary is not enough to describe the change
+# this field accommodate a description without length limits.
+# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment.
+#description:
+
+# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
+component: elastic-agent
+
+# PR URL; optional; the PR number that added the changeset.
+# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
+# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
+# Please provide it if you are adding a fragment for a different PR.
+pr: https://github.com/elastic/elastic-agent/pull/3426
+
+# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
+# If not present is automatically filled by the tooling with the issue linked to the PR number.
+issue: https://github.com/elastic/elastic-agent/issues/3368

internal/pkg/agent/application/upgrade/artifact/download/composed/verifier.go

@@ -10,53 +10,55 @@ import (
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact/download"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
+	"github.com/elastic/elastic-agent/pkg/core/logger"
 )
 
 // Verifier is a verifier with a predefined set of verifiers.
 // During each verify call it tries to call the first one and on failure fallbacks to
 // the next one.
 // Error is returned if all of them fail.
 type Verifier struct {
-	vv []download.Verifier
+	vv  []download.Verifier
+	log *logger.Logger
+}
+
+func (v *Verifier) Name() string {
+	return "composed.verifier"
 }
 
 // NewVerifier creates a verifier composed out of predefined set of verifiers.
 // During each verify call it tries to call the first one and on failure fallbacks to
 // the next one.
 // Error is returned if all of them fail.
-func NewVerifier(verifiers ...download.Verifier) *Verifier {
+func NewVerifier(log *logger.Logger, verifiers ...download.Verifier) *Verifier {
 	return &Verifier{
-		vv: verifiers,
+		log: log,
+		vv:  verifiers,
 	}
 }
 
 // Verify checks the package from configured source.
-func (e *Verifier) Verify(a artifact.Artifact, version string, skipDefaultPgp bool, pgpBytes ...string) error {
+func (v *Verifier) Verify(a artifact.Artifact, version string, skipDefaultPgp bool, pgpBytes ...string) error {
 	var err error
-	var checksumMismatchErr *download.ChecksumMismatchError
-	var invalidSignatureErr *download.InvalidSignatureError
 
-	for _, v := range e.vv {
-		e := v.Verify(a, version, skipDefaultPgp, pgpBytes...)
+	for _, verifier := range v.vv {
+		e := verifier.Verify(a, version, skipDefaultPgp, pgpBytes...)
 		if e == nil {
 			// Success
 			return nil
 		}
 
 		err = multierror.Append(err, e)
 
-		if errors.As(e, &checksumMismatchErr) || errors.As(err, &invalidSignatureErr) {
-			// Stop verification chain on checksum/signature errors.
-			break
-		}
+		v.log.Warnw("Verifier failed!", "verifier", verifier.Name(), "error", e)
 	}
 
 	return err
 }
 
-func (e *Verifier) Reload(c *artifact.Config) error {
-	for _, v := range e.vv {
-		reloadable, ok := v.(download.Reloader)
+func (v *Verifier) Reload(c *artifact.Config) error {
+	for _, verifier := range v.vv {
+		reloadable, ok := verifier.(download.Reloader)
 		if !ok {
 			continue
 		}

internal/pkg/agent/application/upgrade/artifact/download/composed/verifier_test.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact/download"
+	"github.com/elastic/elastic-agent/pkg/core/logger"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -18,6 +19,10 @@ type ErrorVerifier struct {
 	called bool
 }
 
+func (d *ErrorVerifier) Name() string {
+	return "error"
+}
+
 func (d *ErrorVerifier) Verify(a artifact.Artifact, version string, _ bool, _ ...string) error {
 	d.called = true
 	return errors.New("failing")
@@ -29,9 +34,13 @@ type FailVerifier struct {
 	called bool
 }
 
+func (d *FailVerifier) Name() string {
+	return "fail"
+}
+
 func (d *FailVerifier) Verify(a artifact.Artifact, version string, _ bool, _ ...string) error {
 	d.called = true
-	return &download.InvalidSignatureError{}
+	return &download.InvalidSignatureError{File: "", Err: errors.New("invalid signature")}
 }
 
 func (d *FailVerifier) Called() bool { return d.called }
@@ -40,6 +49,10 @@ type SuccVerifier struct {
 	called bool
 }
 
+func (d *SuccVerifier) Name() string {
+	return "succ"
+}
+
 func (d *SuccVerifier) Verify(a artifact.Artifact, version string, _ bool, _ ...string) error {
 	d.called = true
 	return nil
@@ -48,6 +61,7 @@ func (d *SuccVerifier) Verify(a artifact.Artifact, version string, _ bool, _ ...
 func (d *SuccVerifier) Called() bool { return d.called }
 
 func TestVerifier(t *testing.T) {
+	log, _ := logger.New("", false)
 	testCases := []verifyTestCase{
 		{
 			verifiers:      []CheckableVerifier{&ErrorVerifier{}, &SuccVerifier{}, &FailVerifier{}},
@@ -59,21 +73,21 @@ func TestVerifier(t *testing.T) {
 			expectedResult: true,
 		}, {
 			verifiers:      []CheckableVerifier{&FailVerifier{}, &ErrorVerifier{}, &SuccVerifier{}},
-			checkFunc:      func(d []CheckableVerifier) bool { return d[0].Called() && !d[1].Called() && !d[2].Called() },
-			expectedResult: false,
+			checkFunc:      func(d []CheckableVerifier) bool { return d[0].Called() && d[1].Called() && d[2].Called() },
+			expectedResult: true,
 		}, {
 			verifiers:      []CheckableVerifier{&ErrorVerifier{}, &FailVerifier{}, &SuccVerifier{}},
-			checkFunc:      func(d []CheckableVerifier) bool { return d[0].Called() && d[1].Called() && !d[2].Called() },
-			expectedResult: false,
-		}, {
-			verifiers:      []CheckableVerifier{&ErrorVerifier{}, &ErrorVerifier{}, &SuccVerifier{}},
 			checkFunc:      func(d []CheckableVerifier) bool { return d[0].Called() && d[1].Called() && d[2].Called() },
 			expectedResult: true,
+		}, {
+			verifiers:      []CheckableVerifier{&ErrorVerifier{}, &ErrorVerifier{}, &FailVerifier{}},
+			checkFunc:      func(d []CheckableVerifier) bool { return d[0].Called() && d[1].Called() && d[2].Called() },
+			expectedResult: false,
 		},
 	}
 
 	for _, tc := range testCases {
-		d := NewVerifier(tc.verifiers[0], tc.verifiers[1], tc.verifiers[2])
+		d := NewVerifier(log, tc.verifiers[0], tc.verifiers[1], tc.verifiers[2])
 		err := d.Verify(artifact.Artifact{Name: "a", Cmd: "a", Artifact: "a/a"}, "b", false)
 
 		assert.Equal(t, tc.expectedResult, err == nil)

internal/pkg/agent/application/upgrade/artifact/download/fs/verifier.go

@@ -33,6 +33,10 @@ type Verifier struct {
 	log           *logger.Logger
 }
 
+func (v *Verifier) Name() string {
+	return "fs.verifier"
+}
+
 // NewVerifier creates a verifier checking downloaded package on preconfigured
 // location against a key stored on elastic.co website.
 func NewVerifier(log *logger.Logger, config *artifact.Config, allowEmptyPgp bool, pgp []byte) (*Verifier, error) {

internal/pkg/agent/application/upgrade/artifact/download/http/verifier.go

@@ -36,6 +36,10 @@ type Verifier struct {
 	log           progressLogger
 }
 
+func (v *Verifier) Name() string {
+	return "http.verifier"
+}
+
 // NewVerifier create a verifier checking downloaded package on preconfigured
 // location against a key stored on elastic.co website.
 func NewVerifier(log progressLogger, config *artifact.Config, allowEmptyPgp bool, pgp []byte) (*Verifier, error) {

internal/pkg/agent/application/upgrade/artifact/download/localremote/verifier.go

@@ -44,5 +44,5 @@ func NewVerifier(log *logger.Logger, config *artifact.Config, allowEmptyPgp bool
 	}
 	verifiers = append(verifiers, remoteVer)
 
-	return composed.NewVerifier(verifiers...), nil
+	return composed.NewVerifier(log, verifiers...), nil
 }

internal/pkg/agent/application/upgrade/artifact/download/snapshot/verifier.go

@@ -18,6 +18,10 @@ type Verifier struct {
 	versionOverride *agtversion.ParsedSemVer
 }
 
+func (v *Verifier) Name() string {
+	return "snapshot.verifier"
+}
+
 // NewVerifier creates a downloader which first checks local directory
 // and then fallbacks to remote if configured.
 func NewVerifier(log *logger.Logger, config *artifact.Config, allowEmptyPgp bool, pgp []byte, versionOverride *agtversion.ParsedSemVer) (download.Verifier, error) {
@@ -37,17 +41,17 @@ func NewVerifier(log *logger.Logger, config *artifact.Config, allowEmptyPgp bool
 }
 
 // Verify checks the package from configured source.
-func (e *Verifier) Verify(a artifact.Artifact, version string, skipDefaultPgp bool, pgpBytes ...string) error {
-	return e.verifier.Verify(a, version, skipDefaultPgp, pgpBytes...)
+func (v *Verifier) Verify(a artifact.Artifact, version string, skipDefaultPgp bool, pgpBytes ...string) error {
+	return v.verifier.Verify(a, version, skipDefaultPgp, pgpBytes...)
 }
 
-func (e *Verifier) Reload(c *artifact.Config) error {
-	reloader, ok := e.verifier.(artifact.ConfigReloader)
+func (v *Verifier) Reload(c *artifact.Config) error {
+	reloader, ok := v.verifier.(artifact.ConfigReloader)
 	if !ok {
 		return nil
 	}
 
-	cfg, err := snapshotConfig(c, e.versionOverride)
+	cfg, err := snapshotConfig(c, v.versionOverride)
 	if err != nil {
 		return errors.New(err, "snapshot.downloader: failed to generate snapshot config")
 	}

internal/pkg/agent/application/upgrade/artifact/download/verifier.go

@@ -60,7 +60,8 @@ func (e *InvalidSignatureError) Unwrap() error { return e.Err }
 // Verifier is an interface verifying the SHA512 checksum and GPG signature and
 // of a downloaded artifact.
 type Verifier interface {
-	// Verify should verify the artifact and return an error if any checks fail.
+	Name() string
+	// Verify should verify the artifact and return if succeed status (true|false) and an error if any checks fail.
 	// If the checksum does no match Verify returns a
 	// *download.ChecksumMismatchError. And if the GPG signature is invalid then
 	// Verify returns a *download.InvalidSignatureError. Use errors.As() to

internal/pkg/agent/application/upgrade/step_download.go

@@ -84,7 +84,6 @@ func (u *Upgrader) downloadArtifact(ctx context.Context, version, sourceURI stri
 	if err := verifier.Verify(agentArtifact, parsedVersion.VersionWithPrerelease(), skipDefaultPgp, pgpBytes...); err != nil {
 		return "", errors.New(err, "failed verification of agent binary")
 	}
-
 	return path, nil
 }
 
@@ -141,7 +140,7 @@ func newVerifier(version *agtversion.ParsedSemVer, log *logger.Logger, settings
 		return nil, err
 	}
 
-	return composed.NewVerifier(fsVerifier, snapshotVerifier, remoteVerifier), nil
+	return composed.NewVerifier(log, fsVerifier, snapshotVerifier, remoteVerifier), nil
 }
 
 func (u *Upgrader) downloadWithRetries(