Unauthorized keys return 401 (#3135)

* Unauthorized keys return 401 * Update model/openapi.yml Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co> --------- Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
elastic · Dec 1, 2023 · 85d6d8c · 85d6d8c
1 parent 9b5117e
commit 85d6d8c
Show file tree

Hide file tree

Showing 10 changed files with 130 additions and 35 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git a/internal/pkg/server/fleet_integration_test.go b/internal/pkg/server/fleet_integration_test.go
@@ -426,22 +426,16 @@ 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
 			err = json.Unmarshal(raw, &resp)
 			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,22 +455,16 @@ 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
 			err = json.Unmarshal(raw, &resp)
 			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
@@ -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":