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

fix(security_response_headers): update Permissions-Policy default val… #205039

Closed
wants to merge 4 commits into from

Conversation

Ruby-rc
Copy link
Contributor

@Ruby-rc Ruby-rc commented Dec 20, 2024

Summary

Closes #204721

  • Changed the fullscreen setting in the Permissions-Policy header from (self) to * to allow loading from this source.
export const ONBOARDING_VIDEO_SOURCE = '//play.vidyard.com/K6kKDBbP9SpXife9s2tHNP.html?';
  • Since fullscreen is considered a relatively safe browser feature, the setting has been adjusted to allow all sources.

Screen recording after modification:

20241220-191850.mp4

@Ruby-rc Ruby-rc requested a review from a team as a code owner December 20, 2024 11:30
@afharo afharo requested a review from a team December 20, 2024 17:13
@afharo afharo added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) labels Dec 20, 2024
@afharo
Copy link
Member

afharo commented Dec 20, 2024

/ci

@afharo
Copy link
Member

afharo commented Dec 20, 2024

@Ruby-rc, thank you for the contribution!

The changes LGTM. But before approving, I'd love it if @elastic/kibana-security could take a look.

@afharo afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Dec 20, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 20, 2024

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #4 / has defaults for config
  • [job] [logs] Jest Tests #4 / has defaults for config
  • [job] [logs] FTR Configs #15 / serverless common API security/response_headers API endpoint response contains default security headers
  • [job] [logs] FTR Configs #20 / serverless common API security/response_headers API endpoint response contains default security headers
  • [job] [logs] FTR Configs #113 / serverless common API security/response_headers API endpoint response contains default security headers
  • [job] [logs] FTR Configs #15 / serverless common API security/response_headers API endpoint response contains default security headers
  • [job] [logs] FTR Configs #113 / serverless common API security/response_headers API endpoint response contains default security headers
  • [job] [logs] FTR Configs #20 / serverless common API security/response_headers API endpoint response contains default security headers

Metrics [docs]

✅ unchanged

History

@Ruby-rc
Copy link
Contributor Author

Ruby-rc commented Dec 23, 2024

@Ruby-rc, thank you for the contribution!

The changes LGTM. But before approving, I'd love it if @elastic/kibana-security could take a look.

Thank you! There are some uncertainties that need further discussion.

I’ve just noticed an issue with this commit — the property value was incorrect, and some test files were not updated.

I’ve made the necessary fixes and committed the changes again. However, I’m not sure if setting the fullscreen value to allow same-origin (self) and specific sources would be a better approach.

Additionally, I noticed that the docs/setup/settings.asciidoc file contains instructions on how to modify the Permissions-Policy. The exact approach for modifying it should be discussed further.

[[settings]]
== Configure {kib}

The {kib} server reads properties from the `kibana.yml` file on startup. The
location of this file differs depending on how you installed {kib}. For example,
if you installed {kib} from an archive distribution (`.tar.gz` or `.zip`), by
default it is in `$KIBANA_HOME/config`. By default, with package distributions
(Debian or RPM), it is in `/etc/kibana`.  The config directory can be changed via the
`KBN_PATH_CONF` environment variable:

@azasypkin
Copy link
Member

Hey @Ruby-rc,

Thanks a lot for the contribution!

Changed the fullscreen setting in the Permissions-Policy header from (self) to * to allow loading from this source.
export const ONBOARDING_VIDEO_SOURCE = '//play.vidyard.com/K6kKDBbP9SpXife9s2tHNP.html?';

Is this the only video source you wanted to relax the fullscreen restriction for, or are there other sources you believe we need to support fullscreen mode for? I’m asking because switching from self to * is a somewhat significant change, even for fullscreen, and I’m wondering if this specific video is the sole reason behind it.

I’m also tagging @agusruidiazgd, who I believe added this link in https://github.com/elastic/kibana/pull/190696/files#diff-ac85eb4cd1c126a214648960cb92a88735f61adcac7972c8670bc338fca22b4fR20, to understand the expected lifetime of this link, whether we plan to add more, and if vidyard.com is our video hosting service of choice for stuff like this (e.g. to consider switching from self to self "*.vidyard.com" instead *).

Copy link
Contributor

@elena-shostak elena-shostak left a comment

Choose a reason for hiding this comment

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

Requesting changes for this one per Oleg's comment

@Ruby-rc
Copy link
Contributor Author

Ruby-rc commented Dec 24, 2024

Hey @Ruby-rc,

Thanks a lot for the contribution!

Changed the fullscreen setting in the Permissions-Policy header from (self) to * to allow loading from this source.
export const ONBOARDING_VIDEO_SOURCE = '//play.vidyard.com/K6kKDBbP9SpXife9s2tHNP.html?';

Is this the only video source you wanted to relax the fullscreen restriction for, or are there other sources you believe we need to support fullscreen mode for? I’m asking because switching from self to * is a somewhat significant change, even for fullscreen, and I’m wondering if this specific video is the sole reason behind it.

I’m also tagging @agusruidiazgd, who I believe added this link in https://github.com/elastic/kibana/pull/190696/files#diff-ac85eb4cd1c126a214648960cb92a88735f61adcac7972c8670bc338fca22b4fR20, to understand the expected lifetime of this link, whether we plan to add more, and if vidyard.com is our video hosting service of choice for stuff like this (e.g. to consider switching from self to self "*.vidyard.com" instead *).

Thank you for your response. The reason I'm quoting the specific video URL is to discuss whether there's a more appropriate way to write the video source configuration, so we can optimize the setup.

I fully agree with your point, especially when considering the choice of video hosting service and the details of how to handle the fullscreen restriction.

@Ruby-rc
Copy link
Contributor Author

Ruby-rc commented Dec 24, 2024

Hey @elena-shostak

The changes have been made as per comment(sorry , I'm not entirely sure which one is Oleg's 🤔).

However, notice that the Permissions-Policy needs to explicitly specify the protocol, so I have added the specific domain names for both http and https.

@Ruby-rc
Copy link
Contributor Author

Ruby-rc commented Dec 24, 2024

@elasticmachine merge upstream

@Ruby-rc
Copy link
Contributor Author

Ruby-rc commented Dec 24, 2024

@azasypkin

Sorry, I didn't realize this might be a topic for discussion earlier. If the internal solution hasn't been decided yet, feel free to close my PR for now.

@azasypkin
Copy link
Member

azasypkin commented Dec 24, 2024

@azasypkin
Sorry, I didn't realize this might be a topic for discussion earlier. If the internal solution hasn't been decided yet, feel free to close my PR for now.

@Ruby-rc No need to apologize, and sorry that it might take a bit longer to resolve! You’ve found a valid bug and proposed a reasonable solution, we just need a bit more time to ensure we handle it the right way for the long term. Let’s keep your PR open for now until we hear from @agusruidiazgd and have had a chance to discuss this internally. It’s the holiday season, and many people are out, but we’ll get back to you as soon as we’ve discussed it with all stakeholders.

Thanks!

sorry , I'm not entirely sure which one is Oleg's 🤔

That's me (#205039 (comment)) 🙂

@agusruidiazgd
Copy link
Contributor

Hi @Ruby-rc thanks for your contribution!
@azasypkin As far as I now this is the only video I found hosted by .vidyard.com, I will discuss with the team today about the reason of this and will come back with a bit more of context. I'm expecting to add more videos to this page so I'm wondering if we consistently will use .vidyard.com or should we change the host provider

@agusruidiazgd
Copy link
Contributor

Hey @Ruby-rc @azasypkin this is a duplicate of #177777 (comment).
Check the last comment it's blocked by #181812. I would advise to close this one to avoid confusion since it is being discussed internally 😄

@Ruby-rc
Copy link
Contributor Author

Ruby-rc commented Jan 13, 2025

Hey @Ruby-rc @azasypkin this is a duplicate of #177777 (comment). Check the last comment it's blocked by #181812. I would advise to close this one to avoid confusion since it is being discussed internally 😄

Hi @agusruidiazgd, @azasypkin,

Thanks for your feedback and for pointing out the related discussions. After reviewing them, I think this PR might not fully address all the considerations currently being discussed.

To avoid confusion and keep things aligned with the team's direction, I’ll close this PR for now. Once the team reaches a conclusion, I’d be happy to revisit this if needed.

Thanks for your guidance!

@Ruby-rc Ruby-rc closed this Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) 💝community release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] [Bug] Full Screen option for Elastic Security overview video does not work.
7 participants