Bug 195464 - [macOS] Remove the Kerberos rules from the WebContent sandbox
Summary: [macOS] Remove the Kerberos rules from the WebContent sandbox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-08 09:41 PST by Per Arne Vollan
Modified: 2019-03-11 20:24 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.41 KB, patch)
2019-03-08 09:42 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (1.44 KB, patch)
2019-03-08 09:43 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (797.51 KB, application/zip)
2019-03-08 10:19 PST, Build Bot
no flags Details
Patch (1.48 KB, patch)
2019-03-08 10:25 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (1.65 KB, patch)
2019-03-08 11:04 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (1.40 KB, patch)
2019-03-08 11:40 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (1.46 KB, patch)
2019-03-11 17:10 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-03-08 09:41:10 PST
Kerberos auth is done in the UIProcess or NetworkProcess now.
Comment 1 Per Arne Vollan 2019-03-08 09:41:30 PST
rdar://problem/35369230
Comment 2 Per Arne Vollan 2019-03-08 09:42:40 PST
Created attachment 364021 [details]
Patch
Comment 3 Per Arne Vollan 2019-03-08 09:43:39 PST
Created attachment 364022 [details]
Patch
Comment 4 Build Bot 2019-03-08 10:19:14 PST
Comment on attachment 364022 [details]
Patch

Attachment 364022 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11427575

Number of test failures exceeded the failure limit.
Comment 5 Build Bot 2019-03-08 10:19:16 PST
Created attachment 364028 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 6 Per Arne Vollan 2019-03-08 10:25:27 PST
Created attachment 364030 [details]
Patch
Comment 7 Alexey Proskuryakov 2019-03-08 10:55:44 PST
Comment on attachment 364030 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        [macOS] Remove the Kerberos rules from the WebContent sandbox

Is it not needed on all macOS versions any more, or just the newest ones?
Comment 8 Per Arne Vollan 2019-03-08 10:59:14 PST
(In reply to Alexey Proskuryakov from comment #7)
> Comment on attachment 364030 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364030&action=review
> 
> > Source/WebKit/ChangeLog:3
> > +        [macOS] Remove the Kerberos rules from the WebContent sandbox
> 
> Is it not needed on all macOS versions any more, or just the newest ones?

That's a good point, we might have to protect this. Thanks for reviewing!
Comment 9 Per Arne Vollan 2019-03-08 11:04:48 PST
Created attachment 364039 [details]
Patch
Comment 10 Alexey Proskuryakov 2019-03-08 11:13:14 PST
Comment on attachment 364039 [details]
Patch

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

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:695
>  (allow user-preference-read

There is also edu.mit.Kerberos preference rule in this file.

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:708
> +        (literal "/private/etc/services")
> +        (literal "/private/etc/host"))

I'm confused by why this is still needed. Didn't all network loading move from WebContent on all macOS versions?
Comment 11 Brent Fulgham 2019-03-08 11:18:07 PST
Comment on attachment 364039 [details]
Patch

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

>> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:708
>> +        (literal "/private/etc/host"))
> 
> I'm confused by why this is still needed. Didn't all network loading move from WebContent on all macOS versions?

That's true. I chickened out and suggested keeping this because I wasn't sure if CFURL needed it to do its work, and that is still used in the WebContent process.
Comment 12 Per Arne Vollan 2019-03-08 11:40:50 PST
Created attachment 364044 [details]
Patch
Comment 13 Per Arne Vollan 2019-03-08 11:42:05 PST
(In reply to Brent Fulgham from comment #11)
> Comment on attachment 364039 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364039&action=review
> 
> >> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:708
> >> +        (literal "/private/etc/host"))
> > 
> > I'm confused by why this is still needed. Didn't all network loading move from WebContent on all macOS versions?
> 
> That's true. I chickened out and suggested keeping this because I wasn't
> sure if CFURL needed it to do its work, and that is still used in the
> WebContent process.

I disallowed it in the latest patch, but perhaps we should keep it? Thanks for reviewing, all!
Comment 14 Brent Fulgham 2019-03-08 11:43:30 PST
(In reply to Per Arne Vollan from comment #13)
> I disallowed it in the latest patch, but perhaps we should keep it? Thanks
> for reviewing, all!

Let's see what EWS says. If that works, we should proceed.
Comment 15 Per Arne Vollan 2019-03-08 11:44:47 PST
(In reply to Brent Fulgham from comment #14)
> (In reply to Per Arne Vollan from comment #13)
> > I disallowed it in the latest patch, but perhaps we should keep it? Thanks
> > for reviewing, all!
> 
> Let's see what EWS says. If that works, we should proceed.

Great, thanks!
Comment 16 Alexey Proskuryakov 2019-03-08 13:10:15 PST
Didn't we stop doing network loading in WebContent on all macOS versions though? If so, Kerberos also shouldn't be needed I suspect.

Maybe the original patch was correct.
Comment 17 Ryosuke Niwa 2019-03-09 18:20:57 PST
(In reply to Alexey Proskuryakov from comment #16)
> Didn't we stop doing network loading in WebContent on all macOS versions
> though? If so, Kerberos also shouldn't be needed I suspect.

I believe we have. +Alex, Youenn.
Comment 18 Darin Adler 2019-03-10 15:34:42 PDT
Comment on attachment 364044 [details]
Patch

I prefer separate #if with && to nested #if myself.
Comment 19 Darin Adler 2019-03-10 15:35:25 PDT
Oh, sorry, didn’t realize we were waiting to here from experts on what macOS versions this change is needed in.
Comment 20 youenn fablet 2019-03-10 22:36:18 PDT
(In reply to Ryosuke Niwa from comment #17)
> (In reply to Alexey Proskuryakov from comment #16)
> > Didn't we stop doing network loading in WebContent on all macOS versions
> > though? If so, Kerberos also shouldn't be needed I suspect.
> 
> I believe we have. +Alex, Youenn.

Right, I think we are doing all networking in NetworkProcess since we moved app cache to NetworkProcess (rdar://problem/17969182).
Comment 21 Brent Fulgham 2019-03-11 09:34:17 PDT
Comment on attachment 364044 [details]
Patch

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

r- to remove the conditional check and just remove the GSS access.

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:694
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101500

As discussed in the bug, we shouldn't need this version check since all WebKit builds (even to downlevel OSes) have all such interactions happening in the Network Process.
Comment 22 Per Arne Vollan 2019-03-11 17:10:30 PDT
Created attachment 364315 [details]
Patch
Comment 23 Per Arne Vollan 2019-03-11 17:11:03 PDT
(In reply to Brent Fulgham from comment #21)
> Comment on attachment 364044 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364044&action=review
> 
> r- to remove the conditional check and just remove the GSS access.
> 
> > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:694
> > +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101500
> 
> As discussed in the bug, we shouldn't need this version check since all
> WebKit builds (even to downlevel OSes) have all such interactions happening
> in the Network Process.

Thanks for reviewing, all! I have updated the patch.
Comment 24 Brent Fulgham 2019-03-11 17:12:26 PDT
Comment on attachment 364315 [details]
Patch

Yay! r=me (wait for EWS)
Comment 25 Per Arne Vollan 2019-03-11 19:56:05 PDT
Comment on attachment 364315 [details]
Patch

Thanks for reviewing!
Comment 26 WebKit Commit Bot 2019-03-11 20:24:33 PDT
Comment on attachment 364315 [details]
Patch

Clearing flags on attachment: 364315

Committed r242769: <https://trac.webkit.org/changeset/242769>
Comment 27 WebKit Commit Bot 2019-03-11 20:24:35 PDT
All reviewed patches have been landed.  Closing bug.