Bug 31844 – SocketStreamHandleCFNet should support CONNECT proxy credentials

• Component: Platform
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

SocketStreamHandleCFNet should support CONNECT proxy credentials. Patch forthcoming.

[Alexey Proskuryakov]

Created attachment 43796 [details]
proposed patch

[Brady Eidson]

Comment on attachment 43796 [details]
proposed patch

r+ with comments

> +void SocketStreamHandle::addCONNECTCredentials(CFHTTPMessageRef proxyResponse)
> +{
> +    // Creating a temporary request to make CFNetwork apply credentials to it. Unfortunately, this cannot work with NTLM authentication.
> +    RetainPtr<CFHTTPMessageRef> dummyRequest(AdoptCF, CFHTTPMessageCreateRequest(0, CFSTR("GET"), m_httpsURL.get(), kCFHTTPVersion1_1));
> +    RetainPtr<CFHTTPAuthenticationRef> authentication(AdoptCF, CFHTTPAuthenticationCreateFromResponse(0, proxyResponse));
> +
> +    if (!CFHTTPAuthenticationRequiresUserNameAndPassword(authentication.get())) {
> +        // That's all we can offer...
> +        m_client->didFail(this, SocketStreamError()); // FIXME: Provide a sensible error.
> +        return;
> +    }

Suggest re-ordering the above to create the authentication object, check if it needs username/password, do the early return if it doesn't, then create the dummy request.

Plus, how are we going to track getting a sensible error for this(x1)?

> +
> +    int port = 0;
> +    CFNumberGetValue(m_proxyPort.get(), kCFNumberIntType, &port);
> +    RetainPtr<CFStringRef> methodCF(AdoptCF, CFHTTPAuthenticationCopyMethod(authentication.get()));
> +    RetainPtr<CFStringRef> realmCF(AdoptCF, CFHTTPAuthenticationCopyRealm(authentication.get()));
> +    ProtectionSpace protectionSpace(String(m_proxyHost.get()), port, ProtectionSpaceProxyHTTPS, String(realmCF.get()), authenticationSchemeFromAuthenticationMethod(methodCF.get()));
> +    String login;
> +    String password;
> +    if (!m_sentStoredCredentials && getStoredCONNECTProxyCredentials(protectionSpace, login, password)) {
> +        // Try to get sotred credentials, if we haven't tried those already.

Typo on this comment.  Plus, I think you mean to say "Try to *use* stored credentials..."?

> +        RetainPtr<CFStringRef> loginCF(AdoptCF, login.createCFString());
> +        RetainPtr<CFStringRef> passwordCF(AdoptCF, password.createCFString());
> +        Boolean appliedCredentials = CFHTTPMessageApplyCredentials(dummyRequest.get(), authentication.get(), loginCF.get(), passwordCF.get(), 0);
> +        ASSERT_UNUSED(appliedCredentials, appliedCredentials);
> +
> +        RetainPtr<CFStringRef> proxyAuthorizationString(AdoptCF, CFHTTPMessageCopyHeaderFieldValue(dummyRequest.get(), CFSTR("Proxy-Authorization")));
> +
> +        if (!proxyAuthorizationString) {
> +            // Fails e.g. for NTLM auth.
> +            m_client->didFail(this, SocketStreamError()); // FIXME: Provide a sensible error.
> +            return;
> +        }

How are we going to track getting a sensible error for this (x2)?

> +
> +        // Setting the authorization results in a new connection attempt.
> +        wkSetCONNECTProxyAuthorizationForStream(m_readStream.get(), proxyAuthorizationString.get());
> +        m_sentStoredCredentials = true;
> +        return;
> +    }
> +
> +    // FIXME: Ask the client if credentials could not be found.

How are we going to track asking the client?

> +
> +    m_client->didFail(this, SocketStreamError()); // FIXME: Provide a sensible error.

How are we going to track getting a sensible error for this(x3)?

[Brady Eidson]

> > +    if (!m_sentStoredCredentials && getStoredCONNECTProxyCredentials(protectionSpace, login, password)) {
> > +        // Try to get sotred credentials, if we haven't tried those already.
>
> Typo on this comment.  Plus, I think you mean to say "Try to *use* stored
> credentials..."?

Reading it again, I now parse it how you intended it.  I still think it could be clearer - maybe "Try to apply stored credentials"?

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/51354>.

> Suggest re-ordering the above to create the authentication object, check if it
> needs username/password, do the early return if it doesn't, then create the
> dummy request.

Good catch, done!

> Plus, how are we going to track getting a sensible error for this(x1)?

With a FIXME, for now. The error doesn't go anywhere yet.

> Typo on this comment.  Plus, I think you mean to say "Try to *use* stored
> credentials..."?

Fixed, and re-worded a bit.