Bug 133527 – [iOS] Client-certificate authentication isn’t working

• Component: WebKit2
• Status: RESOLVED FIXED

<rdar://problem/17095692>

Responding to an authentication challenge with an identity-based credential just fails.

Created attachment 232511 [details]
Add encoding support for credentials with identities

[Darin Adler]

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?

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

Attempted a build fix in <http://trac.webkit.org/changeset/169657>.

[Alexey Proskuryakov]

More build fix in <http://trac.webkit.org/r169658> - this code only builds on iOS.