Bug 185526

Summary: [macOS] WebProcess needs TCC entitlements for media capture (Take 2)
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit2Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bfulgham, commit-queue, ddkilzer, eric.carlson, ggaren, jberlin, mitz, realdawei, ryanhaddad, saam
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=187981
https://bugs.webkit.org/show_bug.cgi?id=190399
Bug Depends on:    
Bug Blocks: 184485, 186150, 187981    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Brent Fulgham 2018-05-10 12:52:19 PDT
In Bug 181995 I added TCC entitlements for media capture to the macOS entitlements used for local relocatable builds. These changes also need to apply to system builds of WebKit.

Previously we had not needed an entitlements file for system builds of WebKit, so only provide an entitlements file for our relocatable build targets.

This patch does the following:

1. Copies the "WebContent-OSX.entitlements" to "WebContent-Relocatable-OSX.entitlements"
2. Updates the "WebContent.Development.entitlements" to include the TCC entitlements for media capture.
3. Removes the unneeded "com.apple.private.xpc.domain-extension" from WebContent-OSX.entitlement now that it is used for system WebKit, not relocatable WebKit.
4. Updates WebContentService.xcconfig so that CODE_SIGN_ENTITLEMENTS_OSX_WITH_XPC_DOMAIN_EXTENSION_YES points to the new "WebContent-Relocatable-OSX.entitlements" file.
5. Adds a new CODE_SIGN_ENTITLEMENTS_OSX_WITH_XPC_DOMAIN_EXTENSION_NO definition to WebContentService.xcconfig that points to the revised "WebContent-OSX.entitlements" file.
Comment 1 Brent Fulgham 2018-05-10 12:53:14 PDT
<rdar://problem/36674649>
Comment 2 Brent Fulgham 2018-05-10 12:59:09 PDT
Created attachment 340118 [details]
Patch
Comment 3 mitz 2018-05-10 13:05:05 PDT
Comment on attachment 340118 [details]
Patch

I suspect that signing the Web Content service with a restricted entitlement like that will cause it to crash on launch, unless it’s running on an Apple-internal version of macOS that’s been configured a certain way. If my suspicion is true, then we shouldn’t make this change because people from outside Apple won’t be able to build and use TOT WebKit.
Comment 4 Brent Fulgham 2018-05-10 13:26:39 PDT
Created attachment 340120 [details]
Patch
Comment 5 Brent Fulgham 2018-05-10 13:27:47 PDT
(In reply to mitz from comment #3)
> Comment on attachment 340118 [details]
> Patch
> 
> I suspect that signing the Web Content service with a restricted entitlement
> like that will cause it to crash on launch, unless it’s running on an
> Apple-internal version of macOS that’s been configured a certain way. If my
> suspicion is true, then we shouldn’t make this change because people from
> outside Apple won’t be able to build and use TOT WebKit.

I'll discuss this with the sandboxing team to determine what our options are. I won't land anything until we understand the impact of landing the change.
Comment 6 mitz 2018-05-10 13:35:49 PDT
(In reply to Brent Fulgham from comment #5)
> (In reply to mitz from comment #3)
> > Comment on attachment 340118 [details]
> > Patch
> > 
> > I suspect that signing the Web Content service with a restricted entitlement
> > like that will cause it to crash on launch, unless it’s running on an
> > Apple-internal version of macOS that’s been configured a certain way. If my
> > suspicion is true, then we shouldn’t make this change because people from
> > outside Apple won’t be able to build and use TOT WebKit.
> 
> I'll discuss this with the sandboxing team to determine what our options
> are. I won't land anything until we understand the impact of landing the
> change.

The impact could be seen here: <https://webkit-queues.webkit.org/results/7641866> all the tests are crashing because the Web Content process is signed with a restricted entitlement. As I mentioned in the ongoing Apple-internal email discussion of this, one possible setup is to tie signing with restricted entitlements to using an Apple-internal SDK.
Comment 7 Brent Fulgham 2018-05-10 14:58:23 PDT
Created attachment 340134 [details]
Patch
Comment 8 Brent Fulgham 2018-05-10 15:00:23 PDT
(In reply to mitz from comment #6)
> The impact could be seen here:
> <https://webkit-queues.webkit.org/results/7641866> all the tests are
> crashing because the Web Content process is signed with a restricted
> entitlement. As I mentioned in the ongoing Apple-internal email discussion
> of this, one possible setup is to tie signing with restricted entitlements
> to using an Apple-internal SDK.

As we discussed offline, I have added a test for $(USE_INTERNAL_SDK) to the rule selecting entitlements for signing.
Comment 9 mitz 2018-05-10 16:16:07 PDT
Comment on attachment 340134 [details]
Patch

Again, I suspect that this patch doesn’t do the right thing for my coworkers who work on Safari and WebKit at Apple. With this patch, the Web Content service will have an ad-hoc signature and a restricted entitlement, which will cause it to crash on launch in a common Apple-internal configuration, which only permits restricted entitlements when the executable is signed with a specific engineering code signing identity.
Comment 10 Brent Fulgham 2018-05-10 16:39:39 PDT
(In reply to mitz from comment #9)
> Comment on attachment 340134 [details]
> Patch
> 
> Again, I suspect that this patch doesn’t do the right thing for my coworkers
> who work on Safari and WebKit at Apple. With this patch, the Web Content
> service will have an ad-hoc signature and a restricted entitlement, which
> will cause it to crash on launch in a common Apple-internal configuration,
> which only permits restricted entitlements when the executable is signed
> with a specific engineering code signing identity.

I guess I am confused. When you said "one possible setup is to tie signing with restricted entitlements to using an Apple-internal SDK", I assumed that meant that building with Apple Internal could safely build with these entitlements. That is what this patch attempts to do.

Do you mean that the "WebContent.Development.entitlements" change will cause the problem for Safari/WebKit engineers? I could remove those changes without affecting the purpose of this patch.
Comment 11 mitz 2018-05-10 16:48:08 PDT
(In reply to Brent Fulgham from comment #10)
> (In reply to mitz from comment #9)
> > Comment on attachment 340134 [details]
> > Patch
> > 
> > Again, I suspect that this patch doesn’t do the right thing for my coworkers
> > who work on Safari and WebKit at Apple. With this patch, the Web Content
> > service will have an ad-hoc signature and a restricted entitlement, which
> > will cause it to crash on launch in a common Apple-internal configuration,
> > which only permits restricted entitlements when the executable is signed
> > with a specific engineering code signing identity.
> 
> I guess I am confused. When you said "one possible setup is to tie signing
> with restricted entitlements to using an Apple-internal SDK", I assumed that
> meant that building with Apple Internal could safely build with these
> entitlements. That is what this patch attempts to do.

It could safely build with these entitlements if it also signed with the “Safari Engineering” signing identity that folks working on Safari and WebKit use to sign binaries that have restricted entitlements.
 
> Do you mean that the "WebContent.Development.entitlements" change will cause
> the problem for Safari/WebKit engineers? I could remove those changes
> without affecting the purpose of this patch.

No.
Comment 12 Brent Fulgham 2018-05-17 14:34:46 PDT
Created attachment 340645 [details]
Patch
Comment 13 mitz 2018-05-17 15:05:38 PDT
Does attachment 340645 [details] address comment 9?
Comment 14 Brent Fulgham 2018-05-17 15:34:21 PDT
(In reply to mitz from comment #13)
> Does attachment 340645 [details] address comment 9?

This patch does not address the combination of an engineer using an internal SDK without having a valid Apple signing certificate.
Comment 15 mitz 2018-05-17 15:37:03 PDT
(In reply to Brent Fulgham from comment #14)
> (In reply to mitz from comment #13)
> > Does attachment 340645 [details] address comment 9?
> 
> This patch does not address the combination of an engineer using an internal
> SDK without having a valid Apple signing certificate.

Does it address comment 9, which describes the situation of many of my coworkers who work on Safari and WebKit at Apple?
Comment 16 Brent Fulgham 2018-05-18 12:09:14 PDT
(In reply to mitz from comment #15)
> (In reply to Brent Fulgham from comment #14)
> > (In reply to mitz from comment #13)
> > > Does attachment 340645 [details] address comment 9?
> > 
> > This patch does not address the combination of an engineer using an internal
> > SDK without having a valid Apple signing certificate.
> 
> Does it address comment 9, which describes the situation of many of my
> coworkers who work on Safari and WebKit at Apple?

Oh! I don't think it does. I have a new revision.
Comment 17 Brent Fulgham 2018-05-18 12:28:04 PDT
Created attachment 340728 [details]
Patch
Comment 18 mitz 2018-05-18 13:05:16 PDT
Comment on attachment 340728 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340728&action=review

> Source/WebKit/Configurations/WebContentService.xcconfig:38
> +CODE_SIGN_IDENTITY_MACOS_SINCE_1014_YES = Safari Engineering;

I suspect that this will cause the project to fail to build in Apple’s internal production build system, where an internal SDK is being used, but an identity with this name is not available.
Comment 19 Brent Fulgham 2018-05-25 13:07:06 PDT
Created attachment 341313 [details]
Patch
Comment 20 Brent Fulgham 2018-05-25 17:13:07 PDT
Created attachment 341363 [details]
Patch
Comment 21 Alexey Proskuryakov 2018-05-25 17:25:42 PDT
Comment on attachment 341313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341313&action=review

> Source/WebKit/Configurations/DebugRelease.xcconfig:48
> +CODE_SIGN_IDENTITY_BEFORE_1014_YES = -;
> +CODE_SIGN_IDENTITY_BEFORE_1014_NO = -;

Just to confirm that I understand this part of the change correctly:

- Is it accurate that we weren't signing at all before, and are ad hoc signing now?
- Is the ad hoc signature necessary?
- Will it work on other machines once the build product is copied over? In particular, will nightly builds work?

> Source/WebKit/Configurations/DebugRelease.xcconfig:49
> +CODE_SIGN_IDENTITY_MACOS_SINCE_1014_YES = Safari Engineering;

It's not very elegant to expose this here, but I guess it's probably not an actual problem.

However, I think that there is a problem with using the identity like that. Its keychain is initially locked, so we can't use it without programmatically unlocking first. If we try, a user prompt will appear on the screen.

> Source/WebKit/Configurations/WebContent-OSX.entitlements:13
> +	<key>com.apple.security.cs.allow-jit</key>
> +	<true/>

This change doesn't appear to be mentioned in ChangeLog.

> Source/WebKit/Scripts/process-webcontent-entitlements.sh:6
> +    echo "Process WebContent process entitlements for app sandbox.";

There is some inconsistency in messages produced by this script with regards to "process" vs. "processing", and having a period in the end.

> Source/WebKit/Scripts/process-webcontent-entitlements.sh:12
> +            echo "Adding TCC entitlements for app sandbox.";

This comment seems inaccurate in two ways:
1. We also add JIT entitlement.
2. I don't think that this has anything to do with app sandbox.

> Source/WebKit/Scripts/process-webcontent-entitlements.sh:23
> +            /usr/libexec/PlistBuddy -c "Add :com.apple.security.cs.disable-library-validation bool YES" "${PROCESSED_XCENT_FILE}";

Is this still necessary with com.apple.security.cs.allow-jit?

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10342
> +			inputPaths = (
> +				"$(TEMP_FILE_DIR)/$(FULL_PRODUCT_NAME).xcent",
> +			);

WebContent-OSX.entitlements is an input file too.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10347
> +			outputPaths = (
> +			);

Does $(FULL_PRODUCT_NAME).xcent need to be here?

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:819
> +(with-filter (extension "com.apple.webkit.microphone")
> +    (allow device-microphone))

I don't understand how this works (or how this doesn't work without the entitlement). Probably best to discuss in person.
Comment 22 Alexey Proskuryakov 2018-05-25 17:26:38 PDT
These comments are for the earlier version of the patch. I didn't look at the newest one yet.
Comment 23 Brent Fulgham 2018-05-25 21:37:10 PDT
Comment on attachment 341313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341313&action=review

>> Source/WebKit/Configurations/DebugRelease.xcconfig:48
>> +CODE_SIGN_IDENTITY_BEFORE_1014_NO = -;
> 
> Just to confirm that I understand this part of the change correctly:
> 
> - Is it accurate that we weren't signing at all before, and are ad hoc signing now?
> - Is the ad hoc signature necessary?
> - Will it work on other machines once the build product is copied over? In particular, will nightly builds work?

We do not need to sign for builds prior to 1014, so perhaps this is wrong.

I am not sure if nightly builds will work; I think it depends on the security settings of the machine running the executable.

>> Source/WebKit/Configurations/DebugRelease.xcconfig:49
>> +CODE_SIGN_IDENTITY_MACOS_SINCE_1014_YES = Safari Engineering;
> 
> It's not very elegant to expose this here, but I guess it's probably not an actual problem.
> 
> However, I think that there is a problem with using the identity like that. Its keychain is initially locked, so we can't use it without programmatically unlocking first. If we try, a user prompt will appear on the screen.

I noticed this while building locally. I don’t know what our options are to avoid the prompt on build machines, I assume there is some way to override this since other build systems seem to handle this,

>> Source/WebKit/Configurations/WebContent-OSX.entitlements:13
>> +	<true/>
> 
> This change doesn't appear to be mentioned in ChangeLog.

Yes, this is an unrelated change. I removed it in the patch I uploaded after this one.

>> Source/WebKit/Scripts/process-webcontent-entitlements.sh:6
>> +    echo "Process WebContent process entitlements for app sandbox.";
> 
> There is some inconsistency in messages produced by this script with regards to "process" vs. "processing", and having a period in the end.

I’ll clean that up.

>> Source/WebKit/Scripts/process-webcontent-entitlements.sh:12
>> +            echo "Adding TCC entitlements for app sandbox.";
> 
> This comment seems inaccurate in two ways:
> 1. We also add JIT entitlement.
> 2. I don't think that this has anything to do with app sandbox.

I’ll revise the message.

>> Source/WebKit/Scripts/process-webcontent-entitlements.sh:23
>> +            /usr/libexec/PlistBuddy -c "Add :com.apple.security.cs.disable-library-validation bool YES" "${PROCESSED_XCENT_FILE}";
> 
> Is this still necessary with com.apple.security.cs.allow-jit?

I am not 100% sure, but I believe the JIT entitlement (which was not meant to be in this patch) is expected for non-development builds, while the library validation is an engineering environment setting. I don’t believe the cs.allow-jit is as permissive as the library validation  flag.

>> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10342
>> +			);
> 
> WebContent-OSX.entitlements is an input file too.

Ok.

>> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10347
>> +			);
> 
> Does $(FULL_PRODUCT_NAME).xcent need to be here?

I don’t know, the working example I was using as a model did not have that set as an output file.

>> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:819
>> +    (allow device-microphone))
> 
> I don't understand how this works (or how this doesn't work without the entitlement). Probably best to discuss in person.

Sure.
Comment 24 mitz 2018-05-27 09:26:45 PDT
Comment on attachment 341363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341363&action=review

Seems like a good approach. I think the implementation and motivation can be simplified and made clearer with better naming (see inline comments). Like Alexey mentions, the keychain containing the signing identity may need to be unlocked. This can be done with a script built phase than checks whether an Apple-internal script (somewhere under ../Internal relative to the source tree) exists, and if so, invokes it. You can find an example of this in Apple’s internal WebInspector project.

> Source/WebKit/Configurations/DebugRelease.xcconfig:50
> +CODE_SIGN_IDENTITY[sdk=macosx*] = $(CODE_SIGN_IDENTITY$(WK_MACOS_1014)_$(USE_INTERNAL_SDK))
> +CODE_SIGN_IDENTITY_BEFORE_1014_YES = -;
> +CODE_SIGN_IDENTITY_BEFORE_1014_NO = -;
> +CODE_SIGN_IDENTITY_MACOS_SINCE_1014_YES = Safari Engineering;
> +CODE_SIGN_IDENTITY_MACOS_SINCE_1014_NO = -;

There is no downside to using the “Safari Engineering” singing identity when targeting earlier macOS versions, so the first simplification I would suggest is
CODE_SIGN_IDENTITY[sdk=macosx*] = $(CODE_SIGN_IDENTITY_$(USE_INTERNAL_SDK));
CODE_SIGN_IDENTITY_NO = -;
CODE_SIGN_IDENTITY_YES = Safari Engineering;

I think it would be good to be explicit about why we’re using that signing identity (and add some flexibility), though, so I think Base.xcconfig should define
WK_USE_RESTRICTED_ENTITLEMENTS = $(USE_INTERNAL_SDK);

and change the first line to say
CODE_SIGN_IDENTITY[sdk=macosx*] = $(CODE_SIGN_IDENTITY_$(WK_USE_RESTRICTED_ENTITLEMENTS));

We don’t have to name the “Safari Engineering” signing identity here. I suggest using
CODE_SIGN_IDENTITY_YES = $(WK_ENGINEERING_CODE_SIGN_IDENTITY);
and having WK_ENGINEERING_CODE_SIGN_IDENTITY defined in Apple’s HaveInternalSDK.xcconfig, which we already include if it exists.

> Source/WebKit/Configurations/WebContent-OSX.entitlements:11
>  		<string>kTCCServiceCamera</string>
>  		<string>kTCCServiceMicrophone</string>
>  	</array>
> -	<key>com.apple.private.xpc.domain-extension</key>
> -	<true/>
>  </dict>
>  </plist>

The name WebContent-OSX.entitlements has already caused confusion. It seems like this file’s role is to contain *some* restricted entitlements that the Web Content service needs in macOS 10.14 or later, so perhaps it can be named WebContent-OSX-restricted.entitlements. Or maybe it’s just the TCC-related entitlements, so it should named appropriately (and the Data Vaults entitlement would be in a different file, or just added by the script).

> Source/WebKit/Scripts/process-webcontent-entitlements.sh:2
> +

I think we want “set -e” here so that if anything fails unexpectedly the build fails.

> Source/WebKit/Scripts/process-webcontent-entitlements.sh:6
> +    echo "Process WebContent process entitlements for app sandbox.";

I’m not sure I understand the relation to the app sandbox. I thought the Web Content process’s sandbox is unrelated to the app sandbox. I am not sure any of this verbose logging by default is useful

> Source/WebKit/Scripts/process-webcontent-entitlements.sh:8
> +    if [[ ${USE_INTERNAL_SDK} == "YES" ]]; then

I’d change this to my proposed WK_USE_RESTRICTED_ENTITLEMENTS.

> Source/WebKit/Scripts/process-webcontent-entitlements.sh:11
> +        if [[ ${WK_MACOS_1014} == "_MACOS_SINCE_1014" ]]; then

We need the funky BEFORE/SINCE settings for .xcconfig files, where there are no inequality (or equality) operators. Here you should just do something like if (( ${TARGET_MAC_OS_X_VERSION_MAJOR} >= 101400 ));

> Source/WebKit/Scripts/process-webcontent-entitlements.sh:29
> +    echo "No additional entitlements needed for non-macOS builds."

I think this will be annoying to see whenever building for something other than macOS.

> Source/WebKit/Scripts/process-webcontent-entitlements.sh:32
> +echo "Completed."

I think this will be annoying to see whenever building, and unnecessary if the script is made to fail if an error occurs.
Comment 25 Brent Fulgham 2018-05-29 08:44:37 PDT
Comment on attachment 341363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341363&action=review

>> Source/WebKit/Configurations/DebugRelease.xcconfig:50
>> +CODE_SIGN_IDENTITY_MACOS_SINCE_1014_NO = -;
> 
> There is no downside to using the “Safari Engineering” singing identity when targeting earlier macOS versions, so the first simplification I would suggest is
> CODE_SIGN_IDENTITY[sdk=macosx*] = $(CODE_SIGN_IDENTITY_$(USE_INTERNAL_SDK));
> CODE_SIGN_IDENTITY_NO = -;
> CODE_SIGN_IDENTITY_YES = Safari Engineering;
> 
> I think it would be good to be explicit about why we’re using that signing identity (and add some flexibility), though, so I think Base.xcconfig should define
> WK_USE_RESTRICTED_ENTITLEMENTS = $(USE_INTERNAL_SDK);
> 
> and change the first line to say
> CODE_SIGN_IDENTITY[sdk=macosx*] = $(CODE_SIGN_IDENTITY_$(WK_USE_RESTRICTED_ENTITLEMENTS));
> 
> We don’t have to name the “Safari Engineering” signing identity here. I suggest using
> CODE_SIGN_IDENTITY_YES = $(WK_ENGINEERING_CODE_SIGN_IDENTITY);
> and having WK_ENGINEERING_CODE_SIGN_IDENTITY defined in Apple’s HaveInternalSDK.xcconfig, which we already include if it exists.

Will do.

>> Source/WebKit/Configurations/WebContent-OSX.entitlements:11
>>  </plist>
> 
> The name WebContent-OSX.entitlements has already caused confusion. It seems like this file’s role is to contain *some* restricted entitlements that the Web Content service needs in macOS 10.14 or later, so perhaps it can be named WebContent-OSX-restricted.entitlements. Or maybe it’s just the TCC-related entitlements, so it should named appropriately (and the Data Vaults entitlement would be in a different file, or just added by the script).

I'll rename as you suggest.

>> Source/WebKit/Scripts/process-webcontent-entitlements.sh:2
>> +
> 
> I think we want “set -e” here so that if anything fails unexpectedly the build fails.

OK!

>> Source/WebKit/Scripts/process-webcontent-entitlements.sh:8
>> +    if [[ ${USE_INTERNAL_SDK} == "YES" ]]; then
> 
> I’d change this to my proposed WK_USE_RESTRICTED_ENTITLEMENTS.

Will do.

>> Source/WebKit/Scripts/process-webcontent-entitlements.sh:11
>> +        if [[ ${WK_MACOS_1014} == "_MACOS_SINCE_1014" ]]; then
> 
> We need the funky BEFORE/SINCE settings for .xcconfig files, where there are no inequality (or equality) operators. Here you should just do something like if (( ${TARGET_MAC_OS_X_VERSION_MAJOR} >= 101400 ));

OK!

>> Source/WebKit/Scripts/process-webcontent-entitlements.sh:29
>> +    echo "No additional entitlements needed for non-macOS builds."
> 
> I think this will be annoying to see whenever building for something other than macOS.

I will remove it. It was mostly useful during development of this patch.

>> Source/WebKit/Scripts/process-webcontent-entitlements.sh:32
>> +echo "Completed."
> 
> I think this will be annoying to see whenever building, and unnecessary if the script is made to fail if an error occurs.

I will remove it.
Comment 26 Brent Fulgham 2018-05-29 09:39:22 PDT
Created attachment 341491 [details]
Patch
Comment 27 Brent Fulgham 2018-05-29 10:25:05 PDT
(In reply to Alexey Proskuryakov from comment #21)
> > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:819
> > +(with-filter (extension "com.apple.webkit.microphone")
> > +    (allow device-microphone))
> 
> I don't understand how this works (or how this doesn't work without the
> entitlement). Probably best to discuss in person.

I should note that this patch doesn't actually change the sandboxing behavior, it just encapsulates the existing logic in a function so we can call it with an entitlement check on the 'signed' builds, or just permit it in the unsigned builds.
Comment 28 Brent Fulgham 2018-05-30 12:15:40 PDT
Created attachment 341591 [details]
Patch
Comment 29 Brent Fulgham 2018-05-30 14:56:36 PDT
I confirmed ad-hoc signed builds in Release/Debug Open Source builds still work properly.

I also did a Release build locally (in Open Source build mode) and gave the binaries to Alan to try on his machine, and he was able to run WebKit.

I think the current patch (which removes the sandbox change we discussed) is now correct.
Comment 30 Alexey Proskuryakov 2018-05-30 15:08:36 PDT
Comment on attachment 341591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341591&action=review

> Source/WebKit/Configurations/DebugRelease.xcconfig:48
> +CODE_SIGN_IDENTITY_YES = $(WK_ENGINEERING_CODE_SIGN_IDENTITY);

Does this silently evaluate to empty string when WK_USE_RESTRICTED_ENTITLEMENTS is YES, but WK_ENGINEERING_CODE_SIGN_IDENTITY is undefined?

> Source/WebKit/Scripts/process-webcontent-entitlements.sh:26
> +        echo "Processing for Open Source SDK"

This will be confusing in the build output. Processing what?

> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:131
> -#if ENABLE(SANDBOX_EXTENSIONS)
> +#if ENABLE(SANDBOX_EXTENSIONS) && USE(APPLE_INTERNAL_SDK)

Do we actually want this change on all macOS versions?

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10373
> +		7A35F25020BDB7CC003958EC /* Remove stale entitlement file */ = {

Why do we have two of these? Here...

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10391
> +		7A35F25120BDB80C003958EC /* Remove stale entitlement file */ = {

...and here.
Comment 31 Brent Fulgham 2018-05-30 15:17:40 PDT
Comment on attachment 341591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341591&action=review

>> Source/WebKit/Scripts/process-webcontent-entitlements.sh:26
>> +        echo "Processing for Open Source SDK"
> 
> This will be confusing in the build output. Processing what?

Oh, good point. I'll remove this. It was just for debugging.

>> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:131
>> +#if ENABLE(SANDBOX_EXTENSIONS) && USE(APPLE_INTERNAL_SDK)
> 
> Do we actually want this change on all macOS versions?

Eric explained to me that these calls will fail for open source SDKs because you need the special signing stuff for the calls to succeed. So there's no point in executing this code without APPLE_INTERNAL_SDK.

>> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10391
>> +		7A35F25120BDB80C003958EC /* Remove stale entitlement file */ = {
> 
> ...and here.

One of them is for the 'WebContent.xpc' build target, and the other is for 'WebContent.Development.xpc' build target.
Comment 32 Brent Fulgham 2018-05-30 15:22:47 PDT
Comment on attachment 341591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341591&action=review

>> Source/WebKit/Configurations/DebugRelease.xcconfig:48
>> +CODE_SIGN_IDENTITY_YES = $(WK_ENGINEERING_CODE_SIGN_IDENTITY);
> 
> Does this silently evaluate to empty string when WK_USE_RESTRICTED_ENTITLEMENTS is YES, but WK_ENGINEERING_CODE_SIGN_IDENTITY is undefined?

I don't believe WK_ENGINEERING_CODE_SIGN_IDENTITY can be undefined if WK_USE_RESRICTED_ENTITLEMENTS is NO. In my testing you only ever get both, or neither.
Comment 33 Brent Fulgham 2018-05-30 16:29:18 PDT
Created attachment 341617 [details]
Patch for landing
Comment 34 WebKit Commit Bot 2018-05-30 17:08:42 PDT
Comment on attachment 341617 [details]
Patch for landing

Clearing flags on attachment: 341617

Committed r232321: <https://trac.webkit.org/changeset/232321>
Comment 35 WebKit Commit Bot 2018-05-30 17:08:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Dawei Fenton (:realdawei) 2018-05-31 09:41:39 PDT
After this patch it looks we are getting some API test failures on WK1 and Wk2:


https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/4800/steps/run-api-tests/logs/stdio
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/UserMediaSimulateFailedSandbox.mm:121
        Value of: message
          Actual: "denied"
        Expected: [(NSString *)[lastScriptMessage body] UTF8String]
        Which is: "allowed"
        
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/UserMediaSimulateFailedSandbox.mm:139
        Value of: "AbortError,Unable to extend sandbox."
        Expected: [error UTF8String]
        Which is: ""
Comment 37 Brent Fulgham 2018-05-31 10:11:05 PDT
(In reply to David Fenton from comment #36)
> After this patch it looks we are getting some API test failures on WK1 and
> Wk2:
> 
> 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/4800/steps/run-
> api-tests/logs/stdio
>         
>        
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/UserMediaSimulateFailedSandbox.mm:121
>         Value of: message
>           Actual: "denied"
>         Expected: [(NSString *)[lastScriptMessage body] UTF8String]
>         Which is: "allowed"
>         
>         
>        
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/UserMediaSimulateFailedSandbox.mm:139
>         Value of: "AbortError,Unable to extend sandbox."
>         Expected: [error UTF8String]
>         Which is: ""

Looking. I'll land a follow-up fix.
Comment 38 Brent Fulgham 2018-05-31 10:34:15 PDT
(In reply to Brent Fulgham from comment #37)
> (In reply to David Fenton from comment #36)
> > After this patch it looks we are getting some API test failures on WK1 and
> > Wk2:
> Looking. I'll land a follow-up fix.

This will be fixed in Bug 186150.
Comment 39 mitz 2018-05-31 11:40:18 PDT
Comment on attachment 341617 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=341617&action=review

> Source/WebKit/Configurations/WebContentService.xcconfig:-36
>  CODE_SIGN_ENTITLEMENTS_COCOA_TOUCH_NO = $(CODE_SIGN_ENTITLEMENTS_OSX_WITH_XPC_DOMAIN_EXTENSION_$(WK_WEBCONTENT_SERVICE_NEEDS_XPC_DOMAIN_EXTENSION_ENTITLEMENT));
> -CODE_SIGN_ENTITLEMENTS_OSX_WITH_XPC_DOMAIN_EXTENSION_YES = Configurations/WebContent-OSX.entitlements;

It just occurred to me that not having CODE_SIGN_ENTITLEMENTS set at all means that in the production configuration, Xcode is not even going to generate an .xcent file and not going to sign with entitlements (even though we make such a file). I’ll follow up with a patch to fix that.
Comment 40 mitz 2018-05-31 11:41:07 PDT
(In reply to mitz from comment #39)
> Comment on attachment 341617 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341617&action=review
> 
> > Source/WebKit/Configurations/WebContentService.xcconfig:-36
> >  CODE_SIGN_ENTITLEMENTS_COCOA_TOUCH_NO = $(CODE_SIGN_ENTITLEMENTS_OSX_WITH_XPC_DOMAIN_EXTENSION_$(WK_WEBCONTENT_SERVICE_NEEDS_XPC_DOMAIN_EXTENSION_ENTITLEMENT));
> > -CODE_SIGN_ENTITLEMENTS_OSX_WITH_XPC_DOMAIN_EXTENSION_YES = Configurations/WebContent-OSX.entitlements;
> 
> It just occurred to me that not having CODE_SIGN_ENTITLEMENTS set at all
> means that in the production configuration, Xcode is not even going to
> generate an .xcent file and not going to sign with entitlements (even though
> we make such a file). I’ll follow up with a patch to fix that.

Actually, I think Brent can address this in his patch that adds the JIT entitlement, because that one is going to be unconditional.