Bug 185526 – [macOS] WebProcess needs TCC entitlements for media capture (Take 2)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/36674649>

[Brent Fulgham]

Created attachment 340118 [details]
Patch

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.

[Brent Fulgham]

Created attachment 340120 [details]
Patch

[Brent Fulgham]

(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.

(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.

[Brent Fulgham]

Created attachment 340134 [details]
Patch

[Brent Fulgham]

(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 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.

[Brent Fulgham]

(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.

(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.

[Brent Fulgham]

Created attachment 340645 [details]
Patch

Does attachment 340645 [details] address comment 9?

[Brent Fulgham]

(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.

(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?

[Brent Fulgham]

(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.

[Brent Fulgham]

Created attachment 340728 [details]
Patch

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.

[Brent Fulgham]

Created attachment 341313 [details]
Patch

[Brent Fulgham]

Created attachment 341363 [details]
Patch

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

These comments are for the earlier version of the patch. I didn't look at the newest one yet.

[Brent Fulgham]

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 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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 341491 [details]
Patch

[Brent Fulgham]

(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.

[Brent Fulgham]

Created attachment 341591 [details]
Patch

[Brent Fulgham]

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.

[Alexey Proskuryakov]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 341617 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 341617 [details]
Patch for landing

Clearing flags on attachment: 341617

Committed r232321: <https://trac.webkit.org/changeset/232321>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dawei Fenton (:realdawei)]

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: ""

[Brent Fulgham]

(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.

[Brent Fulgham]

(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 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.

(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.