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

Remove osd-name from response headers #574

Closed
wants to merge 1 commit into from

Conversation

tmarkley
Copy link
Contributor

@tmarkley tmarkley commented Jun 30, 2021

This is a draft PR to gather feedback.

Description

Addresses a potential security risk where the header could be used for targeted attacks. Depending on the deployment configuration, this header may include the IP address of the host. The header is included even if an error response is returned, meaning that it's possible that anyone could get access to this information, even if they are not authenticated/authorized to access the Dashboards instance.

As for why this header exists in the first place, I dug back through the commit history and found this comment attach the app name to the server, so we can be sure we are actually talking to kibana here (which was merged to the main repo): https://github.com/pgayvallet/kibana/blob/ec481861799ed8dcced9cafd8112e5b26e641c54/src/legacy/server/http/index.js#L63

However, there aren't any references that I could find in our code base or for any plugins that require this header. I can keep digging for more information.

Testing

Screen Shot 2021-07-06 at 1 28 40 PM

Issues Resolved

N/A - security risk

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

Addresses a potential security risk where the header could be used for
targeted attacks.

Signed-off-by: Tommy Markley <markleyt@amazon.com>
@tmarkley tmarkley self-assigned this Jun 30, 2021
@tmarkley tmarkley marked this pull request as draft June 30, 2021 23:45
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 17002b1

@kavilla
Copy link
Member

kavilla commented Jun 30, 2021

This has no impact in the application? Do plugins consume it?

@tmarkley
Copy link
Contributor Author

tmarkley commented Jul 1, 2021

This has no impact in the application? Do plugins consume it?

@kavilla I did some basic smoke tests and everything was fine. I also searched across repos for both "osd-name" and "kbn-name" and didn't find anything. I can post that information in the description.

@galangel
Copy link
Contributor

galangel commented Jul 1, 2021

@tmarkley Hi, can you elaborate on why this header was necessary or give some examples of the possible exploit?
I'm just not sure why it was there in the first place.

@tmarkley
Copy link
Contributor Author

tmarkley commented Jul 1, 2021

@tmarkley Hi, can you elaborate on why this header was necessary or give some examples of the possible exploit?

I'm just not sure why it was there in the first place.

@galangel Yeah, great question. Depending on the deployment configuration, this header can actually include the IP address of the host. The header is included even if an error response is returned, meaning that it's possible that anyone could get access to this information, even if they are not authenticated/authorized to access the Dashboards instance.

As for why it was there, I had to dig back through the commit history to try to figure that out. The only information I've found thus far was the comment 'attach the app name to the server, so we can be sure we are actually talking to kibana' here (which was merged to the main repo): https://github.com/pgayvallet/kibana/blob/ec481861799ed8dcced9cafd8112e5b26e641c54/src/legacy/server/http/index.js#L63

However, there aren't any references that I could find in our code base or for any plugins that require this header. I can keep digging for more information.

@tmarkley tmarkley changed the title Removes osd-name from response headers Remove osd-name from response headers Jul 1, 2021
@tmarkley tmarkley removed the v1.0.0 label Jul 7, 2021
@tmarkley
Copy link
Contributor Author

Closing for now - we want to be hesitant with breaking changes for our 1.x releases, there is a known workaround (changing the http config name), and this only applies to a small subset of environments.

@tmarkley tmarkley closed this Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants