Backport elastic-agent/pull/255

x-pack/elastic-agent/CHANGELOG.next.asciidoc

@@ -95,6 +95,7 @@
 - Fix agent configuration overwritten by default fleet config. {pull}29297[29297]
 - Allow agent containers to use basic auth to create a service token. {pull}29651[29651]
 - Fix issue where a failing artifact verification does not remove the bad artifact. {pull}30281[30281]
+- Fix download verification in snapshot builds. {issue}252[252]
 
 ==== New features
 

x-pack/elastic-agent/pkg/agent/application/upgrade/step_download.go

@@ -20,7 +20,7 @@ import (
 	"github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/release"
 )
 
-func (u *Upgrader) downloadArtifact(ctx context.Context, version, sourceURI string) (string, error) {
+func (u *Upgrader) downloadArtifact(ctx context.Context, version, sourceURI string) (_ string, err error) {
 	// do not update source config
 	settings := *u.settings
 	if sourceURI != "" {
@@ -48,13 +48,9 @@ func (u *Upgrader) downloadArtifact(ctx context.Context, version, sourceURI stri
 		return "", errors.New(err, "failed upgrade of agent binary")
 	}
 
-	matches, err := verifier.Verify(agentSpec, version, true)
-	if err != nil {
+	if err := verifier.Verify(agentSpec, version); err != nil {
 		return "", errors.New(err, "failed verification of agent binary")
 	}
-	if !matches {
-		return "", errors.New("failed verification of agent binary, hash does not match", errors.TypeSecurity)
-	}
 
 	return path, nil
 }

x-pack/elastic-agent/pkg/agent/operation/common_test.go

@@ -148,8 +148,8 @@ var _ download.Downloader = &DummyDownloader{}
 
 type DummyVerifier struct{}
 
-func (*DummyVerifier) Verify(_ program.Spec, _ string, _ bool) (bool, error) {
-	return true, nil
+func (*DummyVerifier) Verify(_ program.Spec, _ string) error {
+	return nil
 }
 
 var _ download.Verifier = &DummyVerifier{}

x-pack/elastic-agent/pkg/agent/operation/operation_verify.go

@@ -66,18 +66,11 @@ func (o *operationVerify) Run(_ context.Context, application Application) (err e
 		}
 	}()
 
-	isVerified, err := o.verifier.Verify(o.program.Spec(), o.program.Version(), true)
-	if err != nil {
+	if err := o.verifier.Verify(o.program.Spec(), o.program.Version()); err != nil {
 		return errors.New(err,
 			fmt.Sprintf("operation '%s' failed to verify %s.%s", o.Name(), o.program.BinaryName(), o.program.Version()),
 			errors.TypeSecurity)
 	}
 
-	if !isVerified {
-		return errors.New(err,
-			fmt.Sprintf("operation '%s' marked '%s.%s' corrupted", o.Name(), o.program.BinaryName(), o.program.Version()),
-			errors.TypeSecurity)
-	}
-
 	return nil
 }

x-pack/elastic-agent/pkg/artifact/download/composed/verifier.go

@@ -5,6 +5,8 @@
 package composed
 
 import (
+	"errors"
+
 	"github.com/hashicorp/go-multierror"
 
 	"github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/agent/program"
@@ -30,17 +32,25 @@ func NewVerifier(verifiers ...download.Verifier) *Verifier {
 }
 
 // Verify checks the package from configured source.
-func (e *Verifier) Verify(spec program.Spec, version string, removeOnFailure bool) (bool, error) {
+func (e *Verifier) Verify(spec program.Spec, version string) error {
 	var err error
+	var checksumMismatchErr *download.ChecksumMismatchError
+	var invalidSignatureErr *download.InvalidSignatureError
 
 	for _, v := range e.vv {
-		b, e := v.Verify(spec, version, removeOnFailure)
+		e := v.Verify(spec, version)
 		if e == nil {
-			return b, 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
+		}
 	}
 
-	return false, err
+	return err
 }

x-pack/elastic-agent/pkg/artifact/download/composed/verifier_test.go

@@ -18,9 +18,9 @@ type ErrorVerifier struct {
 	called bool
 }
 
-func (d *ErrorVerifier) Verify(spec program.Spec, version string, removeOnFailure bool) (bool, error) {
+func (d *ErrorVerifier) Verify(spec program.Spec, version string) error {
 	d.called = true
-	return false, errors.New("failing")
+	return errors.New("failing")
 }
 
 func (d *ErrorVerifier) Called() bool { return d.called }
@@ -29,21 +29,20 @@ type FailVerifier struct {
 	called bool
 }
 
-func (d *FailVerifier) Verify(spec program.Spec, version string, removeOnFailure bool) (bool, error) {
+func (d *FailVerifier) Verify(spec program.Spec, version string) error {
 	d.called = true
-	return false, nil
+	return &download.InvalidSignatureError{}
 }
 
 func (d *FailVerifier) Called() bool { return d.called }
 
 type SuccVerifier struct {
-	called          bool
-	removeOnFailure bool
+	called bool
 }
 
-func (d *SuccVerifier) Verify(spec program.Spec, version string, removeOnFailure bool) (bool, error) {
+func (d *SuccVerifier) Verify(spec program.Spec, version string) error {
 	d.called = true
-	return true, nil
+	return nil
 }
 
 func (d *SuccVerifier) Called() bool { return d.called }
@@ -75,9 +74,9 @@ func TestVerifier(t *testing.T) {
 
 	for _, tc := range testCases {
 		d := NewVerifier(tc.verifiers[0], tc.verifiers[1], tc.verifiers[2])
-		r, _ := d.Verify(program.Spec{Name: "a", Cmd: "a", Artifact: "a/a"}, "b", true)
+		err := d.Verify(program.Spec{Name: "a", Cmd: "a", Artifact: "a/a"}, "b")
 
-		assert.Equal(t, tc.expectedResult, r)
+		assert.Equal(t, tc.expectedResult, err == nil)
 
 		assert.True(t, tc.checkFunc(tc.verifiers))
 	}

x-pack/elastic-agent/pkg/artifact/download/fs/verifier.go

@@ -5,39 +5,32 @@
 package fs
 
 import (
-	"bufio"
-	"bytes"
-	"crypto/sha512"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"strings"
-
-	"github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/agent/program"
-
-	"golang.org/x/crypto/openpgp"
 
 	"github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/agent/errors"
+	"github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/agent/program"
 	"github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/artifact"
+	"github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/artifact/download"
 )
 
 const (
-	ascSuffix    = ".asc"
-	sha512Length = 128
+	ascSuffix = ".asc"
 )
 
-// Verifier verifies a downloaded package by comparing with public ASC
-// file from elastic.co website.
+// Verifier verifies an artifact's GPG signature as read from the filesystem.
+// The signature is validated against Elastic's public GPG key that is
+// embedded into Elastic Agent.
 type Verifier struct {
 	config        *artifact.Config
 	pgpBytes      []byte
 	allowEmptyPgp bool
 }
 
-// NewVerifier create a verifier checking downloaded package on preconfigured
-// location agains a key stored on elastic.co website.
+// NewVerifier creates a verifier checking downloaded package on preconfigured
+// location against a key stored on elastic.co website.
 func NewVerifier(config *artifact.Config, allowEmptyPgp bool, pgp []byte) (*Verifier, error) {
 	if len(pgp) == 0 && !allowEmptyPgp {
 		return nil, errors.New("expecting PGP but retrieved none", errors.TypeSecurity)
@@ -53,106 +46,50 @@ func NewVerifier(config *artifact.Config, allowEmptyPgp bool, pgp []byte) (*Veri
 }
 
 // Verify checks downloaded package on preconfigured
-// location agains a key stored on elastic.co website.
-func (v *Verifier) Verify(spec program.Spec, version string, removeOnFailure bool) (isMatch bool, err error) {
+// location against a key stored on elastic.co website.
+func (v *Verifier) Verify(spec program.Spec, version string) error {
 	filename, err := artifact.GetArtifactName(spec, version, v.config.OS(), v.config.Arch())
 	if err != nil {
-		return false, errors.New(err, "retrieving package name")
+		return errors.New(err, "retrieving package name")
 	}
 
 	fullPath := filepath.Join(v.config.TargetDirectory, filename)
-	defer func() {
-		if removeOnFailure && (!isMatch || err != nil) {
-			// remove bits so they can be redownloaded
+
+	if err = download.VerifySHA512Hash(fullPath); err != nil {
+		var checksumMismatchErr *download.ChecksumMismatchError
+		if errors.As(err, &checksumMismatchErr) {
 			os.Remove(fullPath)
 			os.Remove(fullPath + ".sha512")
-			os.Remove(fullPath + ".asc")
 		}
-	}()
-
-	if isMatch, err := v.verifyHash(filename, fullPath); !isMatch || err != nil {
-		return isMatch, err
+		return err
 	}
 
-	return v.verifyAsc(filename, fullPath)
-}
-
-func (v *Verifier) verifyHash(filename, fullPath string) (bool, error) {
-	hashFilePath := fullPath + ".sha512"
-	hashFileHandler, err := os.Open(hashFilePath)
-	if err != nil {
-		return false, err
-	}
-	defer hashFileHandler.Close()
-
-	// get hash
-	// content of a file is in following format
-	// hash  filename
-	var expectedHash string
-	scanner := bufio.NewScanner(hashFileHandler)
-	for scanner.Scan() {
-		line := strings.TrimSpace(scanner.Text())
-		if !strings.HasSuffix(line, filename) {
-			continue
-		}
-
-		if len(line) > sha512Length {
-			expectedHash = strings.TrimSpace(line[:sha512Length])
+	if err = v.verifyAsc(fullPath); err != nil {
+		var invalidSignatureErr *download.InvalidSignatureError
+		if errors.As(err, &invalidSignatureErr) {
+			os.Remove(fullPath + ".asc")
 		}
+		return err
 	}
 
-	if expectedHash == "" {
-		return false, fmt.Errorf("hash for '%s' not found in '%s'", filename, hashFilePath)
-	}
-
-	// compute file hash
-	fileReader, err := os.OpenFile(fullPath, os.O_RDONLY, 0666)
-	if err != nil {
-		return false, errors.New(err, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, fullPath))
-	}
-	defer fileReader.Close()
-
-	hash := sha512.New()
-	if _, err := io.Copy(hash, fileReader); err != nil {
-		return false, err
-	}
-	computedHash := fmt.Sprintf("%x", hash.Sum(nil))
-
-	return expectedHash == computedHash, nil
+	return nil
 }
 
-func (v *Verifier) verifyAsc(filename, fullPath string) (bool, error) {
+func (v *Verifier) verifyAsc(fullPath string) error {
 	if len(v.pgpBytes) == 0 {
 		// no pgp available skip verification process
-		return true, nil
+		return nil
 	}
 
 	ascBytes, err := v.getPublicAsc(fullPath)
 	if err != nil && v.allowEmptyPgp {
 		// asc not available but we allow empty for dev use-case
-		return true, nil
+		return nil
 	} else if err != nil {
-		return false, err
-	}
-
-	pubkeyReader := bytes.NewReader(v.pgpBytes)
-	ascReader := bytes.NewReader(ascBytes)
-	fileReader, err := os.OpenFile(fullPath, os.O_RDONLY, 0666)
-	if err != nil {
-		return false, errors.New(err, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, fullPath))
-	}
-	defer fileReader.Close()
-
-	keyring, err := openpgp.ReadArmoredKeyRing(pubkeyReader)
-	if err != nil {
-		return false, errors.New(err, "read armored key ring", errors.TypeSecurity)
-	}
-	_, err = openpgp.CheckArmoredDetachedSignature(keyring, fileReader, ascReader)
-	if err != nil {
-		return false, errors.New(err, "check detached signature", errors.TypeSecurity)
+		return err
 	}
 
-	return true, nil
+	return download.VerifyGPGSignature(fullPath, ascBytes, v.pgpBytes)
 }
 
 func (v *Verifier) getPublicAsc(fullPath string) ([]byte, error) {