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

build: use separate entitlements for different macOS helper executables #94728

Merged
merged 1 commit into from
May 7, 2020

Conversation

deepak1556
Copy link
Collaborator

@deepak1556 deepak1556 commented Apr 8, 2020

We currently bundle a single entitlement file for all the helper executables on macOS, currently we have Code Helper (Renderer) , Code Helper (GPU), Code Helper (Plugin) and Code Helper, this is not good from a security perspective, whatever we shipped so far was not an issue but once we have sandboxed renderer we would limit the capabilities of the renderer helper.

@joaomoreno
Copy link
Member

joaomoreno commented Apr 9, 2020

Strange that access to capabilities without the right entitlements leads to a crash. 🤔 Is this an Apple design decision?

@connor4312 Why do we want to enable getUserMedia() in VS Code?

@deepak1556 Feel free to refactor the build to add different entitlements to the different processes.

@joaomoreno
Copy link
Member

@connor4312 Why do we want to enable getUserMedia() in VS Code?

Just read microsoft/vscode-js-debug#400. So strange that it is VS Code that needs the entitlements... not Chrome. 🤔

@connor4312
Copy link
Member

In their API reference for Process, they mention that this is intentional:

In a sandboxed application, child processes created with the Process class inherit the sandbox of the parent app. You should generally write helper applications as XPC Services instead, because XPC Services allows you to specify different sandbox entitlements for helper apps

An alternative workaround might be to embed an XPC service that launches Chrome inside of the js-debug extension, but that seems like it would be complex.

@deepak1556 deepak1556 marked this pull request as ready for review April 10, 2020 10:08
@deepak1556 deepak1556 force-pushed the robo/fix_entitlements branch 2 times, most recently from 59a7f93 to 8f92c67 Compare April 10, 2020 21:19
@connor4312
Copy link
Member

Per the Chromium issue, it seems that there's a workaround we can apply on the js-debug side. I will investigate within the next couple days and follow up here.

@deepak1556
Copy link
Collaborator Author

deepak1556 commented Apr 10, 2020

Thanks @connor4312 ! that seems to be a more appropriate solution for the js-debug issue.

I am changing the PR to just refactor the entitlements applied.

@deepak1556 deepak1556 changed the title fix: add entitlement for media device usage build: use separate entitlements for different macOS helper executables Apr 10, 2020
@connor4312
Copy link
Member

Following up in the Chrome thread, I think we can close this. They seem amenable to a solution, and we can use port-based debugging instead of pipe-based debugging on OSX as a near-term workaround if necessary.

@deepak1556
Copy link
Collaborator Author

Thanks @connor4312 , I am keep this PR open for refactoring the entitlements, which will be useful once we sandbox the renderer #92164.

@deepak1556
Copy link
Collaborator Author

deepak1556 commented Apr 13, 2020

@deepak1556 deepak1556 modified the milestones: April 2020, May 2020 Apr 28, 2020
Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

Given the build runs fine, this LGTM! 🚀

@deepak1556 deepak1556 merged commit 9a087f1 into master May 7, 2020
@deepak1556 deepak1556 deleted the robo/fix_entitlements branch May 7, 2020 18:11
@kieferrm kieferrm mentioned this pull request May 10, 2020
61 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Jun 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants