From e8dca50386c3c99b779deec4a0779cf3416aeb65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paolo=20Chil=C3=A0?= Date: Wed, 4 Oct 2023 15:56:09 +0200 Subject: [PATCH] Revert "[Feature] Secondary fallback for package signature verification (#3453)" (#3509) This reverts commit cdca2116142efde46775cf8e6dd77051238f110b. --- ...ck-for-package-signature-verification.yaml | 31 ----- internal/pkg/agent/application/application.go | 2 +- .../pkg/agent/application/managed_mode.go | 53 ++++---- .../upgrade/artifact/download/fs/verifier.go | 2 +- .../artifact/download/http/verifier.go | 2 +- .../upgrade/artifact/download/verifier.go | 16 +-- .../artifact/download/verifier_test.go | 127 ------------------ .../application/upgrade/step_download.go | 30 +---- .../application/upgrade/step_download_test.go | 35 ++--- .../pkg/agent/application/upgrade/upgrade.go | 21 +-- internal/pkg/remote/client.go | 4 - 11 files changed, 45 insertions(+), 278 deletions(-) delete mode 100644 changelog/fragments/1695289867-Secondary-fallback-for-package-signature-verification.yaml delete mode 100644 internal/pkg/agent/application/upgrade/artifact/download/verifier_test.go diff --git a/changelog/fragments/1695289867-Secondary-fallback-for-package-signature-verification.yaml b/changelog/fragments/1695289867-Secondary-fallback-for-package-signature-verification.yaml deleted file mode 100644 index 07c8c4e5cf8..00000000000 --- a/changelog/fragments/1695289867-Secondary-fallback-for-package-signature-verification.yaml +++ /dev/null @@ -1,31 +0,0 @@ -# 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: feature - -# Change summary; a 80ish characters long description of the change. -summary: Secondary fallback for package signature verification - -# Long description; in case the summary is not enough to describe the change -# this field accommodate a description without length limits. -description: Ability to upgrade securely in Air gapped environment where fleet server is the only reachable URI. - -# Affected component; a word indicating the component this changeset affects. -component: elastic-agent - -# PR number; 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/3453 - -# Issue number; 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/3264 diff --git a/internal/pkg/agent/application/application.go b/internal/pkg/agent/application/application.go index 6ca33c35c05..dca8973f79e 100644 --- a/internal/pkg/agent/application/application.go +++ b/internal/pkg/agent/application/application.go @@ -161,7 +161,7 @@ func New( EndpointSignedComponentModifier(), ) - managed, err = newManagedConfigManager(ctx, log, agentInfo, cfg, store, runtime, fleetInitTimeout, upgrader) + managed, err = newManagedConfigManager(ctx, log, agentInfo, cfg, store, runtime, fleetInitTimeout) if err != nil { return nil, nil, nil, err } diff --git a/internal/pkg/agent/application/managed_mode.go b/internal/pkg/agent/application/managed_mode.go index 58aebd06451..f29be7fe258 100644 --- a/internal/pkg/agent/application/managed_mode.go +++ b/internal/pkg/agent/application/managed_mode.go @@ -10,7 +10,6 @@ import ( "time" "github.com/elastic/elastic-agent-client/v7/pkg/client" - "github.com/elastic/elastic-agent/internal/pkg/agent/application/actions" "github.com/elastic/elastic-agent/internal/pkg/agent/application/actions/handlers" "github.com/elastic/elastic-agent/internal/pkg/agent/application/coordinator" "github.com/elastic/elastic-agent/internal/pkg/agent/application/dispatcher" @@ -39,18 +38,17 @@ import ( const dispatchFlushInterval = time.Minute * 5 type managedConfigManager struct { - log *logger.Logger - agentInfo *info.AgentInfo - cfg *configuration.Configuration - client *remote.Client - store storage.Store - stateStore *store.StateStore - actionQueue *queue.ActionQueue - dispatcher *dispatcher.ActionDispatcher - runtime *runtime.Manager - coord *coordinator.Coordinator - fleetInitTimeout time.Duration - initialClientSetters []actions.ClientSetter + log *logger.Logger + agentInfo *info.AgentInfo + cfg *configuration.Configuration + client *remote.Client + store storage.Store + stateStore *store.StateStore + actionQueue *queue.ActionQueue + dispatcher *dispatcher.ActionDispatcher + runtime *runtime.Manager + coord *coordinator.Coordinator + fleetInitTimeout time.Duration ch chan coordinator.ConfigChange errCh chan error @@ -64,7 +62,6 @@ func newManagedConfigManager( storeSaver storage.Store, runtime *runtime.Manager, fleetInitTimeout time.Duration, - clientSetters ...actions.ClientSetter, ) (*managedConfigManager, error) { client, err := fleetclient.NewAuthWithConfig(log, cfg.Fleet.AccessAPIKey, cfg.Fleet.Client) if err != nil { @@ -91,19 +88,18 @@ func newManagedConfigManager( } return &managedConfigManager{ - log: log, - agentInfo: agentInfo, - cfg: cfg, - client: client, - store: storeSaver, - stateStore: stateStore, - actionQueue: actionQueue, - dispatcher: actionDispatcher, - runtime: runtime, - fleetInitTimeout: fleetInitTimeout, - ch: make(chan coordinator.ConfigChange), - errCh: make(chan error), - initialClientSetters: clientSetters, + log: log, + agentInfo: agentInfo, + cfg: cfg, + client: client, + store: storeSaver, + stateStore: stateStore, + actionQueue: actionQueue, + dispatcher: actionDispatcher, + runtime: runtime, + fleetInitTimeout: fleetInitTimeout, + ch: make(chan coordinator.ConfigChange), + errCh: make(chan error), }, nil } @@ -200,9 +196,6 @@ func (m *managedConfigManager) Run(ctx context.Context) error { policyChanger.AddSetter(gateway) policyChanger.AddSetter(ack) } - for _, cs := range m.initialClientSetters { - policyChanger.AddSetter(cs) - } // Proxy errors from the gateway to our own channel. gatewayErrorsRunner := runner.Start(context.Background(), func(ctx context.Context) error { 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 263f6ea8bf5..72066129222 100644 --- a/internal/pkg/agent/application/upgrade/artifact/download/fs/verifier.go +++ b/internal/pkg/agent/application/upgrade/artifact/download/fs/verifier.go @@ -124,7 +124,7 @@ func (v *Verifier) verifyAsc(fullPath string, skipDefaultPgp bool, pgpSources .. if len(check) == 0 { continue } - raw, err := download.PgpBytesFromSource(v.log, check, &v.client) + raw, err := download.PgpBytesFromSource(v.log, check, v.client) if err != nil { return err } 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 84f3f6045f0..61992e08816 100644 --- a/internal/pkg/agent/application/upgrade/artifact/download/http/verifier.go +++ b/internal/pkg/agent/application/upgrade/artifact/download/http/verifier.go @@ -125,7 +125,7 @@ func (v *Verifier) verifyAsc(a artifact.Artifact, version string, skipDefaultPgp if len(check) == 0 { continue } - raw, err := download.PgpBytesFromSource(v.log, check, &v.client) + raw, err := download.PgpBytesFromSource(v.log, check, v.client) if err != nil { return err } diff --git a/internal/pkg/agent/application/upgrade/artifact/download/verifier.go b/internal/pkg/agent/application/upgrade/artifact/download/verifier.go index 662367f4909..2c716cb2844 100644 --- a/internal/pkg/agent/application/upgrade/artifact/download/verifier.go +++ b/internal/pkg/agent/application/upgrade/artifact/download/verifier.go @@ -36,7 +36,6 @@ const ( var ( ErrRemotePGPDownloadFailed = errors.New("Remote PGP download failed") ErrInvalidLocation = errors.New("Remote PGP location is invalid") - ErrUnknownPGPSource = errors.New("unknown pgp source") ) // warnLogger is a logger that only needs to implement Warnf, as that is the only functions @@ -181,7 +180,7 @@ func VerifyGPGSignature(file string, asciiArmorSignature, publicKey []byte) erro return nil } -func PgpBytesFromSource(log warnLogger, source string, client HTTPClient) ([]byte, error) { +func PgpBytesFromSource(log warnLogger, source string, client http.Client) ([]byte, error) { if strings.HasPrefix(source, PgpSourceRawPrefix) { return []byte(strings.TrimPrefix(source, PgpSourceRawPrefix)), nil } @@ -190,14 +189,11 @@ func PgpBytesFromSource(log warnLogger, source string, client HTTPClient) ([]byt pgpBytes, err := fetchPgpFromURI(strings.TrimPrefix(source, PgpSourceURIPrefix), client) if errors.Is(err, ErrRemotePGPDownloadFailed) || errors.Is(err, ErrInvalidLocation) { log.Warnf("Skipped remote PGP located at %q because it's unavailable: %v", strings.TrimPrefix(source, PgpSourceURIPrefix), err) - } else if err != nil { - log.Warnf("Failed to fetch remote PGP") } - return pgpBytes, nil } - return nil, ErrUnknownPGPSource + return nil, errors.New("unknown pgp source") } func CheckValidDownloadUri(rawURI string) error { @@ -213,7 +209,7 @@ func CheckValidDownloadUri(rawURI string) error { return nil } -func fetchPgpFromURI(uri string, client HTTPClient) ([]byte, error) { +func fetchPgpFromURI(uri string, client http.Client) ([]byte, error) { if err := CheckValidDownloadUri(uri); err != nil { return nil, err } @@ -225,7 +221,7 @@ func fetchPgpFromURI(uri string, client HTTPClient) ([]byte, error) { if err != nil { return nil, err } - resp, err := client.Do(req) + resp, err := http.DefaultClient.Do(req) if err != nil { return nil, multierror.Append(err, ErrRemotePGPDownloadFailed) } @@ -237,7 +233,3 @@ func fetchPgpFromURI(uri string, client HTTPClient) ([]byte, error) { return ioutil.ReadAll(resp.Body) } - -type HTTPClient interface { - Do(*http.Request) (*http.Response, error) -} diff --git a/internal/pkg/agent/application/upgrade/artifact/download/verifier_test.go b/internal/pkg/agent/application/upgrade/artifact/download/verifier_test.go deleted file mode 100644 index 05ad9a96b91..00000000000 --- a/internal/pkg/agent/application/upgrade/artifact/download/verifier_test.go +++ /dev/null @@ -1,127 +0,0 @@ -// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one -// or more contributor license agreements. Licensed under the Elastic License; -// you may not use this file except in compliance with the Elastic License. - -package download - -import ( - "bytes" - "io" - "net/http" - "testing" - - "github.com/stretchr/testify/require" - - "github.com/elastic/elastic-agent/internal/pkg/agent/errors" - "github.com/elastic/elastic-agent/pkg/core/logger" -) - -func TestPgpBytesFromSource(t *testing.T) { - testCases := []struct { - Name string - Source string - ClientDoErr error - ClientBody []byte - ClientStatus int - - ExpectedPGP []byte - ExpectedErr error - ExpectedLogMessage string - }{ - { - "successful call", - PgpSourceURIPrefix + "https://location/path", - nil, - []byte("pgp-body"), - 200, - []byte("pgp-body"), - nil, - "", - }, - { - "unknown source call", - "https://location/path", - nil, - []byte("pgp-body"), - 200, - nil, - ErrUnknownPGPSource, - "", - }, - { - "invalid location is filtered call", - PgpSourceURIPrefix + "http://location/path", - nil, - []byte("pgp-body"), - 200, - nil, - nil, - "Skipped remote PGP located ", - }, - { - "do error is filtered", - PgpSourceURIPrefix + "https://location/path", - errors.New("error"), - []byte("pgp-body"), - 200, - nil, - nil, - "Skipped remote PGP located", - }, - { - "invalid status code is filtered out", - PgpSourceURIPrefix + "https://location/path", - nil, - []byte("pgp-body"), - 500, - nil, - nil, - "Failed to fetch remote PGP", - }, - { - "invalid status code is filtered out", - PgpSourceURIPrefix + "https://location/path", - nil, - []byte("pgp-body"), - 404, - nil, - nil, - "Failed to fetch remote PGP", - }, - } - - for _, tc := range testCases { - t.Run(tc.Name, func(t *testing.T) { - log, obs := logger.NewTesting(tc.Name) - mockClient := &MockClient{ - DoFunc: func(req *http.Request) (*http.Response, error) { - if tc.ClientDoErr != nil { - return nil, tc.ClientDoErr - } - - return &http.Response{ - StatusCode: tc.ClientStatus, - Body: io.NopCloser(bytes.NewReader(tc.ClientBody)), - }, nil - }, - } - - resPgp, resErr := PgpBytesFromSource(log, tc.Source, mockClient) - require.Equal(t, tc.ExpectedErr, resErr) - require.Equal(t, tc.ExpectedPGP, resPgp) - if tc.ExpectedLogMessage != "" { - logs := obs.FilterMessageSnippet(tc.ExpectedLogMessage) - require.NotEqual(t, 0, logs.Len()) - } - - }) - } -} - -type MockClient struct { - DoFunc func(req *http.Request) (*http.Response, error) -} - -func (m *MockClient) Do(req *http.Request) (*http.Response, error) { - return m.DoFunc(req) -} diff --git a/internal/pkg/agent/application/upgrade/step_download.go b/internal/pkg/agent/application/upgrade/step_download.go index a273460f337..1bb0d3ac9f8 100644 --- a/internal/pkg/agent/application/upgrade/step_download.go +++ b/internal/pkg/agent/application/upgrade/step_download.go @@ -7,7 +7,6 @@ package upgrade import ( "context" "fmt" - "net/url" "os" "strings" "time" @@ -31,8 +30,7 @@ import ( ) const ( - defaultUpgradeFallbackPGP = "https://artifacts.elastic.co/GPG-KEY-elastic-agent" - fleetUpgradeFallbackPGPFormat = "/api/agents/upgrades/%d.%d.%d/pgp-public-key" + defaultUpgradeFallbackPGP = "https://artifacts.elastic.co/GPG-KEY-elastic-agent" ) func (u *Upgrader) downloadArtifact(ctx context.Context, version, sourceURI string, skipVerifyOverride bool, skipDefaultPgp bool, pgpBytes ...string) (_ string, err error) { @@ -42,7 +40,7 @@ func (u *Upgrader) downloadArtifact(ctx context.Context, version, sourceURI stri span.End() }() - pgpBytes = u.appendFallbackPGP(version, pgpBytes) + pgpBytes = appendFallbackPGP(pgpBytes) // do not update source config settings := *u.settings @@ -89,35 +87,13 @@ func (u *Upgrader) downloadArtifact(ctx context.Context, version, sourceURI stri return path, nil } -func (u *Upgrader) appendFallbackPGP(targetVersion string, pgpBytes []string) []string { +func appendFallbackPGP(pgpBytes []string) []string { if pgpBytes == nil { pgpBytes = make([]string, 0, 1) } fallbackPGP := download.PgpSourceURIPrefix + defaultUpgradeFallbackPGP pgpBytes = append(pgpBytes, fallbackPGP) - - // add a secondary fallback if fleet server is configured - u.log.Debugf("Considering fleet server uri for pgp check fallback %q", u.fleetServerURI) - if u.fleetServerURI != "" { - tpv, err := agtversion.ParseVersion(targetVersion) - if err != nil { - // best effort, log failure - u.log.Warnf("failed to parse agent version (%q) for secondary GPG fallback: %v", targetVersion, err) - } else { - secondaryPath, err := url.JoinPath( - u.fleetServerURI, - fmt.Sprintf(fleetUpgradeFallbackPGPFormat, tpv.Major(), tpv.Minor(), tpv.Patch()), - ) - if err != nil { - u.log.Warnf("failed to compose Fleet Server URI: %v", err) - } else { - secondaryFallback := download.PgpSourceURIPrefix + secondaryPath - pgpBytes = append(pgpBytes, secondaryFallback) - } - } - } - return pgpBytes } diff --git a/internal/pkg/agent/application/upgrade/step_download_test.go b/internal/pkg/agent/application/upgrade/step_download_test.go index 330a60f5288..1670eb3e5a6 100644 --- a/internal/pkg/agent/application/upgrade/step_download_test.go +++ b/internal/pkg/agent/application/upgrade/step_download_test.go @@ -7,7 +7,6 @@ package upgrade import ( "context" "fmt" - "strings" "testing" "time" @@ -33,40 +32,22 @@ func (md *mockDownloader) Download(ctx context.Context, agentArtifact artifact.A func TestFallbackIsAppended(t *testing.T) { testCases := []struct { - name string - passedBytes []string - expectedLen int - expectedDefaultIdx int - expectedSecondaryIdx int - fleetServerURI string - targetVersion string + name string + passedBytes []string + expectedLen int }{ - {"nil input", nil, 1, 0, -1, "", ""}, - {"empty input", []string{}, 1, 0, -1, "", ""}, - {"valid input with pgp", []string{"pgp-bytes"}, 2, 1, -1, "", ""}, - {"valid input with pgp and version, no fleet uri", []string{"pgp-bytes"}, 2, 1, -1, "", "1.2.3"}, - {"valid input with pgp and version and fleet uri", []string{"pgp-bytes"}, 3, 1, 2, "some-uri", "1.2.3"}, - {"valid input with pgp and fleet uri no version", []string{"pgp-bytes"}, 2, 1, -1, "some-uri", ""}, + {"nil input", nil, 1}, + {"empty input", []string{}, 1}, + {"valid input", []string{"pgp-bytes"}, 2}, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - l, _ := logger.NewTesting(tc.name) - u := Upgrader{ - fleetServerURI: tc.fleetServerURI, - log: l, - } - res := u.appendFallbackPGP(tc.targetVersion, tc.passedBytes) + res := appendFallbackPGP(tc.passedBytes) // check default fallback is passed and is very last require.NotNil(t, res) require.Equal(t, tc.expectedLen, len(res)) - require.Equal(t, download.PgpSourceURIPrefix+defaultUpgradeFallbackPGP, res[tc.expectedDefaultIdx]) - - if tc.expectedSecondaryIdx >= 0 { - // last element is fleet uri - expectedPgpURI := download.PgpSourceURIPrefix + tc.fleetServerURI + strings.Replace(fleetUpgradeFallbackPGPFormat, "%d.%d.%d", tc.targetVersion, 1) - require.Equal(t, expectedPgpURI, res[len(res)-1]) - } + require.Equal(t, download.PgpSourceURIPrefix+defaultUpgradeFallbackPGP, res[len(res)-1]) }) } } diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index d477537a6f1..d03a55fb336 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -26,7 +26,6 @@ import ( "github.com/elastic/elastic-agent/internal/pkg/agent/errors" "github.com/elastic/elastic-agent/internal/pkg/fleetapi" "github.com/elastic/elastic-agent/internal/pkg/fleetapi/acker" - fleetclient "github.com/elastic/elastic-agent/internal/pkg/fleetapi/client" "github.com/elastic/elastic-agent/internal/pkg/release" "github.com/elastic/elastic-agent/pkg/core/logger" ) @@ -49,11 +48,10 @@ var ErrSameVersion = errors.New("upgrade did not occur because its the same vers // Upgrader performs an upgrade type Upgrader struct { - log *logger.Logger - settings *artifact.Config - agentInfo *info.AgentInfo - upgradeable bool - fleetServerURI string + log *logger.Logger + settings *artifact.Config + agentInfo *info.AgentInfo + upgradeable bool } // IsUpgradeable when agent is installed and running as a service or flag was provided. @@ -73,17 +71,6 @@ func NewUpgrader(log *logger.Logger, settings *artifact.Config, agentInfo *info. } } -// SetClient reloads URI based on up to date fleet client -func (u *Upgrader) SetClient(c fleetclient.Sender) { - if c == nil { - u.log.Debug("client nil, resetting Fleet Server URI") - u.fleetServerURI = "" - } - - u.fleetServerURI = c.URI() - u.log.Debugf("Set client changed URI to %s", u.fleetServerURI) -} - // Reload reloads the artifact configuration for the upgrader. func (u *Upgrader) Reload(rawConfig *config.Config) error { type reloadConfig struct { diff --git a/internal/pkg/remote/client.go b/internal/pkg/remote/client.go index 9a3f3d974d3..078016d1d87 100644 --- a/internal/pkg/remote/client.go +++ b/internal/pkg/remote/client.go @@ -247,10 +247,6 @@ func (c *Client) checkApiVersionHeaders(reqID string, resp *http.Response) { // URI returns the remote URI. func (c *Client) URI() string { host := c.config.GetHosts()[0] - if strings.HasPrefix(host, string(ProtocolHTTPS)+"://") || - strings.HasPrefix(host, string(ProtocolHTTP)+"://") { - return host + "/" + c.config.Path - } return string(c.config.Protocol) + "://" + host + "/" + c.config.Path }