[8.9](backport #3101) [Integration Test] Upgrade failed default verif…

…ication test (#3194) * [Integration Test] Upgrade failed default verification test (#3101) ## What does this PR do? Close #3001 This PR adds an integration test for use case when default option of package verification fails and we need to try remote pgp published at https://artifacts.elastic.co/GPG-KEY-elastic-agent We test this by introducing a flag `--skip-default-pgp` which disables checks with embedded key and providing corrupted key using `--pgp`. --------- Co-authored-by: Paolo Chilà <[email protected]> (cherry picked from commit 2c9e581) # Conflicts: # internal/pkg/agent/application/actions/handlers/handler_action_upgrade.go # internal/pkg/agent/application/actions/handlers/handler_helpers.go # internal/pkg/agent/application/coordinator/coordinator.go # internal/pkg/agent/application/coordinator/coordinator_test.go # internal/pkg/agent/application/coordinator/coordinator_unit_test.go # pkg/control/v1/proto/control_v1.pb.go # pkg/control/v2/cproto/control_v2.pb.go # testing/integration/upgrade_test.go * conflics * removed helpers * removed helpers * added get version for test * added get version for test * added get version for test * tests --------- Co-authored-by: Michal Pristas <[email protected]>
elastic · Aug 25, 2023 · e63710e · e63710e
1 parent c1abed2
commit e63710e
Show file tree

Hide file tree

Showing 29 changed files with 565 additions and 267 deletions.
diff --git a/bin/controller-gen b/bin/controller-gen
diff --git a/bin/golangci-lint b/bin/golangci-lint
diff --git a/bin/kustomize b/bin/kustomize
diff --git a/control_v2.proto b/control_v2.proto
@@ -97,6 +97,11 @@ message UpgradeRequest {
   //
   // If provided Elastic Agent package is checked against these pgp keys as well.
   repeated string pgpBytes = 4;
+
+  // (Optional) Overrides predefined behavior for agent package verification.
+  //
+  // If provided Elastic Agent package embedded PGP key is not checked for signature during upgrade.
+  bool skipDefaultPgp = 5;
 }
 
 // A upgrade response message.

diff --git a/internal/pkg/agent/application/actions/handlers/handler_action_upgrade.go b/internal/pkg/agent/application/actions/handlers/handler_action_upgrade.go
@@ -53,8 +53,7 @@ func (h *Upgrade) Handle(ctx context.Context, a fleetapi.Action, ack acker.Acker
 	}
 	go func() {
 		h.log.Infof("starting upgrade to version %s in background", action.Version)
-		if err := h.coord.Upgrade(asyncCtx, action.Version, action.SourceURI, action, false); err != nil {
-
+		if err := h.coord.Upgrade(asyncCtx, action.Version, action.SourceURI, action, false, false); err != nil {
 			h.log.Errorf("upgrade to version %s failed: %v", action.Version, err)
 			// If context is cancelled in getAsyncContext, the actions are acked there
 			if !errors.Is(asyncCtx.Err(), context.Canceled) {

diff --git a/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go b/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go
@@ -35,7 +35,7 @@ func (u *mockUpgradeManager) Reload(rawConfig *config.Config) error {
 	return nil
 }
 
-func (u *mockUpgradeManager) Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, skipVerifyOverride bool, pgpBytes ...string) (_ reexec.ShutdownCallbackFn, err error) {
+func (u *mockUpgradeManager) Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, skipVerifyOverride bool, skipDefaultPgp bool, pgpBytes ...string) (_ reexec.ShutdownCallbackFn, err error) {
 	select {
 	case <-time.After(2 * time.Second):
 		u.msgChan <- "completed " + version

diff --git a/internal/pkg/agent/application/coordinator/coordinator.go b/internal/pkg/agent/application/coordinator/coordinator.go
@@ -58,7 +58,7 @@ type UpgradeManager interface {
 	Reload(rawConfig *config.Config) error
 
 	// Upgrade upgrades running agent.
-	Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, skipVerifyOverride bool, pgpBytes ...string) (_ reexec.ShutdownCallbackFn, err error)
+	Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, skipVerifyOverride bool, skipDefaultPgp bool, pgpBytes ...string) (_ reexec.ShutdownCallbackFn, err error)
 
 	// Ack is used on startup to check if the agent has upgraded and needs to send an ack for the action
 	Ack(ctx context.Context, acker acker.Acker) error
@@ -266,7 +266,7 @@ func (c *Coordinator) ReExec(callback reexec.ShutdownCallbackFn, argOverrides ..
 
 // Upgrade runs the upgrade process.
 // Called from external goroutines.
-func (c *Coordinator) Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, skipVerifyOverride bool, pgpBytes ...string) error {
+func (c *Coordinator) Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, skipVerifyOverride bool, skipDefaultPgp bool, pgpBytes ...string) error {
 	// early check outside of upgrader before overridding the state
 	if !c.upgradeMgr.Upgradeable() {
 		return ErrNotUpgradable
@@ -299,8 +299,10 @@ func (c *Coordinator) Upgrade(ctx context.Context, version string, sourceURI str
 	}
 
 	// override the overall state to upgrading until the re-execution is complete
+
 	c.state.SetOverrideState(agentclient.Upgrading, fmt.Sprintf("Upgrading to version %s", version))
-	cb, err := c.upgradeMgr.Upgrade(ctx, version, sourceURI, action, skipVerifyOverride, pgpBytes...)
+	cb, err := c.upgradeMgr.Upgrade(ctx, version, sourceURI, action, skipVerifyOverride, skipDefaultPgp, pgpBytes...)
+
 	if err != nil {
 		c.state.ClearOverrideState()
 		return err

diff --git a/internal/pkg/agent/application/coordinator/coordinator_test.go b/internal/pkg/agent/application/coordinator/coordinator_test.go
@@ -462,7 +462,7 @@ func TestCoordinator_Upgrade(t *testing.T) {
 	require.NoError(t, err)
 	cfgMgr.Config(ctx, cfg)
 
-	err = coord.Upgrade(ctx, "9.0.0", "", nil, true)
+	err = coord.Upgrade(ctx, "9.0.0", "", nil, true, false)
 	require.ErrorIs(t, err, ErrNotUpgradable)
 	cancel()
 
@@ -570,7 +570,7 @@ func (f *fakeUpgradeManager) Reload(_ *config.Config) error {
 	return nil
 }
 
-func (f *fakeUpgradeManager) Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, skipVerifyOverride bool, pgpBytes ...string) (_ reexec.ShutdownCallbackFn, err error) {
+func (f *fakeUpgradeManager) Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, skipVerifyOverride bool, skipDefaultPgp bool, pgpBytes ...string) (_ reexec.ShutdownCallbackFn, err error) {
 	return func() error { return nil }, nil
 }
 

diff --git a/internal/pkg/agent/application/coordinator/mocks/upgrade_manager.go b/internal/pkg/agent/application/coordinator/mocks/upgrade_manager.go
diff --git a/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier.go b/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier.go
@@ -31,13 +31,13 @@ func NewVerifier(verifiers ...download.Verifier) *Verifier {
 }
 
 // Verify checks the package from configured source.
-func (e *Verifier) Verify(a artifact.Artifact, version string, pgpBytes ...string) error {
+func (e *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, pgpBytes...)
+		e := v.Verify(a, version, skipDefaultPgp, pgpBytes...)
 		if e == nil {
 			// Success
 			return nil

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
@@ -18,7 +18,7 @@ type ErrorVerifier struct {
 	called bool
 }
 
-func (d *ErrorVerifier) Verify(a artifact.Artifact, version string, _ ...string) error {
+func (d *ErrorVerifier) Verify(a artifact.Artifact, version string, _ bool, _ ...string) error {
 	d.called = true
 	return errors.New("failing")
 }
@@ -29,7 +29,7 @@ type FailVerifier struct {
 	called bool
 }
 
-func (d *FailVerifier) Verify(a artifact.Artifact, version string, _ ...string) error {
+func (d *FailVerifier) Verify(a artifact.Artifact, version string, _ bool, _ ...string) error {
 	d.called = true
 	return &download.InvalidSignatureError{}
 }
@@ -40,7 +40,7 @@ type SuccVerifier struct {
 	called bool
 }
 
-func (d *SuccVerifier) Verify(a artifact.Artifact, version string, _ ...string) error {
+func (d *SuccVerifier) Verify(a artifact.Artifact, version string, _ bool, _ ...string) error {
 	d.called = true
 	return nil
 }
@@ -74,7 +74,7 @@ func TestVerifier(t *testing.T) {
 
 	for _, tc := range testCases {
 		d := NewVerifier(tc.verifiers[0], tc.verifiers[1], tc.verifiers[2])
-		err := d.Verify(artifact.Artifact{Name: "a", Cmd: "a", Artifact: "a/a"}, "b")
+		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
@@ -63,7 +63,7 @@ func NewVerifier(log *logger.Logger, config *artifact.Config, allowEmptyPgp bool
 
 // Verify checks downloaded package on preconfigured
 // location against a key stored on elastic.co website.
-func (v *Verifier) Verify(a artifact.Artifact, version string, pgpBytes ...string) error {
+func (v *Verifier) Verify(a artifact.Artifact, version string, skipDefaultPgp bool, pgpBytes ...string) error {
 	filename, err := artifact.GetArtifactName(a, version, v.config.OS(), v.config.Arch())
 	if err != nil {
 		return errors.New(err, "retrieving package name")
@@ -80,7 +80,7 @@ func (v *Verifier) Verify(a artifact.Artifact, version string, pgpBytes ...strin
 		return err
 	}
 
-	if err = v.verifyAsc(fullPath, pgpBytes...); err != nil {
+	if err = v.verifyAsc(fullPath, skipDefaultPgp, pgpBytes...); err != nil {
 		var invalidSignatureErr *download.InvalidSignatureError
 		if errors.As(err, &invalidSignatureErr) {
 			os.Remove(fullPath + ".asc")
@@ -109,9 +109,9 @@ func (v *Verifier) Reload(c *artifact.Config) error {
 	return nil
 }
 
-func (v *Verifier) verifyAsc(fullPath string, pgpSources ...string) error {
+func (v *Verifier) verifyAsc(fullPath string, skipDefaultPgp bool, pgpSources ...string) error {
 	var pgpBytes [][]byte
-	if len(v.pgpBytes) > 0 {
+	if len(v.pgpBytes) > 0 && !skipDefaultPgp {
 		v.log.Infof("Default PGP being appended")
 		pgpBytes = append(pgpBytes, v.pgpBytes)
 	}

diff --git a/internal/pkg/agent/application/upgrade/artifact/download/fs/verifier_test.go b/internal/pkg/agent/application/upgrade/artifact/download/fs/verifier_test.go
@@ -70,7 +70,7 @@ func TestFetchVerify(t *testing.T) {
 	// first download verify should fail:
 	// download skipped, as invalid package is prepared upfront
 	// verify fails and cleans download
-	err = verifier.Verify(s, version)
+	err = verifier.Verify(s, version, false)
 	var checksumErr *download.ChecksumMismatchError
 	assert.ErrorAs(t, err, &checksumErr)
 
@@ -93,7 +93,7 @@ func TestFetchVerify(t *testing.T) {
 	_, err = os.Stat(hashTargetFilePath)
 	assert.NoError(t, err)
 
-	err = verifier.Verify(s, version)
+	err = verifier.Verify(s, version, false)
 	assert.NoError(t, err)
 
 	// Enable GPG signature validation.
@@ -113,7 +113,7 @@ func TestFetchVerify(t *testing.T) {
 
 	// Missing .asc file.
 	{
-		err = verifier.Verify(s, version)
+		err = verifier.Verify(s, version, false)
 		require.Error(t, err)
 
 		// Don't delete these files when GPG validation failure.
@@ -126,7 +126,7 @@ func TestFetchVerify(t *testing.T) {
 		err = ioutil.WriteFile(targetFilePath+".asc", []byte("bad sig"), 0o600)
 		require.NoError(t, err)
 
-		err = verifier.Verify(s, version)
+		err = verifier.Verify(s, version, false)
 		var invalidSigErr *download.InvalidSignatureError
 		assert.ErrorAs(t, err, &invalidSigErr)
 
@@ -211,7 +211,7 @@ func TestVerify(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	err = testVerifier.Verify(beatSpec, version)
+	err = testVerifier.Verify(beatSpec, version, false)
 	require.NoError(t, err)
 
 	os.Remove(artifact)

diff --git a/internal/pkg/agent/application/upgrade/artifact/download/http/elastic_test.go b/internal/pkg/agent/application/upgrade/artifact/download/http/elastic_test.go
@@ -127,7 +127,7 @@ func TestVerify(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			err = testVerifier.Verify(beatSpec, version)
+			err = testVerifier.Verify(beatSpec, version, false)
 			require.NoError(t, err)
 
 			os.Remove(artifact)

diff --git a/internal/pkg/agent/application/upgrade/artifact/download/http/verifier.go b/internal/pkg/agent/application/upgrade/artifact/download/http/verifier.go
@@ -5,13 +5,15 @@
 package http
 
 import (
+	"context"
 	"fmt"
 	"io/ioutil"
 	"net/http"
 	"net/url"
 	"os"
 	"path"
 	"strings"
+	"time"
 
 	"github.com/elastic/elastic-agent-libs/transport/httpcommon"
 
@@ -82,7 +84,7 @@ func (v *Verifier) Reload(c *artifact.Config) error {
 
 // Verify checks downloaded package on preconfigured
 // location against a key stored on elastic.co website.
-func (v *Verifier) Verify(a artifact.Artifact, version string, pgpBytes ...string) error {
+func (v *Verifier) Verify(a artifact.Artifact, version string, skipDefaultPgp bool, pgpBytes ...string) error {
 	fullPath, err := artifact.GetArtifactPath(a, version, v.config.OS(), v.config.Arch(), v.config.TargetDirectory)
 	if err != nil {
 		return errors.New(err, "retrieving package path")
@@ -97,7 +99,7 @@ func (v *Verifier) Verify(a artifact.Artifact, version string, pgpBytes ...strin
 		return err
 	}
 
-	if err = v.verifyAsc(a, version, pgpBytes...); err != nil {
+	if err = v.verifyAsc(a, version, skipDefaultPgp, pgpBytes...); err != nil {
 		var invalidSignatureErr *download.InvalidSignatureError
 		if errors.As(err, &invalidSignatureErr) {
 			os.Remove(fullPath + ".asc")
@@ -108,9 +110,9 @@ func (v *Verifier) Verify(a artifact.Artifact, version string, pgpBytes ...strin
 	return nil
 }
 
-func (v *Verifier) verifyAsc(a artifact.Artifact, version string, pgpSources ...string) error {
+func (v *Verifier) verifyAsc(a artifact.Artifact, version string, skipDefaultPgp bool, pgpSources ...string) error {
 	var pgpBytes [][]byte
-	if len(v.pgpBytes) > 0 {
+	if len(v.pgpBytes) > 0 && !skipDefaultPgp {
 		v.log.Infof("Default PGP being appended")
 		pgpBytes = append(pgpBytes, v.pgpBytes)
 	}
@@ -194,7 +196,14 @@ func (v *Verifier) composeURI(filename, artifactName string) (string, error) {
 }
 
 func (v *Verifier) getPublicAsc(sourceURI string) ([]byte, error) {
-	resp, err := v.client.Get(sourceURI)
+	ctx, cancelFn := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancelFn()
+	// Change NewRequest to NewRequestWithContext and pass context it
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, sourceURI, nil)
+	if err != nil {
+		return nil, errors.New(err, "failed create request for loading public key", errors.TypeNetwork, errors.M(errors.MetaKeyURI, sourceURI))
+	}
+	resp, err := http.DefaultClient.Do(req)
 	if err != nil {
 		return nil, errors.New(err, "failed loading public key", errors.TypeNetwork, errors.M(errors.MetaKeyURI, sourceURI))
 	}

diff --git a/internal/pkg/agent/application/upgrade/artifact/download/snapshot/verifier.go b/internal/pkg/agent/application/upgrade/artifact/download/snapshot/verifier.go
@@ -37,8 +37,8 @@ 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, pgpBytes ...string) error {
-	return e.verifier.Verify(a, version, pgpBytes...)
+func (e *Verifier) Verify(a artifact.Artifact, version string, skipDefaultPgp bool, pgpBytes ...string) error {
+	return e.verifier.Verify(a, version, skipDefaultPgp, pgpBytes...)
 }
 
 func (e *Verifier) Reload(c *artifact.Config) error {

diff --git a/internal/pkg/agent/application/upgrade/artifact/download/verifier.go b/internal/pkg/agent/application/upgrade/artifact/download/verifier.go
@@ -7,6 +7,7 @@ package download
 import (
 	"bufio"
 	"bytes"
+	"context"
 	"crypto/sha512"
 	"encoding/hex"
 	"fmt"
@@ -17,6 +18,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"time"
 
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
 
@@ -63,7 +65,7 @@ type Verifier interface {
 	// *download.ChecksumMismatchError. And if the GPG signature is invalid then
 	// Verify returns a *download.InvalidSignatureError. Use errors.As() to
 	// check error types.
-	Verify(a artifact.Artifact, version string, pgpBytes ...string) error
+	Verify(a artifact.Artifact, version string, skipDefaultPgp bool, pgpBytes ...string) error
 }
 
 // VerifySHA512Hash checks that a sidecar file containing a sha512 checksum
@@ -195,7 +197,14 @@ func fetchPgpFromURI(uri string, client http.Client) ([]byte, error) {
 		return nil, err
 	}
 
-	resp, err := client.Get(uri)
+	ctx, cancelFn := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancelFn()
+	// Change NewRequest to NewRequestWithContext and pass context it
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, uri, nil)
+	if err != nil {
+		return nil, err
+	}
+	resp, err := http.DefaultClient.Do(req)
 	if err != nil {
 		return nil, err
 	}

diff --git a/internal/pkg/agent/application/upgrade/step_download.go b/internal/pkg/agent/application/upgrade/step_download.go
@@ -33,7 +33,7 @@ const (
 	defaultUpgradeFallbackPGP = "https://artifacts.elastic.co/GPG-KEY-elastic-agent"
 )
 
-func (u *Upgrader) downloadArtifact(ctx context.Context, version, sourceURI string, skipVerifyOverride bool, pgpBytes ...string) (_ string, err error) {
+func (u *Upgrader) downloadArtifact(ctx context.Context, version, sourceURI string, skipVerifyOverride bool, skipDefaultPgp bool, pgpBytes ...string) (_ string, err error) {
 	span, ctx := apm.StartSpan(ctx, "downloadArtifact", "app.internal")
 	defer func() {
 		apm.CaptureError(ctx, err).Send()
@@ -81,7 +81,7 @@ func (u *Upgrader) downloadArtifact(ctx context.Context, version, sourceURI stri
 		return "", errors.New(err, "initiating verifier")
 	}
 
-	if err := verifier.Verify(agentArtifact, parsedVersion.VersionWithPrerelease(), pgpBytes...); err != nil {
+	if err := verifier.Verify(agentArtifact, parsedVersion.VersionWithPrerelease(), skipDefaultPgp, pgpBytes...); err != nil {
 		return "", errors.New(err, "failed verification of agent binary")
 	}
 

diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go
@@ -108,7 +108,7 @@ func (u *Upgrader) Upgradeable() bool {
 }
 
 // Upgrade upgrades running agent, function returns shutdown callback that must be called by reexec.
-func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, skipVerifyOverride bool, pgpBytes ...string) (_ reexec.ShutdownCallbackFn, err error) {
+func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, skipVerifyOverride bool, skipDefaultPgp bool, pgpBytes ...string) (_ reexec.ShutdownCallbackFn, err error) {
 	u.log.Infow("Upgrading agent", "version", version, "source_uri", sourceURI)
 	span, ctx := apm.StartSpan(ctx, "upgrade", "app.internal")
 	defer span.End()
@@ -119,7 +119,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string
 	}
 
 	sourceURI = u.sourceURI(sourceURI)
-	archivePath, err := u.downloadArtifact(ctx, version, sourceURI, skipVerifyOverride, pgpBytes...)
+	archivePath, err := u.downloadArtifact(ctx, version, sourceURI, skipVerifyOverride, skipDefaultPgp, pgpBytes...)
 	if err != nil {
 		// Run the same pre-upgrade cleanup task to get rid of any newly downloaded files
 		// This may have an issue if users are upgrading to the same version number.