Bug 170074 – Re-enable the network and web processes' keychain access to fix client certificate authentication

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

[John Wilander]

https://bugs.webkit.org/show_bug.cgi?id=163710 caused a regression in client certificate authentication for macOS.

[John Wilander]

rdar://problem/31095987

[John Wilander]

Created attachment 305321 [details]
Patch

[Brent Fulgham]

Comment on attachment 305321 [details]
Patch

This looks great to me. r=me.

[John Wilander]

Comment on attachment 305321 [details]
Patch

Thanks, Brent!

[Alexey Proskuryakov]

Comment on attachment 305321 [details]
Patch

Shouldn't we remove the ifdef from WebProcess.sb too? There are still loads that occur in WebContent.

[John Wilander]

I do not have a test case that requires a WebProcess.sb changes to pass. My failing test only needed the change in this patch. I'm happy to revisit.

[Brent Fulgham]

(In reply to Alexey Proskuryakov from comment #5)
> Comment on attachment 305321 [details]
> Patch
> 
> Shouldn't we remove the ifdef from WebProcess.sb too? There are still loads
> that occur in WebContent.

We believe the loads in WebContent are doing the right thing using the updated APIs, and so we want to keep them.

If we discover evidence that we need to weaken WebProcess, too, we can do so once we have data indicating it is actually needed.

[WebKit Commit Bot]

Comment on attachment 305321 [details]
Patch

Clearing flags on attachment: 305321

Committed r214389: <http://trac.webkit.org/changeset/214389>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

There was no evidence that the original change was safe, and I don't see any evidence presented that it's safe to keep in WebProcess.

What did you test to see client certificate authentication work in WebProcess?

[Alexey Proskuryakov]

Resource loading has always required identical sandbox rules in WebContent and Networking processes, so it's quite unlikely that this case is different.

[Alexey Proskuryakov]

The reason why I'm re-opening this bug and not just filing a new one is that I do believe that the above invariant is important, so we should either restore it right away, or positively prove that it's not required any more.

[Brent Fulgham]

That is certainly important information. It's a shame no one on the architecture team ever bothered to document this anywhere!

Can you share which aspects of loading must be kept the same between the two processes? Certainly there are many more exceptions in WebProcess than NetworkProcess. Are any of them running afoul of this invariant,too?

John: Can you please provide a supplemental patch to support this undocumented invariant, and please add a comment somewhere so that we remember to keep these in sync in the future?

[John Wilander]

Created attachment 305382 [details]
Patch

[John Wilander]

My patch reverts the version check instead of doing an exact match with the network process sandbox. I don't want to change pre-existing sandbox behavior before we get client certificate authentication working again.

[Brent Fulgham]

Comment on attachment 305382 [details]
Patch

R=me

[WebKit Commit Bot]

Comment on attachment 305382 [details]
Patch

Clearing flags on attachment: 305382

Committed r214404: <http://trac.webkit.org/changeset/214404>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.