Summary: | [iOS] Client-certificate authentication isn’t working | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||
Component: | WebKit2 | Assignee: | mitz | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ap | ||||
Priority: | P1 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
mitz
2014-06-04 16:44:54 PDT
Created attachment 232511 [details]
Add encoding support for credentials with identities
Comment on attachment 232511 [details] Add encoding support for credentials with identities View in context: https://bugs.webkit.org/attachment.cgi?id=232511&action=review > Source/WebCore/WebCore.exp.in:2919 > +#if PLATFORM(COCOA) This file is only used for PLATFORM(COCOA), so I suggest removing this #if and re-sorting the file. > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:488 > +#endif > encoder << credential.user() << credential.password(); I suggest adding a blank line here. > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:505 > + RetainPtr<CFArrayRef> certificates; I’d move this down past the decoding of hasCertificates. > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:522 > +#endif > String user; I suggest adding a blank line here. > Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:587 > + SecIdentityCopyCertificate(identity, &certificate); Can this ever fail? If so, the CFRelease below will crash. Same pattern happens 3 times. Is crashing the right behavior or do we want something else? Comment on attachment 232511 [details]
Add encoding support for credentials with identities
I'm unsure about the status of code in AuthenticationManager::tryUseCertificateInfoForChallenge. Is it still needed, now that actual identities are being passed?
(In reply to comment #3) > (From update of attachment 232511 [details]) > I'm unsure about the status of code in AuthenticationManager::tryUseCertificateInfoForChallenge. Is it still needed, now that actual identities are being passed? At least for now, yes, it is still needed, because of the C SPI. Clients of the C SPI pass an identity by creating a WKCredential with a WKCretificateInfoRef, which they create from certificates they extract from the identity (a key reference isn’t passed at all, the Network process resorts to looking it up based on the certificate). We don’t have a way to create a WKCredentialRef from a SecIdentityRef. Comment on attachment 232511 [details] Add encoding support for credentials with identities View in context: https://bugs.webkit.org/attachment.cgi?id=232511&action=review >> Source/WebCore/WebCore.exp.in:2919 >> +#if PLATFORM(COCOA) > > This file is only used for PLATFORM(COCOA), so I suggest removing this #if and re-sorting the file. I don’t know what I was thinking. >> Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:587 >> + SecIdentityCopyCertificate(identity, &certificate); > > Can this ever fail? If so, the CFRelease below will crash. Same pattern happens 3 times. Is crashing the right behavior or do we want something else? This one and the next one can’t fail, as far as I can tell, but the third one can fail. I think in that case we should avoid crashing, and encode something that decodes as a null identity, and make sure that we don’t crash on the receiving side when the identity is null. Fixed in <http://trac.webkit.org/r169655>. Attempted a build fix in <http://trac.webkit.org/changeset/169657>. More build fix in <http://trac.webkit.org/r169658> - this code only builds on iOS. |