Don't remove warning headers on all failure #76434
Conversation
We shouldn't remove warning when request is failing not because of security reasons (syntax error for ex.). Note, that security related failure could happen not only during authentication (therefore we will check for the rest status), also all failures happened during authentication will be considered security related and warnings will be removed from the response. Resolves: elastic#75739
BigPandaToo
added
:Security/Security
|
Pinging @elastic/es-security (Team:Security) |
|
@jbaiera adding you, because this change affecting your recent addition to SecurityRestFilter |
|
Can we have a test for this please? I think we need 3 basic tests all of which generate warning headers, then
And check that the warning header is removed in the first case, but not in the other 2. |
| private void handleException(String actionType, RestRequest request, RestChannel channel, Exception e) { | ||
| logger.debug(new ParameterizedMessage("{} failed for REST request [{}]", actionType, request.uri()), e); | ||
| protected void handleException(ActionType actionType, RestRequest request, RestChannel channel, Exception e) { | ||
| logger.debug(new ParameterizedMessage("{} failed for REST request [{}]", actionType.name(), request.uri()), e); |
There was a problem hiding this comment.
| logger.debug(new ParameterizedMessage("{} failed for REST request [{}]", actionType.name(), request.uri()), e); | |
| logger.debug(new ParameterizedMessage("{} failed for REST request [{}]", actionType, request.uri()), e); |
.name() will return the enum constant. I think you want to use the toString here instead don't you?
| if (headers.containsKey("X-elastic-product")) { | ||
| headers = Maps.copyMapWithRemovedEntry(headers, "X-elastic-product"); | ||
| if (actionType != ActionType.RequestHandling | ||
| || (restStatus == RestStatus.UNAUTHORIZED || restStatus == RestStatus.FORBIDDEN)) { |
There was a problem hiding this comment.
Do we want to remove warning headers on 403 (FORBIDDEN)? A 403 means authentication is successful. IIUC, the original intention is to remove warnings if authentication fails, i.e. 401.
There was a problem hiding this comment.
We discussed it with @tvernum. More broadly the intention was to remove warnings in case request fails because of any security problem, which include failing authentication and failing the request handling because access was denied at some point of the handling.
|
@elasticmachine update branch |
|
@elasticmachine update branch |
|
@elasticmachine update branch |
|
@elasticmachine update branch |
* master: (868 commits) Query API key - Rest spec and yaml tests (elastic#76238) Delay shard reassignment from nodes which are known to be restarting (elastic#75606) Reenable bwc tests for elastic#76475 (elastic#76576) Set version to 7.15 in BWC code (elastic#76577) Don't remove warning headers on all failure (elastic#76434) Disable bwc tests for elastic#76475 (elastic#76541) Re-enable bwc tests (elastic#76567) Keep track of data recovered from snapshots in RecoveryState (elastic#76499) [Transform] Align transform checkpoint range with date_histogram interval for better performance (elastic#74004) EQL: Remove "wildcard" function (elastic#76099) Fix 'accept' and 'content_type' fields for search_mvt API Add persistent licensed feature tracking (elastic#76476) Add system data streams to feature state snapshots (elastic#75902) fix the error message for instance methods that don't exist (elastic#76512) ILM: Add validation of the number_of_shards parameter in Shrink Action of ILM (elastic#74219) remove dashboard only reserved role (elastic#76507) Fix Stack Overflow in UnassignedInfo in Corner Case (elastic#76480) Add (Extended)KeyUsage KeyUsage, CipherSuite & Protocol to SSL diagnostics (elastic#65634) Add recovery from snapshot to tests (elastic#76535) Reenable BwC Tests after elastic#76532 (elastic#76534) ...
We shouldn't remove warning when request is failing not because of
security reasons (syntax error for ex.).
Note, that security related failure could happen not only during
authentication (therefore we will check for the rest status), also all
failures happened during authentication will be considered security
related and warnings will be removed from the response.
Resolves: #75739