Skip to content

Handle Unauthenticated OPTIONS requests#96061

Merged
albertzaharovits merged 8 commits intoelastic:mainfrom
albertzaharovits:fix-authn-for-options-request
May 12, 2023
Merged

Handle Unauthenticated OPTIONS requests#96061
albertzaharovits merged 8 commits intoelastic:mainfrom
albertzaharovits:fix-authn-for-options-request

Conversation

@albertzaharovits
Copy link
Copy Markdown
Contributor

@albertzaharovits albertzaharovits commented May 12, 2023

This address HTTP OPTIONS requests following
the authentication refactoring in #95112.

Relates #95112

@albertzaharovits albertzaharovits changed the title Handle OPTIONS requests Handle Unauthenticated OPTIONS requests May 12, 2023
Comment on lines +1659 to +1668
if (httpPreRequest.method() != RestRequest.Method.OPTIONS) {
authenticationService.authenticate(
httpPreRequest,
ActionListener.wrap(ignored -> listener.onResponse(null), listener::onFailure)
);
} else {
// allow for unauthenticated OPTIONS request
// this includes CORS preflight, and regular OPTIONS that return permitted methods for a given path
listener.onResponse(null);
}
Copy link
Copy Markdown
Contributor Author

@albertzaharovits albertzaharovits May 12, 2023

Choose a reason for hiding this comment

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

This is the interesting part. Allow requests with OPTIONS method to bypass authentication.

Comment on lines +64 to +71
// requests with the OPTIONS method should be handled elsewhere, and not by calling {@code RestHandler#handleRequest}
// authn is bypassed for HTTP requests with the OPTIONS method, so this sanity check prevents dispatching unauthenticated requests
if (request.method() == Method.OPTIONS) {
// CORS - allow for preflight unauthenticated OPTIONS request
restHandler.handleRequest(request, channel, client);
handleException(
request,
channel,
new ElasticsearchSecurityException("Cannot dispatch OPTIONS request, as they are not authenticated")
);
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 is the second most interesting part.
Because OPTIONS requests bypass authentication, this is a sanity check that unauthenticated OPTIONS requests are not dispatched.

@albertzaharovits albertzaharovits added :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) >non-issue labels May 12, 2023
@albertzaharovits albertzaharovits marked this pull request as ready for review May 12, 2023 15:55
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label May 12, 2023
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor

@Tim-Brooks Tim-Brooks 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
Copy link
Copy Markdown
Contributor Author

Will loop this with @jakelandis post merge, as this is blocking some time-sensitive projects. ⚡

@albertzaharovits albertzaharovits merged commit 1f8d6eb into elastic:main May 12, 2023
@albertzaharovits albertzaharovits deleted the fix-authn-for-options-request branch May 12, 2023 18:37
legrego added a commit to elastic/kibana that referenced this pull request May 16, 2023
Resolves #157017
Resolves #157018

Unskips our Interactive Setup functional tests, which started failing
after a recent ES snapshot promotion. This was caused by a regression in
Elasticsearch, which was resolved via
elastic/elasticsearch#96061.

I will not be running a flaky test suite here, as these tests were
consistently failing, as opposed to flaky.
jasonrhodes pushed a commit to elastic/kibana that referenced this pull request May 17, 2023
Resolves #157017
Resolves #157018

Unskips our Interactive Setup functional tests, which started failing
after a recent ES snapshot promotion. This was caused by a regression in
Elasticsearch, which was resolved via
elastic/elasticsearch#96061.

I will not be running a flaky test suite here, as these tests were
consistently failing, as opposed to flaky.
albertzaharovits added a commit to albertzaharovits/elasticsearch that referenced this pull request Jun 15, 2023
This address HTTP OPTIONS requests following
the authentication refactoring in elastic#95112.

Relates elastic#95112
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>non-issue :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team v8.9.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants