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

tools: add debug entitlements for macOS 10.15+ #34378

Closed
wants to merge 1 commit into from

Conversation

ggreco
Copy link
Contributor

@ggreco ggreco commented Jul 15, 2020

To debug native modules node should be a debuggable process, from MacOS 10.15, Catalina, that will require the com.apple.security.get-task-allow entitlement to be added to the codesign process.

Fixes: #34340

Checklist
  • [X ] make -j4 test (UNIX), or vcbuild test (Windows) passes
  • [X ] commit message follows commit guidelines

By making a contribution to this project, I certify that:

The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file

To debug native modules node should be a debuggable process, that will require the **com.apple.security.get-task-allow** entitlement to be added to the codesign procedure.

Fixes: nodejs#34340

nodejs#34340
@nodejs-github-bot nodejs-github-bot added macos Issues and PRs related to the macOS platform / OSX. tools Issues and PRs related to the tools directory. labels Jul 15, 2020
@richardlau
Copy link
Member

cc @nodejs/platform-macos

@addaleax
Copy link
Member

ping @nodejs/collaborators (since the macos platform team might be a bit small)

@rvagg
Copy link
Member

rvagg commented Jul 20, 2020

This entitlement facilitates debugging on a system that uses System Integrity Protection (SIP) by circumventing certain security checks.

So get-task-allow I believe allows debuggers to attach to the process, but obviously that activates a bunch of interesting security vectors that Apple would rather avoid for the most part. I think that notarization would fail for this unless we weren't already doing disable-library-validation which enables whole different class of security vectors through plugin loading.

@ggreco can you confirm that macOS still has issues with node binaries from the darwin tarball and not just the .pkg installer? If it's possible for us to recommend that anyone needing a debugger should just get the tarball and use that then I think I'd rather err on the side of not adding this, but if our binaries are blocked regardless then I guess asking people to compile their own from source is probably not a great solution.

The challenge we have is that node is bundled in a lot of other downstream products, so the choices we make wrt liberal entitlements have flow-on effects for those products and introduce potential security vulnerabilities that may otherwise be stopped by the stricter macOS entitlements management system. The more of these entitlements we add, the closer we get to just turning it off entirely (which we're already pretty close to!).

@ggreco
Copy link
Contributor Author

ggreco commented Jul 21, 2020

Yes, the tarball has the same problem as the .pkg.

IMHO the fact that node already allows unsigned libraries is a far bigger security risk than allowing a debugger to attach the process.

I'm quite sure that at the moment you can do the notarization also with security.get-task-allow.

IMHO when you bundle node inside an application, for the appstore or also for custom distribution (simple notarization) you will have to resign everything, including the node binary, so that is not an issue.

@ggreco
Copy link
Contributor Author

ggreco commented Jul 21, 2020

Apple made a specific exception for cases like node (plugin debugging):

https://developer.apple.com/documentation/xcode/notarizing_macos_software_before_distribution/resolving_common_notarization_issues

Note

To enable debugging a plug-in in the context of a host executable, the host can include the com.apple.security.get-task-allow entitlement if it also includes the Disable Library Validation Entitlement. Don’t disable library validation for executables that don’t host plug-ins because library validation protects them from loading untrusted code.

@rvagg
Copy link
Member

rvagg commented Jul 21, 2020

IMHO when you bundle node inside an application, for the appstore or also for custom distribution (simple notarization) you will have to resign everything, including the node binary, so that is not an issue.

You can just bundle our signed binaries in your thing and sign anything else that's unsigned. A security conscious upstream bundler might want to resign our binaries with more restricted entitlements. I'm just a little doubtful that there will be many who even understand this though. It's really on us to be as restrictive as practical .. which unfortunately isn't very restrictive!

@nodejs/security this might be a something for you to at least be aware of and have the opportunity to object to: we're already being very liberal with our entitlements for the new macOS Gatekeeper. We're already asking for almost everything, including the ability to load unsigned plugins (Node addons). This one adds get-task to allow attaching to the process by an arbitrary secondary process, meant for debugging. Without it, debuggers are either difficult or impossible (I'm not sure how you work around this with Gatekeeper, there's probably a way to turn it off somewhere deep in the bowels of the OS). I don't know if we have too many options here if we want macOS users to be able to attach with an external debugger on Catalina onward.

@rvagg
Copy link
Member

rvagg commented Jul 21, 2020

Test build for someone to verify the darwin tarball and .pkg works as expected on Catalina: https://nodejs.org/download/test/v15.0.0-test20200721954cff688d/

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

fine .. pending verification that test build works as expected

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM, as I think we have to support debuggers.

@ggreco
Copy link
Contributor Author

ggreco commented Jul 22, 2020

fine .. pending verification that test build works as expected

debugging works as intended

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

lpinca pushed a commit that referenced this pull request Aug 10, 2020
To debug native modules node should be a debuggable process, that will
require the **com.apple.security.get-task-allow** entitlement to be
added to the codesign procedure.

PR-URL: #34378
Fixes: #34340
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@lpinca
Copy link
Member

lpinca commented Aug 10, 2020

Landed in b0e4970.

@lpinca lpinca closed this Aug 10, 2020
MylesBorins pushed a commit that referenced this pull request Aug 17, 2020
To debug native modules node should be a debuggable process, that will
require the **com.apple.security.get-task-allow** entitlement to be
added to the codesign procedure.

PR-URL: #34378
Fixes: #34340
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@danielleadams danielleadams mentioned this pull request Aug 20, 2020
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
To debug native modules node should be a debuggable process, that will
require the **com.apple.security.get-task-allow** entitlement to be
added to the codesign procedure.

PR-URL: #34378
Fixes: #34340
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams pushed a commit that referenced this pull request Aug 25, 2020
Notable changes:

- build: set --v8-enable-object-print by default (Mary Marchini)
  [#34705](#34705)
- deps:
  - upgrade to libuv 1.39.0 (cjihrig)
    [#34915](#34915)
  - upgrade npm to 6.14.8 (Ruy Adorno)
    [#34834](#34834)
  - V8: cherry-pick e06ace6b5cdb (Anna Henningsen)
    [#34673](#34673)
- n-api: handle weak no-finalizer refs correctly (Gabriel Schulhof)
  [#34839](#34839)
- tools: add debug entitlements for macOS 10.15+ (Gabriele Greco)
  [#34378](#34378)

PR-URL: #34852
danielleadams pushed a commit that referenced this pull request Aug 26, 2020
Notable changes:

- build: set --v8-enable-object-print by default (Mary Marchini)
  [#34705](#34705)
- deps:
  - upgrade to libuv 1.39.0 (cjihrig)
    [#34915](#34915)
  - upgrade npm to 6.14.8 (Ruy Adorno)
    [#34834](#34834)
  - V8: cherry-pick e06ace6b5cdb (Anna Henningsen)
    [#34673](#34673)
- n-api: handle weak no-finalizer refs correctly (Gabriel Schulhof)
  [#34839](#34839)
- tools: add debug entitlements for macOS 10.15+ (Gabriele Greco)
  [#34378](#34378)

PR-URL: #34852
danielleadams pushed a commit that referenced this pull request Aug 26, 2020
Notable changes:

- build: set --v8-enable-object-print by default (Mary Marchini)
  [#34705](#34705)
- deps:
  - upgrade to libuv 1.39.0 (cjihrig)
    [#34915](#34915)
  - upgrade npm to 6.14.8 (Ruy Adorno)
    [#34834](#34834)
  - V8: cherry-pick e06ace6b5cdb (Anna Henningsen)
    [#34673](#34673)
- n-api: handle weak no-finalizer refs correctly (Gabriel Schulhof)
  [#34839](#34839)
- tools: add debug entitlements for macOS 10.15+ (Gabriele Greco)
  [#34378](#34378)

PR-URL: #34852
danielleadams pushed a commit that referenced this pull request Aug 26, 2020
Notable changes:

- build: set --v8-enable-object-print by default (Mary Marchini)
  [#34705](#34705)
- deps:
  - upgrade to libuv 1.39.0 (cjihrig)
    [#34915](#34915)
  - upgrade npm to 6.14.8 (Ruy Adorno)
    [#34834](#34834)
  - V8: cherry-pick e06ace6b5cdb (Anna Henningsen)
    [#34673](#34673)
- n-api: handle weak no-finalizer refs correctly (Gabriel Schulhof)
  [#34839](#34839)
- tools: add debug entitlements for macOS 10.15+ (Gabriele Greco)
  [#34378](#34378)

PR-URL: #34852
danielleadams pushed a commit that referenced this pull request Aug 27, 2020
Notable changes:

- build: set --v8-enable-object-print by default (Mary Marchini)
  [#34705](#34705)
- deps:
  - upgrade to libuv 1.39.0 (cjihrig)
    [#34915](#34915)
  - upgrade npm to 6.14.8 (Ruy Adorno)
    [#34834](#34834)
  - V8: cherry-pick e06ace6b5cdb (Anna Henningsen)
    [#34673](#34673)
- n-api: handle weak no-finalizer refs correctly (Gabriel Schulhof)
  [#34839](#34839)
- tools: add debug entitlements for macOS 10.15+ (Gabriele Greco)
  [#34378](#34378)

PR-URL: #34852
BethGriggs pushed a commit that referenced this pull request Aug 27, 2020
Notable changes:

- build: set --v8-enable-object-print by default (Mary Marchini)
  [#34705](#34705)
- deps:
  - upgrade to libuv 1.39.0 (cjihrig)
    [#34915](#34915)
  - upgrade npm to 6.14.8 (Ruy Adorno)
    [#34834](#34834)
  - V8: cherry-pick e06ace6b5cdb (Anna Henningsen)
    [#34673](#34673)
- n-api: handle weak no-finalizer refs correctly (Gabriel Schulhof)
  [#34839](#34839)
- tools: add debug entitlements for macOS 10.15+ (Gabriele Greco)
  [#34378](#34378)

PR-URL: #34852
@clarkttfu
Copy link

Will this be merged into v12 branch?

@BethGriggs
Copy link
Member

@clarkttfu, I think it should be. I've added the lts-watch-v12.x label to indicate that it should be considered for the next v12.x release.

/cc @nodejs/lts

@rvagg
Copy link
Member

rvagg commented Sep 1, 2020

+1 to backporting

addaleax pushed a commit that referenced this pull request Sep 22, 2020
To debug native modules node should be a debuggable process, that will
require the **com.apple.security.get-task-allow** entitlement to be
added to the codesign procedure.

PR-URL: #34378
Fixes: #34340
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
To debug native modules node should be a debuggable process, that will
require the **com.apple.security.get-task-allow** entitlement to be
added to the codesign procedure.

PR-URL: #34378
Fixes: #34340
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
richardlau pushed a commit that referenced this pull request Oct 7, 2020
To debug native modules node should be a debuggable process, that will
require the **com.apple.security.get-task-allow** entitlement to be
added to the codesign procedure.

PR-URL: #34378
Fixes: #34340
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@richardlau richardlau mentioned this pull request Oct 7, 2020
3 tasks
richardlau added a commit that referenced this pull request Oct 7, 2020
Notable changes:
- deps:
  - upgrade npm to 6.14.8 (Ruy Adorno)
    #34834
- n-api:
  - create N-API version 7 (Gabriel Schulhof)
    #35199
  - expose napi_build_version variable (NickNaso)
    #27835
- tools:
  - add debug entitlements for macOS 10.15+ (Gabriele Greco)
    #34378

PR-URL: #35544
richardlau added a commit that referenced this pull request Oct 9, 2020
Notable changes:
- deps:
  - upgrade npm to 6.14.8 (Ruy Adorno)
    #34834
- n-api:
  - create N-API version 7 (Gabriel Schulhof)
    #35199
  - expose napi_build_version variable (NickNaso)
    #27835
- tools:
  - add debug entitlements for macOS 10.15+ (Gabriele Greco)
    #34378

PR-URL: #35544
richardlau added a commit that referenced this pull request Oct 27, 2020
Notable changes:
- deps:
  - upgrade npm to 6.14.8 (Ruy Adorno)
    #34834
- n-api:
  - create N-API version 7 (Gabriel Schulhof)
    #35199
  - expose napi_build_version variable (NickNaso)
    #27835
- tools:
  - add debug entitlements for macOS 10.15+ (Gabriele Greco)
    #34378

PR-URL: #35544
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macos Issues and PRs related to the macOS platform / OSX. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow debugging of native addons on macOS