Skip to content

Reject OPTIONS requests with a body#96357

Merged
albertzaharovits merged 4 commits intoelastic:mainfrom
albertzaharovits:do-not-let-through-OPTIONS-with-a-body
May 29, 2023
Merged

Reject OPTIONS requests with a body#96357
albertzaharovits merged 4 commits intoelastic:mainfrom
albertzaharovits:do-not-let-through-OPTIONS-with-a-body

Conversation

@albertzaharovits
Copy link
Copy Markdown
Contributor

@albertzaharovits albertzaharovits commented May 25, 2023 •
edited
Loading

Instead of not authN and letting them through, this PR rejects OPTIONS requests with a body (400).

Relates #95112

@albertzaharovits albertzaharovits self-assigned this May 25, 2023
@albertzaharovits albertzaharovits changed the title Main Reject OPTIONS requests with a body May 26, 2023
@albertzaharovits albertzaharovits mentioned this pull request May 26, 2023
Closed
@albertzaharovits albertzaharovits added :Distributed/Network Http and internode communication implementations :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) >non-issue labels May 26, 2023
@albertzaharovits
Copy link
Copy Markdown
Contributor Author

albertzaharovits commented May 26, 2023

To reviewers:
I struggled a bit more than expected with this one PR.
The reasons are:

  • OPTIONS requests without a body that go through unauthenticated, must still have the thread-context populated with the request headers, like regular requests have (not really important, but nice to have for tracing purposes....)
  • I wanted to have the logic that denies OPTIONS request with a body and the logic that lets OPTIONS request without a body through unauthenticated, close to each other.
  • Testing OPTIONS requests with a body is not possible in an IT because the apache http lib (that our rest client uses internally) doesn't allow it (but HTTP allows it and curl can issue those).

So it's funkier than I expected.

@albertzaharovits albertzaharovits marked this pull request as ready for review May 26, 2023 15:07
@elasticsearchmachine elasticsearchmachine added Team:Distributed Meta label for distributed team. Team:Security Meta label for security team labels May 26, 2023
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented May 26, 2023

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

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented May 26, 2023

Pinging @elastic/es-distributed (Team:Distributed)

jakelandis
jakelandis approved these changes May 26, 2023
View reviewed changes
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.

I originally thought this might be a breaking change but now I am pretty sure it is not breaking in a way anyone would ever notice. I spent some time understanding CORS preflight requests (the primary workflow for unauthenticated OPTIONS call) and since the Browser typically makes that call and there is no reason for it send a body we can be pretty confident they don't. Similarly, JS library don't really have a reason to make the preflight call but if they do I could not find any evidence to why they might include a body.

Tim-Brooks
Tim-Brooks approved these changes May 26, 2023
View reviewed changes
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

@Tim-Brooks
Copy link
Copy Markdown
Contributor

Tim-Brooks commented May 26, 2023

Browser typically makes that call and there is no reason for it send a body we can be pretty confident they don't

Also the RFC does not allow for bodies in OPTION requests. Most server implementations would already be rejecting this.

@albertzaharovits albertzaharovits merged commit 0e95bb4 into elastic:main May 29, 2023
@albertzaharovits albertzaharovits deleted the do-not-let-through-OPTIONS-with-a-body branch May 29, 2023 11:43
albertzaharovits added a commit to albertzaharovits/elasticsearch that referenced this pull request Jun 15, 2023
@albertzaharovits
Reject OPTIONS requests with a body (elastic#96357)
f76d8a0 
Instead of not authN and letting them through,
this PR rejects OPTIONS requests with a body (400).

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

Reviewers

@Tim-Brooks Tim-Brooks Tim-Brooks approved these changes

@jakelandis jakelandis jakelandis approved these changes

Assignees

@albertzaharovits albertzaharovits

Labels

:Distributed/Network Http and internode communication implementations >non-issue :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Distributed Meta label for distributed team. Team:Security Meta label for security team v8.9.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@albertzaharovits @elasticsearchmachine @Tim-Brooks @jakelandis