Bug 195464 – [macOS] Remove the Kerberos rules from the WebContent sandbox

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

Kerberos auth is done in the UIProcess or NetworkProcess now.

[Per Arne Vollan]

rdar://problem/35369230

[Per Arne Vollan]

Created attachment 364021 [details]
Patch

[Per Arne Vollan]

Created attachment 364022 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Per Arne Vollan]

Created attachment 364030 [details]
Patch

[Alexey Proskuryakov]

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?

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 364039 [details]
Patch

[Alexey Proskuryakov]

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?

[Brent Fulgham]

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.

[Per Arne Vollan]

Created attachment 364044 [details]
Patch

[Per Arne Vollan]

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

[Brent Fulgham]

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

[Per Arne Vollan]

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

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

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

[Darin Adler]

Comment on attachment 364044 [details]
Patch

I prefer separate #if with && to nested #if myself.

[Darin Adler]

Oh, sorry, didn’t realize we were waiting to here from experts on what macOS versions this change is needed in.

[youenn fablet]

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

[Brent Fulgham]

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.

[Per Arne Vollan]

Created attachment 364315 [details]
Patch

[Per Arne Vollan]

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

[Brent Fulgham]

Comment on attachment 364315 [details]
Patch

Yay! r=me (wait for EWS)

[Per Arne Vollan]

Comment on attachment 364315 [details]
Patch

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 364315 [details]
Patch

Clearing flags on attachment: 364315

Committed r242769: <https://trac.webkit.org/changeset/242769>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.