Omit adding the `osd-version` header when the Fetch request is to an external origin #3643

AMoo-Miki · 2023-03-21T18:25:59Z

Description

Making fetch requests using core/public/http/fetch, an osd-version header is forcefully added, even to external requests. This change examines the destination and only adds the header to relative URLs and those that are to OSD itself.
This change also adds osd-xsrf to calls that use osd-version incorrectly to satisfy XSRF protection

Issues Resolved

Fixes #3277

Check List

kavilla

I think the failures since we utilized this for XRSF.

OpenSearch-Dashboards/src/core/server/http/lifecycle_handlers.ts

Line 57 in de06344

    
           if (!isSafeMethod(request.route.method) && !hasVersionHeader && !hasXsrfHeader) {

tries to find a header and then it can't find it so it throws the error.

It might not be the proper way of handling XSRF but by removing it causes these errors to be thrown.

AMoo-Miki · 2023-03-22T19:35:42Z

OpenSearch-Dashboards/src/core/server/http/lifecycle_handlers.ts

Line 57 in de06344

    
           if (!isSafeMethod(request.route.method) && !hasVersionHeader && !hasXsrfHeader) {

If it is a GET or OPTIONS call, in will not fail and also if it contains the osd-xsrf. 621bf0e suggests osd-version used to be used for XSRF prior to K5 and they never cared to clean the code up over the next 6 years. I have added osd-xsrf to any call that was using osd-version for XSRF protection.

joshuarrrr · 2023-03-23T01:12:01Z

@AMoo-Miki There are some snapshots that need updating.

AMoo-Miki · 2023-03-23T16:01:43Z

@AMoo-Miki There are some snapshots that need updating.

Oops; i thought i committed and pushed it... just did.

…external origin * Making `fetch` requests using core/public/http/fetch, an `osd-version` header is forcefully added, even to external requests. This change examines the destination and only adds the header to relative URLs and those that are to OSD itself. * This change also adds `osd-xsrf` to calls that use `osd-version` incorrectly to satisfy XSRF protection Fixes opensearch-project#3277 Signed-off-by: Miki <[email protected]>

kavilla

I think looks good to me. Pulled it down working. It's also working with the basic settings for security plugin.

One thing I'm not able to completely test is if any functionality relied on the osd-version header being present just so the redirect can be attached to the request and not fail.

For example, on main:
This succeeds:

curl localhost:5601/hdn/api/status -ku 'admin:admin'

This succeeds:

curl localhost:5601/hdn/api/status -ku 'admin:admin' -H osd-version:3.0.0

This succeeds:

curl localhost:5601/hdn/api/status -ku 'admin:admin' -H osd-version:

This fails:

curl localhost:5601/hdn/api/status -ku 'admin:admin' -H osd-version:3.5.0

Which makes sense because the version mismatch. So if any functionality outside of what I tested relied on the osd-version and utilized the fetch call might be broken if the fetch was passed from one request to another.

So I don't know want to block it on something that I can't test especially if the value being not present still works for us. But I do believe we might want to mention in higher in the changelog.

Like something that mentions,

HTTP Fetch from Core OpenSearch Dashboards no longer default appends the osd-version.

AMoo-Miki · 2023-04-17T22:09:09Z

Rocky, this change makes sure osd-xsrf is used all over and only removed node-version when the call is to a destination outside of OSD; any call to OSD will have node-version.

joshuarrrr

@AMoo-Miki Is there a follow-up issue already created to keep track of these todos?

joshuarrrr · 2023-04-17T22:18:06Z

src/core/server/http/lifecycle_handlers.ts

@@ -54,8 +54,9 @@ export const createXsrfPostAuthHandler = (config: HttpConfig): OnPostAuthHandler
    const hasVersionHeader = VERSION_HEADER in request.headers;
    const hasXsrfHeader = XSRF_HEADER in request.headers;

+    // ToDo: Remove !hasVersionHeader; `osd-version` incorrectly used for satisfying XSRF protection


nit - todo comments should have issues opened to link to.

…external origin (#3643) * Making `fetch` requests using core/public/http/fetch, an `osd-version` header is forcefully added, even to external requests. This change examines the destination and only adds the header to relative URLs and those that are to OSD itself. * This change also adds `osd-xsrf` to calls that use `osd-version` incorrectly to satisfy XSRF protection Fixes #3277 Signed-off-by: Miki <[email protected]> (cherry picked from commit 0762566) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

…quest is to an external origin (#3867) * Omit adding the `osd-version` header when the Fetch request is to an external origin (#3643) * Making `fetch` requests using core/public/http/fetch, an `osd-version` header is forcefully added, even to external requests. This change examines the destination and only adds the header to relative URLs and those that are to OSD itself. * This change also adds `osd-xsrf` to calls that use `osd-version` incorrectly to satisfy XSRF protection Fixes #3277 Signed-off-by: Miki <[email protected]> (cherry picked from commit 0762566) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md * add changelog Signed-off-by: Josh Romero <[email protected]> --------- Signed-off-by: Josh Romero <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Josh Romero <[email protected]>

AMoo-Miki requested a review from a team as a code owner March 21, 2023 18:26

kavilla requested changes Mar 21, 2023

View reviewed changes

AMoo-Miki force-pushed the limit-osd-version-header-in-fetch branch from 4236cef to fb9468f Compare March 22, 2023 18:55

abbyhu2000 assigned kavilla Mar 22, 2023

AMoo-Miki force-pushed the limit-osd-version-header-in-fetch branch from fb9468f to c55ee28 Compare March 22, 2023 19:34

AMoo-Miki force-pushed the limit-osd-version-header-in-fetch branch from c55ee28 to d8be993 Compare March 22, 2023 19:39

AMoo-Miki changed the title ~~Omit adding the osd-version header when the Fetch request was explicitly asked to not prepend the basePath~~ Omit adding the osd-version header when the _Fetch_ request is to an external origin Mar 22, 2023

AMoo-Miki changed the title ~~Omit adding the osd-version header when the _Fetch_ request is to an external origin~~ Omit adding the osd-version header when the Fetch request is to an external origin Mar 22, 2023

AMoo-Miki force-pushed the limit-osd-version-header-in-fetch branch from d8be993 to f0b5ae9 Compare March 22, 2023 19:46

AMoo-Miki requested a review from kavilla March 22, 2023 19:47

AMoo-Miki force-pushed the limit-osd-version-header-in-fetch branch from f0b5ae9 to 18a3b21 Compare March 22, 2023 19:51

AMoo-Miki force-pushed the limit-osd-version-header-in-fetch branch from 18a3b21 to ead7e34 Compare March 23, 2023 16:01

AMoo-Miki force-pushed the limit-osd-version-header-in-fetch branch from ead7e34 to b22eba6 Compare March 23, 2023 16:04

joshuarrrr mentioned this pull request Mar 23, 2023

2.7.0 Platform Upgrades for Plugins (O11y, MDS, Geospatial,+) #3674

Closed

23 tasks

seanneumann added the v2.7.0 label Mar 23, 2023

AMoo-Miki force-pushed the limit-osd-version-header-in-fetch branch from b22eba6 to c71b2ac Compare March 23, 2023 17:31

joshuarrrr added the backport 2.x label Mar 23, 2023

AMoo-Miki force-pushed the limit-osd-version-header-in-fetch branch from c71b2ac to ff6f6f6 Compare April 5, 2023 15:59

kavilla approved these changes Apr 17, 2023

View reviewed changes

joshuarrrr approved these changes Apr 17, 2023

View reviewed changes

joshuarrrr merged commit 0762566 into opensearch-project:main Apr 17, 2023

opensearch-trigger-bot bot mentioned this pull request Apr 17, 2023

[Backport 2.x] Omit adding the osd-version header when the Fetch request is to an external origin #3867

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Omit adding the `osd-version` header when the Fetch request is to an external origin #3643

Omit adding the `osd-version` header when the Fetch request is to an external origin #3643

AMoo-Miki commented Mar 21, 2023 •

edited

Loading

kavilla left a comment

AMoo-Miki commented Mar 22, 2023

joshuarrrr commented Mar 23, 2023

AMoo-Miki commented Mar 23, 2023

kavilla left a comment

AMoo-Miki commented Apr 17, 2023

joshuarrrr left a comment

joshuarrrr Apr 17, 2023

Omit adding the osd-version header when the Fetch request is to an external origin #3643

Omit adding the osd-version header when the Fetch request is to an external origin #3643

Conversation

AMoo-Miki commented Mar 21, 2023 • edited Loading

Description

Issues Resolved

Check List

kavilla left a comment

Choose a reason for hiding this comment

AMoo-Miki commented Mar 22, 2023

joshuarrrr commented Mar 23, 2023

AMoo-Miki commented Mar 23, 2023

kavilla left a comment

Choose a reason for hiding this comment

AMoo-Miki commented Apr 17, 2023

joshuarrrr left a comment

Choose a reason for hiding this comment

joshuarrrr Apr 17, 2023

Choose a reason for hiding this comment

Omit adding the `osd-version` header when the Fetch request is to an external origin #3643

Omit adding the `osd-version` header when the Fetch request is to an external origin #3643

AMoo-Miki commented Mar 21, 2023 •

edited

Loading