Skip to content

Only remove telltale response headers in case of 401 or 403#95189

Merged
albertzaharovits merged 4 commits intoelastic:mainfrom
albertzaharovits:simplify-security-error-response
Apr 12, 2023
Merged

Only remove telltale response headers in case of 401 or 403#95189
albertzaharovits merged 4 commits intoelastic:mainfrom
albertzaharovits:simplify-security-error-response

Conversation

@albertzaharovits
Copy link
Copy Markdown
Contributor

@albertzaharovits albertzaharovits commented Apr 12, 2023

This ensures that Warning and X-elastic-product response
headers are only removed in case of 401 or 403 HTTP error
response codes.
Previously, the response headers were also removed in non
401 and 403 conditions, in case of esoteric errors during authN,
but this behavior displaced the response-filtering logic to
an uncomfortable code site from a reusability pov.

@albertzaharovits albertzaharovits changed the title Simplify security error response Only remove telltale response headers in case of 401 or 403 Apr 12, 2023
@albertzaharovits albertzaharovits added >refactoring :Security/Security Security issues without another label labels Apr 12, 2023
@albertzaharovits albertzaharovits marked this pull request as ready for review April 12, 2023 13:57
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Apr 12, 2023
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-security (Team:Security)

@albertzaharovits
Copy link
Copy Markdown
Contributor Author

@jakelandis I realised I haven't done too great of a job explaining this live, so I'll try it a second time below:

Right now, authN failure responses are generated in the SecurityRestFilter. The intent of this PR is to simplify the logic that generates the responses from authN exceptions. Ultimately, with the moving of the authN itelsef, that response logic is going to be moved as well, to the RestController#dispatchBadRequest, or to its caller, as authN failues will begin to look more like "invalid" requests. So, the interest here is that generating responses for "invalid" and unauthenticated requests be as similar as possible, i.e. new RestResponse(channel, BAD_REQUEST, e) vs new RestResponse(channel, e).

In order to achieve that, I moved most of the special code in the SecurityRestFilter inside the RestResponse class itself.

  1. 22eda88

Makes it that all RestResponses with HTTP 401 error codes, across all the ES codebase, will hide stacktraces. Previously, only 401 errors generated from the SecurityRestFilter would behave like that; now that's been extended to all 401 errors. In practice, it doesn't make any difference since 401s are only generated from the SecurityRestFilter.

  1. 751976f

Makes it that Warning and X-elastic-product response headers are cleared (aka not returned) when the response is a 401 or a 403, in all cases. Again, previously, only such errors generated from the SecurityRestFilter exhibited this behavior; now it's been generalized. Also, again, in practice, it shouldn't make any difference since 401/403 responses are not generated from anywhere else but the SecurityRestFilter.
BUT here's a small wrinkle. Previously, any sort of exception during authN (even non-401) would make it that the response doesn't include the said headers. This behavior is now removed, because this doesn't generalize well... In practice it shouldn't make a difference because most authN exceptions are 401 cases.

}

});
channel.sendResponse(new RestResponse(channel, e));
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This here is the goal: new RestResponse(channel, e) is how responses are generated from exceptions everywhere else in the codebase.
This is important because authN failure exceptions will be signaled like exceptions for invalid/malformed requests.

Copy link
Copy Markdown
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@albertzaharovits albertzaharovits merged commit 8f884c9 into elastic:main Apr 12, 2023
@albertzaharovits albertzaharovits deleted the simplify-security-error-response branch April 12, 2023 17:38
@albertzaharovits
Copy link
Copy Markdown
Contributor Author

Thank you for the ⚡ review Jake!

albertzaharovits added a commit to albertzaharovits/elasticsearch that referenced this pull request Jun 13, 2023
…95189)

This ensures that Warning and X-elastic-product response
headers are only removed in case of 401 or 403 HTTP error
response codes.
Previously, the response headers were also removed in non
401 and 403 conditions, in case of esoteric errors during authN,
but this behavior displaced the response-filtering logic to
an uncomfortable code site from a reusability pov.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>refactoring :Security/Security Security issues without another label Team:Security Meta label for security team v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants