Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Unauthenticated OPTIONS requests #96061

Merged

Conversation

albertzaharovits
Copy link
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
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 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
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
Collaborator

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

Copy link
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
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
11 checks passed
@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
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
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.

None yet

3 participants