Bug 134878

Summary: [iOS] Networking process writes persistent credentials to the keychain
Product: WebKit Reporter: mitz
Component: WebKit2Assignee: mitz
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, rniwa, sam
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Use a session-persistent credential instead of a permanent one
none
Route CFNetwork calls to Security API through to the UI process
sam: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 none

Description mitz 2014-07-13 19:28:19 PDT
<rdar://problem/17657391>
Comment 1 mitz 2014-07-13 19:30:23 PDT
Created attachment 234838 [details]
Use a session-persistent credential instead of a permanent one
Comment 2 mitz 2014-07-13 20:25:06 PDT
The patch is missing a change to WebCore’s exports file.
Comment 3 Alexey Proskuryakov 2014-07-13 20:52:57 PDT
Comment on attachment 234838 [details]
Use a session-persistent credential instead of a permanent one

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

It's going to be sad without shimming. Perhaps we should indicate why we fail, so that developers know why their APIs don't work as expected?

I think that there is a way to implement shimming though, see <rdar://problem/11965615>.

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:119
> +#if PLATFORM(IOS)

I'm somewhat torn about suggesting to move this to ResourceHandle::receivedCredential(), where we have very similar code.

Existing code seems like it probably doesn't handle downloading.

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:122
> +    if (credential.persistence() == CredentialPersistencePermanent) {

Do proxy credentials go through this code path? I suspect that they do, and that we very much do want to store them persistently anyway.
Comment 4 mitz 2014-07-13 21:43:08 PDT
(In reply to comment #3)
> (From update of attachment 234838 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234838&action=review
> 
> It's going to be sad without shimming. Perhaps we should indicate why we fail, so that developers know why their APIs don't work as expected?
> 
> I think that there is a way to implement shimming though, see <rdar://problem/11965615>.

If that works, it’s much better than my patch, so I am going to try that first.

> 
> > Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:119
> > +#if PLATFORM(IOS)
> 
> I'm somewhat torn about suggesting to move this to ResourceHandle::receivedCredential(), where we have very similar code.
> 
> Existing code seems like it probably doesn't handle downloading.
> 
> > Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:122
> > +    if (credential.persistence() == CredentialPersistencePermanent) {
> 
> Do proxy credentials go through this code path? I suspect that they do, and that we very much do want to store them persistently anyway.

I think proxy credentials go through this same code path, but I don’t understand why we’d ever want to Networking process to write to the keychain. We can discuss that separately from this bug, though.
Comment 5 mitz 2014-07-14 00:08:03 PDT
Created attachment 234843 [details]
Route CFNetwork calls to Security API through to the UI process
Comment 6 Build Bot 2014-07-14 01:19:34 PDT
Comment on attachment 234843 [details]
Route CFNetwork calls to Security API through to the UI process

Attachment 234843 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5769428302036992

New failing tests:
media/W3C/video/src/src_reflects_attribute_not_source_elements.html
Comment 7 Build Bot 2014-07-14 01:19:36 PDT
Created attachment 234845 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 8 mitz 2014-07-14 09:19:43 PDT
Fixed in <http://trac.webkit.org/r171066>.