| Summary: | Re-enable the network and web processes' keychain access to fix client certificate authentication | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | John Wilander <wilander> | ||||||
| Component: | WebKit2 | Assignee: | John Wilander <wilander> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | achristensen, bfulgham, commit-queue, webkit-bug-importer, wilander | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=163710 | ||||||||
| Attachments: |
|
||||||||
|
Description
John Wilander
2017-03-24 13:39:55 PDT
Created attachment 305321 [details]
Patch
Comment on attachment 305321 [details]
Patch
This looks great to me. r=me.
Comment on attachment 305321 [details]
Patch
Thanks, Brent!
Comment on attachment 305321 [details]
Patch
Shouldn't we remove the ifdef from WebProcess.sb too? There are still loads that occur in WebContent.
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. (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. Comment on attachment 305321 [details] Patch Clearing flags on attachment: 305321 Committed r214389: <http://trac.webkit.org/changeset/214389> All reviewed patches have been landed. Closing bug. 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? Resource loading has always required identical sandbox rules in WebContent and Networking processes, so it's quite unlikely that this case is different. 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. 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? Created attachment 305382 [details]
Patch
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. Comment on attachment 305382 [details]
Patch
R=me
Comment on attachment 305382 [details] Patch Clearing flags on attachment: 305382 Committed r214404: <http://trac.webkit.org/changeset/214404> All reviewed patches have been landed. Closing bug. |