diff --git a/.gitignore b/.gitignore index 17857e88f..54e2faa6a 100644 --- a/.gitignore +++ b/.gitignore @@ -23,5 +23,5 @@ dev-tools/cloud/terraform/*.tfvars* *.swo *.swn -.service_token +.service_token* .kibana_service_token diff --git a/Makefile b/Makefile index f9b582f4e..f55035f88 100644 --- a/Makefile +++ b/Makefile @@ -89,7 +89,7 @@ $(COVER_TARGETS): cover-%: ## - Build a binary with the -cover flag for integrat .PHONY: clean clean: ## - Clean up build artifacts @printf "${CMD_COLOR_ON} Clean up build artifacts\n${CMD_COLOR_OFF}" - rm -rf .service_token .kibana_service_token ./bin/ ./build/ + rm -rf .service_token* .kibana_service_token ./bin/ ./build/ .PHONY: generate generate: ## - Generate schema models @@ -315,7 +315,7 @@ int-docker-start: ## - Start docker envronment for integration tests and wait un .PHONY: int-docker-stop int-docker-stop: ## - Stop docker environment for integration tests @docker compose -f ./dev-tools/integration/docker-compose.yml --env-file ./dev-tools/integration/.env down - @rm -f .service_token + @rm -f .service_token* # Run integration tests with starting/stopping docker .PHONY: test-int diff --git a/changelog/fragments/1701381092-Unauthorized-API-keys-return-401.yaml b/changelog/fragments/1701381092-Unauthorized-API-keys-return-401.yaml new file mode 100644 index 000000000..18cb9d07a --- /dev/null +++ b/changelog/fragments/1701381092-Unauthorized-API-keys-return-401.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: enhancement + +# Change summary; a 80ish characters long description of the change. +summary: Unauthorized API keys return 401 + +# 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; a word indicating the component this changeset affects. +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/owner/repo/1234 + +# 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: 2861 diff --git a/internal/pkg/api/error.go b/internal/pkg/api/error.go index ddba237b4..fd8016a15 100644 --- a/internal/pkg/api/error.go +++ b/internal/pkg/api/error.go @@ -167,7 +167,7 @@ func NewHTTPErrResp(err error) HTTPErrResp { { ErrAgentIdentity, HTTPErrResp{ - http.StatusBadRequest, + http.StatusForbidden, "ErrAgentIdentity", "Agent header contains wrong identifier", zerolog.InfoLevel, @@ -185,7 +185,7 @@ func NewHTTPErrResp(err error) HTTPErrResp { { ErrAgentInactive, HTTPErrResp{ - http.StatusBadRequest, + http.StatusUnauthorized, "ErrAgentInactive", "Agent inactive", zerolog.InfoLevel, @@ -194,7 +194,7 @@ func NewHTTPErrResp(err error) HTTPErrResp { { ErrAPIKeyNotEnabled, HTTPErrResp{ - http.StatusBadRequest, + http.StatusUnauthorized, "ErrAPIKeyNotEnabled", "APIKey not enabled", zerolog.InfoLevel, @@ -240,7 +240,7 @@ func NewHTTPErrResp(err error) HTTPErrResp { { apikey.ErrNoAuthHeader, HTTPErrResp{ - http.StatusBadRequest, + http.StatusUnauthorized, "ErrNoAuthHeader", "no authorization header", zerolog.InfoLevel, @@ -258,7 +258,7 @@ func NewHTTPErrResp(err error) HTTPErrResp { { apikey.ErrUnauthorized, HTTPErrResp{ - http.StatusBadRequest, + http.StatusUnauthorized, "ErrUnauthorized", "unauthorized", zerolog.InfoLevel, @@ -276,7 +276,7 @@ func NewHTTPErrResp(err error) HTTPErrResp { { apikey.ErrInvalidToken, HTTPErrResp{ - http.StatusBadRequest, + http.StatusUnauthorized, "ErrInvalidToken", "token not valid utf8", zerolog.InfoLevel, @@ -285,7 +285,7 @@ func NewHTTPErrResp(err error) HTTPErrResp { { apikey.ErrAPIKeyNotFound, HTTPErrResp{ - http.StatusBadRequest, + http.StatusUnauthorized, "ErrAPIKeyNotFound", "api key not found", zerolog.InfoLevel, diff --git a/internal/pkg/api/handleUpload_test.go b/internal/pkg/api/handleUpload_test.go index a9c5137d5..d69bc3e34 100644 --- a/internal/pkg/api/handleUpload_test.go +++ b/internal/pkg/api/handleUpload_test.go @@ -236,8 +236,8 @@ func TestUploadBeginAuth(t *testing.T) { ExpectStatus int }{ {"Agent ID matching API Key succeeds", true, "abc123", "abc123", http.StatusOK}, - {"Agent ID not matching API Key should reject", true, "oneID", "differentID", http.StatusBadRequest}, - {"Bad auth should reject request", false, "", "IDinDoc", http.StatusBadRequest}, + {"Agent ID not matching API Key should reject", true, "oneID", "differentID", http.StatusForbidden}, + {"Bad auth should reject request", false, "", "IDinDoc", http.StatusUnauthorized}, } for _, tc := range tests { @@ -577,8 +577,8 @@ func TestUploadCompleteRequiresMatchingAuth(t *testing.T) { ExpectStatus int }{ {"Agent ID matching API Key succeeds", true, "abc123", "abc123", http.StatusOK}, - {"Agent ID in File not matching API Key should reject", true, "oneID", "differentID", http.StatusBadRequest}, - {"Bad auth should reject request", false, "", "IDinDoc", http.StatusBadRequest}, + {"Agent ID in File not matching API Key should reject", true, "oneID", "differentID", http.StatusForbidden}, + {"Bad auth should reject request", false, "", "IDinDoc", http.StatusUnauthorized}, } mockUploadID := "abc123" diff --git a/internal/pkg/api/openapi.gen.go b/internal/pkg/api/openapi.gen.go index dc5ebc4df..ce4b90fff 100644 --- a/internal/pkg/api/openapi.gen.go +++ b/internal/pkg/api/openapi.gen.go @@ -802,6 +802,9 @@ type BadRequest = Error // Deadline Error processing request. type Deadline = Error +// Forbidden Error processing request. +type Forbidden = Error + // InternalServerError Error processing request. type InternalServerError = Error diff --git a/internal/pkg/server/fleet_integration_test.go b/internal/pkg/server/fleet_integration_test.go index b43979e8a..48d9658d6 100644 --- a/internal/pkg/server/fleet_integration_test.go +++ b/internal/pkg/server/fleet_integration_test.go @@ -426,10 +426,7 @@ func TestServerUnauthorized(t *testing.T) { t.Fatal(err) } defer res.Body.Close() - diff := cmp.Diff(http.StatusBadRequest, res.StatusCode) - if diff != "" { - t.Fatal(diff) - } + require.Equal(t, http.StatusUnauthorized, res.StatusCode) raw, _ := ioutil.ReadAll(res.Body) var resp api.HTTPErrResp @@ -437,11 +434,8 @@ func TestServerUnauthorized(t *testing.T) { if err != nil { t.Fatal(err) } - diff = cmp.Diff(http.StatusBadRequest, resp.StatusCode) - if diff != "" { - t.Fatal(diff) - } - diff = cmp.Diff("ErrNoAuthHeader", resp.Error) + require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + diff := cmp.Diff("ErrNoAuthHeader", resp.Error) if diff != "" { t.Fatal(diff) } @@ -461,10 +455,7 @@ func TestServerUnauthorized(t *testing.T) { require.NoError(t, err) defer res.Body.Close() - diff := cmp.Diff(http.StatusBadRequest, res.StatusCode) - if diff != "" { - t.Fatal(diff) - } + require.Equal(t, http.StatusUnauthorized, res.StatusCode) raw, _ := ioutil.ReadAll(res.Body) var resp api.HTTPErrResp @@ -472,11 +463,8 @@ func TestServerUnauthorized(t *testing.T) { if err != nil { t.Fatal(err) } - diff = cmp.Diff(400, resp.StatusCode) - if diff != "" { - t.Fatal(diff) - } - diff = cmp.Diff("ErrUnauthorized", resp.Error) + require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + diff := cmp.Diff("ErrUnauthorized", resp.Error) if diff != "" { t.Fatal(diff) } @@ -912,7 +900,7 @@ func Test_Agent_Auth_errors(t *testing.T) { res, err := cli.Do(req) require.NoError(t, err) res.Body.Close() - require.Equal(t, http.StatusBadRequest, res.StatusCode) + require.Equal(t, http.StatusForbidden, res.StatusCode) }) t.Run("use another agent's api key", func(t *testing.T) { ctx := testlog.SetLogger(t).WithContext(ctx) @@ -952,7 +940,7 @@ func Test_Agent_Auth_errors(t *testing.T) { res, err = cli.Do(req) require.NoError(t, err) res.Body.Close() - require.Equal(t, http.StatusBadRequest, res.StatusCode) + require.Equal(t, http.StatusForbidden, res.StatusCode) }) t.Run("use api key for enrollment", func(t *testing.T) { ctx := testlog.SetLogger(t).WithContext(ctx) @@ -987,7 +975,7 @@ func Test_Agent_request_errors(t *testing.T) { res, err := cli.Do(req) require.NoError(t, err) res.Body.Close() - require.Equal(t, http.StatusBadRequest, res.StatusCode) + require.Equal(t, http.StatusUnauthorized, res.StatusCode) }) t.Run("bad path", func(t *testing.T) { ctx := testlog.SetLogger(t).WithContext(ctx) diff --git a/model/openapi.yml b/model/openapi.yml index 62e53fb42..1a4103d1f 100644 --- a/model/openapi.yml +++ b/model/openapi.yml @@ -996,6 +996,25 @@ components: statusCode: 401 error: Unauthorized message: ApiKey not enabled + forbidden: + description: 403 response when the API key is valid but does not match the agent + headers: + Elastic-Api-Version: + $ref: "#/components/headers/apiVersion" + X-Request-Id: + $ref: "#/components/headers/requestID" + content: + application/json: + schema: + allOf: + - $ref: "#/components/schemas/error" + examples: + unauthorized: + description: The ApiKey is not enabled + value: + statusCode: 403 + error: ErrAgentIdentity + message: Agent header contains wrong identifier agentNotFound: description: 404 response when the agent is not found. May be returned by checkin endpoint. or endpoints that use the agentApiKey auth scheme headers: @@ -1319,6 +1338,8 @@ paths: $ref: "#/components/responses/badRequest" "401": $ref: "#/components/responses/keyNotEnabled" + "403": + $ref: "#/components/responses/forbidden" "404": $ref: "#/components/responses/agentNotFound" "408": @@ -1389,6 +1410,8 @@ paths: $ref: "#/components/responses/badRequest" "401": $ref: "#/components/responses/keyNotEnabled" + "403": + $ref: "#/components/responses/forbidden" "404": $ref: "#/components/responses/agentNotFound" "408": @@ -1478,6 +1501,8 @@ paths: $ref: "#/components/responses/badRequest" "401": $ref: "#/components/responses/keyNotEnabled" + "403": + $ref: "#/components/responses/forbidden" "408": $ref: "#/components/responses/deadline" "500": @@ -1538,6 +1563,8 @@ paths: $ref: "#/components/responses/badRequest" "401": $ref: "#/components/responses/keyNotEnabled" + "403": + $ref: "#/components/responses/forbidden" "408": $ref: "#/components/responses/deadline" "500": @@ -1589,6 +1616,8 @@ paths: $ref: "#/components/responses/badRequest" "401": $ref: "#/components/responses/keyNotEnabled" + "403": + $ref: "#/components/responses/forbidden" "408": $ref: "#/components/responses/deadline" "500": diff --git a/pkg/api/client.gen.go b/pkg/api/client.gen.go index 1aeb7f84d..62ef47168 100644 --- a/pkg/api/client.gen.go +++ b/pkg/api/client.gen.go @@ -1166,6 +1166,7 @@ type AgentAcksResponse struct { JSON200 *AckResponse JSON400 *BadRequest JSON401 *KeyNotEnabled + JSON403 *Forbidden JSON404 *AgentNotFound JSON408 *Deadline JSON500 *InternalServerError @@ -1194,6 +1195,7 @@ type AgentCheckinResponse struct { JSON200 *CheckinResponse JSON400 *BadRequest JSON401 *KeyNotEnabled + JSON403 *Forbidden JSON404 *AgentNotFound JSON408 *Deadline JSON500 *InternalServerError @@ -1276,6 +1278,7 @@ type UploadBeginResponse struct { JSON200 *UploadBeginAPIResponse JSON400 *BadRequest JSON401 *KeyNotEnabled + JSON403 *Forbidden JSON408 *Deadline JSON500 *InternalServerError JSON503 *Unavailable @@ -1305,6 +1308,7 @@ type UploadCompleteResponse struct { } JSON400 *BadRequest JSON401 *KeyNotEnabled + JSON403 *Forbidden JSON408 *Deadline JSON500 *InternalServerError JSON503 *Unavailable @@ -1331,6 +1335,7 @@ type UploadChunkResponse struct { HTTPResponse *http.Response JSON400 *BadRequest JSON401 *KeyNotEnabled + JSON403 *Forbidden JSON408 *Deadline JSON500 *InternalServerError JSON503 *Unavailable @@ -1635,6 +1640,13 @@ func ParseAgentAcksResponse(rsp *http.Response) (*AgentAcksResponse, error) { } response.JSON401 = &dest + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 403: + var dest Forbidden + if err := json.Unmarshal(bodyBytes, &dest); err != nil { + return nil, err + } + response.JSON403 = &dest + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 404: var dest AgentNotFound if err := json.Unmarshal(bodyBytes, &dest); err != nil { @@ -1703,6 +1715,13 @@ func ParseAgentCheckinResponse(rsp *http.Response) (*AgentCheckinResponse, error } response.JSON401 = &dest + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 403: + var dest Forbidden + if err := json.Unmarshal(bodyBytes, &dest); err != nil { + return nil, err + } + response.JSON403 = &dest + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 404: var dest AgentNotFound if err := json.Unmarshal(bodyBytes, &dest); err != nil { @@ -1893,6 +1912,13 @@ func ParseUploadBeginResponse(rsp *http.Response) (*UploadBeginResponse, error) } response.JSON401 = &dest + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 403: + var dest Forbidden + if err := json.Unmarshal(bodyBytes, &dest); err != nil { + return nil, err + } + response.JSON403 = &dest + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 408: var dest Deadline if err := json.Unmarshal(bodyBytes, &dest); err != nil { @@ -1956,6 +1982,13 @@ func ParseUploadCompleteResponse(rsp *http.Response) (*UploadCompleteResponse, e } response.JSON401 = &dest + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 403: + var dest Forbidden + if err := json.Unmarshal(bodyBytes, &dest); err != nil { + return nil, err + } + response.JSON403 = &dest + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 408: var dest Deadline if err := json.Unmarshal(bodyBytes, &dest); err != nil { @@ -2010,6 +2043,13 @@ func ParseUploadChunkResponse(rsp *http.Response) (*UploadChunkResponse, error) } response.JSON401 = &dest + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 403: + var dest Forbidden + if err := json.Unmarshal(bodyBytes, &dest); err != nil { + return nil, err + } + response.JSON403 = &dest + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 408: var dest Deadline if err := json.Unmarshal(bodyBytes, &dest); err != nil { diff --git a/pkg/api/types.gen.go b/pkg/api/types.gen.go index d8f6de3e2..fcaad02b8 100644 --- a/pkg/api/types.gen.go +++ b/pkg/api/types.gen.go @@ -799,6 +799,9 @@ type BadRequest = Error // Deadline Error processing request. type Deadline = Error +// Forbidden Error processing request. +type Forbidden = Error + // InternalServerError Error processing request. type InternalServerError = Error