From a72957fda3672af19b2e7d1f1be8ebc5b6c798e7 Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Fri, 20 Sep 2024 12:57:40 -0400 Subject: [PATCH] Fix bad error handling in api key auth (#3935) * Fix bad error handling in api key auth. * Don't return so much information. * bug-fix * Remove un-need changes. (cherry picked from commit 28e96bfe06af16c6fe29f036a90047e37266d054) --- ...-Fix-authentication-return-error-code.yaml | 33 +++++++++++++++++++ internal/pkg/apikey/auth.go | 20 ++++++++--- internal/pkg/apikey/auth_test.go | 31 +++++++++++++++-- 3 files changed, 77 insertions(+), 7 deletions(-) create mode 100644 changelog/fragments/1726845328-Fix-authentication-return-error-code.yaml diff --git a/changelog/fragments/1726845328-Fix-authentication-return-error-code.yaml b/changelog/fragments/1726845328-Fix-authentication-return-error-code.yaml new file mode 100644 index 000000000..78ab16f5f --- /dev/null +++ b/changelog/fragments/1726845328-Fix-authentication-return-error-code.yaml @@ -0,0 +1,33 @@ +# 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 authentication return error code + +# 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: | + A non-401 error code from elasticsearch will no longer result in a 401 error code being returned to the caller. + +# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc. +component: + +# 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/fleet-server/pull/3935 + +# 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/fleet-server/issues/3929 diff --git a/internal/pkg/apikey/auth.go b/internal/pkg/apikey/auth.go index 6dbb20d2b..2c7c2d368 100644 --- a/internal/pkg/apikey/auth.go +++ b/internal/pkg/apikey/auth.go @@ -9,9 +9,12 @@ import ( "encoding/json" "errors" "fmt" + "net/http" "github.com/elastic/go-elasticsearch/v8" "github.com/elastic/go-elasticsearch/v8/esapi" + + "github.com/elastic/fleet-server/v7/internal/pkg/es" ) var ( @@ -33,7 +36,7 @@ type SecurityInfo struct { // Authenticate will return the SecurityInfo associated with the APIKey (retrieved from Elasticsearch). // Note: Prefer the bulk wrapper on this API -func (k APIKey) Authenticate(ctx context.Context, es *elasticsearch.Client) (*SecurityInfo, error) { +func (k APIKey) Authenticate(ctx context.Context, client *elasticsearch.Client) (*SecurityInfo, error) { token := fmt.Sprintf("%s%s", authPrefix, k.Token()) @@ -41,7 +44,7 @@ func (k APIKey) Authenticate(ctx context.Context, es *elasticsearch.Client) (*Se Header: map[string][]string{AuthKey: []string{token}}, } - res, err := req.Do(ctx, es) + res, err := req.Do(ctx, client) if err != nil { return nil, fmt.Errorf("apikey auth request %s: %w", k.ID, err) @@ -52,11 +55,18 @@ func (k APIKey) Authenticate(ctx context.Context, es *elasticsearch.Client) (*Se } if res.IsError() { - returnError := ErrUnauthorized - if res.StatusCode == 429 { + var returnError error + switch res.StatusCode { + case http.StatusUnauthorized: + returnError = ErrUnauthorized + case http.StatusTooManyRequests: returnError = ErrElasticsearchAuthLimit } - return nil, fmt.Errorf("%w: %w", returnError, fmt.Errorf("apikey auth response %s: %s", k.ID, res.String())) + if returnError != nil { + return nil, fmt.Errorf("%w: %w", returnError, fmt.Errorf("apikey auth response %s: %s", k.ID, res.String())) + } + // body is not parsed to not give the caller too much information + return nil, es.TranslateError(res.StatusCode, nil) } var info SecurityInfo diff --git a/internal/pkg/apikey/auth_test.go b/internal/pkg/apikey/auth_test.go index 994e5c7c9..9b58efe87 100644 --- a/internal/pkg/apikey/auth_test.go +++ b/internal/pkg/apikey/auth_test.go @@ -7,6 +7,7 @@ package apikey import ( "context" "encoding/base64" + "fmt" "net/http" "testing" @@ -32,15 +33,41 @@ func setup(t *testing.T, statusCode int) (context.Context, *APIKey, *elasticsear } func TestAuth429(t *testing.T) { - ctx, apiKey, mockES := setup(t, 429) + ctx, apiKey, mockES := setup(t, http.StatusTooManyRequests) _, err := apiKey.Authenticate(ctx, mockES) assert.Equal(t, "elasticsearch auth limit: apikey auth response foo: [429 Too Many Requests] ", err.Error()) } func TestAuth401(t *testing.T) { - ctx, apiKey, mockES := setup(t, 401) + ctx, apiKey, mockES := setup(t, http.StatusUnauthorized) _, err := apiKey.Authenticate(ctx, mockES) assert.Equal(t, "unauthorized: apikey auth response foo: [401 Unauthorized] ", err.Error()) } + +func TestAuthOtherErrors(t *testing.T) { + scenarios := []struct { + StatusCode int + }{ + {StatusCode: http.StatusBadRequest}, + // 401 is handled in TestAuth401 + {StatusCode: http.StatusForbidden}, + {StatusCode: http.StatusNotFound}, + {StatusCode: http.StatusMethodNotAllowed}, + {StatusCode: http.StatusConflict}, + // 429 is handled in TestAuth429 + {StatusCode: http.StatusInternalServerError}, + {StatusCode: http.StatusBadGateway}, + {StatusCode: http.StatusServiceUnavailable}, + {StatusCode: http.StatusGatewayTimeout}, + } + for _, scenario := range scenarios { + t.Run(fmt.Sprintf("%d", scenario.StatusCode), func(t *testing.T) { + ctx, apiKey, mockES := setup(t, scenario.StatusCode) + _, err := apiKey.Authenticate(ctx, mockES) + + assert.Equal(t, fmt.Sprintf("elastic fail %d", scenario.StatusCode), err.Error()) + }) + } +}