Unauthorized keys return 401 #3135

michel-laterman · 2023-11-30T22:09:33Z

What is the problem this PR solves?

Any issues with the API key result in a 400 status code

How does this PR solve the problem?

Issues with API keys will result in a 401, or a 403 if the key does not match the agent.

Checklist

~~I have commented my code, particularly in hard-to-understand areas~~
I have made corresponding changes to the documentation
~~I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
I have added an entry in ./changelog/fragments using the changelog tool

Related issues

Closes Check enrollment status before authenticating agents #2861

michel-laterman · 2023-11-30T22:13:24Z

internal/pkg/api/error.go

@@ -167,7 +167,7 @@ func NewHTTPErrResp(err error) HTTPErrResp {
 		{
 			ErrAgentIdentity,
 			HTTPErrResp{
-				http.StatusBadRequest,
+				http.StatusForbidden,


I'm pretty sure I changed all the relevant auth errors to return a 401 here.

However I think it makes sense to use a 403 instead of a 401 in this case where the API key does not match the requested agent. I've altered the openapi spec to include it as a return for the relevant endpoints

cc @joshdover

Makes sense to me.
So IIUC if the key doesn't match it will be a 403 everything else will be 401.

If the key is not found, shouldn't we also return a 403 instead of a 401?

If the key is not found, shouldn't we also return a 403 instead of a 401?

This would be an unauthorized case, right? So 401 would be fine.

jsoriano · 2023-12-01T16:04:01Z

internal/pkg/api/error.go

@@ -167,7 +167,7 @@ func NewHTTPErrResp(err error) HTTPErrResp {
 		{
 			ErrAgentIdentity,
 			HTTPErrResp{
-				http.StatusBadRequest,
+				http.StatusForbidden,


If the key is not found, shouldn't we also return a 403 instead of a 401?

This would be an unauthorized case, right? So 401 would be fine.

model/openapi.yml

internal/pkg/api/error.go

Co-authored-by: Jaime Soriano Pastor <[email protected]>

jsoriano

Change makes sense to me 👍

Do we need to change anything in how the Agent handles authorization errors to completely close #2861?

michel-laterman · 2023-12-01T18:11:47Z

I don't think so, the isUnauth check for the checkin would trigger based on the response code the agent gets, so the agent would unenroll iteself after too many failures.