From 7be7f62fcca48351b2262419ca06e2cf5df1418c Mon Sep 17 00:00:00 2001 From: Pierre HILBERT Date: Wed, 20 Sep 2023 18:07:39 +0200 Subject: [PATCH] If one gpg check is working, upgrade should continue (#3426) --- .../1694700201-gpg-unreachable-url-fix.yaml | 32 +++++++++++++++++++ .../artifact/download/composed/verifier.go | 32 ++++++++++--------- .../download/composed/verifier_test.go | 30 ++++++++++++----- .../upgrade/artifact/download/fs/verifier.go | 4 +++ .../artifact/download/http/verifier.go | 4 +++ .../artifact/download/localremote/verifier.go | 2 +- .../artifact/download/snapshot/verifier.go | 14 +++++--- .../upgrade/artifact/download/verifier.go | 3 +- .../application/upgrade/step_download.go | 3 +- 9 files changed, 92 insertions(+), 32 deletions(-) create mode 100644 changelog/fragments/1694700201-gpg-unreachable-url-fix.yaml diff --git a/changelog/fragments/1694700201-gpg-unreachable-url-fix.yaml b/changelog/fragments/1694700201-gpg-unreachable-url-fix.yaml new file mode 100644 index 00000000000..42c8945cb91 --- /dev/null +++ b/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 diff --git a/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier.go b/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier.go index 0326fbb6f38..c0b9cce26e9 100644 --- a/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier.go +++ b/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier.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/internal/pkg/agent/errors" + "github.com/elastic/elastic-agent/pkg/core/logger" ) // Verifier is a verifier with a predefined set of verifiers. @@ -17,27 +18,31 @@ import ( // 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 @@ -45,18 +50,15 @@ func (e *Verifier) Verify(a artifact.Artifact, version string, skipDefaultPgp bo 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 } diff --git a/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier_test.go b/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier_test.go index 393e28e3a63..088c1a29b6d 100644 --- a/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier_test.go +++ b/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) diff --git a/internal/pkg/agent/application/upgrade/artifact/download/fs/verifier.go b/internal/pkg/agent/application/upgrade/artifact/download/fs/verifier.go index 27af0cba205..e75d25dbb7e 100644 --- a/internal/pkg/agent/application/upgrade/artifact/download/fs/verifier.go +++ b/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) { diff --git a/internal/pkg/agent/application/upgrade/artifact/download/http/verifier.go b/internal/pkg/agent/application/upgrade/artifact/download/http/verifier.go index 7fb013d7705..e708fe79e91 100644 --- a/internal/pkg/agent/application/upgrade/artifact/download/http/verifier.go +++ b/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) { diff --git a/internal/pkg/agent/application/upgrade/artifact/download/localremote/verifier.go b/internal/pkg/agent/application/upgrade/artifact/download/localremote/verifier.go index b6313bf30b2..fc2c3a806be 100644 --- a/internal/pkg/agent/application/upgrade/artifact/download/localremote/verifier.go +++ b/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 } diff --git a/internal/pkg/agent/application/upgrade/artifact/download/snapshot/verifier.go b/internal/pkg/agent/application/upgrade/artifact/download/snapshot/verifier.go index 517dd1048b3..302aa93e766 100644 --- a/internal/pkg/agent/application/upgrade/artifact/download/snapshot/verifier.go +++ b/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") } diff --git a/internal/pkg/agent/application/upgrade/artifact/download/verifier.go b/internal/pkg/agent/application/upgrade/artifact/download/verifier.go index 9e6bf24c953..36e0197a0f1 100644 --- a/internal/pkg/agent/application/upgrade/artifact/download/verifier.go +++ b/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 diff --git a/internal/pkg/agent/application/upgrade/step_download.go b/internal/pkg/agent/application/upgrade/step_download.go index a1f3bd79ca2..1bb0d3ac9f8 100644 --- a/internal/pkg/agent/application/upgrade/step_download.go +++ b/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(