reCaptcha verification is hardcoded to use plaintext HTTP

The reCaptcha API which BridgeDB uses hardcodes the CAPTCHA verification submission to occur over plaintext HTTP. See their source code and grep for 'verify'.

Their API is also blocking. BridgeDB would probably go faster if it were asynchronous.

added bridgedb-0.1.5 bridgedb-https component::circumvention/bridgedb owner::isis priority::high resolution::fixed status::closed type::defect labels

I wrote a Twisted reCaptcha client which only uses SSL. It also does full certificate chain verification and cert hostname verification on a per-request basis.

It's much faster. And it has full unittest coverage. :)

I left the methods for creating the bridgedb.crypto.SSLVerifyingContextFactory and twisted.web.client.Agent separate from the main reCaptcha API functions, so we can use them for other requests. (For example, there is another blocking HTTP request in bridgedb.[R|c]aptcha.Recaptcha.get(), which obtains the CAPTCHA image and challenge string from the reCaptcha server, that could easily benefit from this as well.) Possible this SSL client-side stuff should be separate somewhere, but for now I just put it all in bridgedb.txrecaptcha (except for bridgedb.crypto.SSLVerifyingContextFactory.

Please review!

Trac:
Status: new to needs_review

Replying to isis:

I wrote a Twisted reCaptcha client which only uses SSL. It also does full certificate chain verification and cert hostname verification on a per-request basis.

Mmmm

It's much faster. And it has full unittest coverage. :)

Mmmmm

I left the methods for creating the bridgedb.crypto.SSLVerifyingContextFactory and twisted.web.client.Agent separate from the main reCaptcha API functions, so we can use them for other requests. (For example, there is another blocking HTTP request in bridgedb.[R|c]aptcha.Recaptcha.get(), which obtains the CAPTCHA image and challenge string from the reCaptcha server, that could easily benefit from this as well.) Possible this SSL client-side stuff should be separate somewhere, but for now I just put it all in bridgedb.txrecaptcha (except for bridgedb.crypto.SSLVerifyingContextFactory.

Please review!

I guess I should actually tell you that I reviewed it!

Ran 263 tests in 215.672s

PASSED (skips=1, successes=262)

It looks good, I don't have any blockers on the code. A couple minor comments on the unit test.

1. These appear to be the same test, but maybe I'm missing something:

    def test_trueResponse(self):
        """A valid API response which states 'true' should result in
        ``RecaptchaResponse.is_valid`` being ``True``.
        """
        responseBody = "true\nsome-reason-or-another\n"
        response = self._test(responseBody, ResponseDone)
        self.assertIsInstance(response, txrecaptcha.RecaptchaResponse)
        self.assertTrue(response.is_valid)
        self.assertEqual(response.error_code, "some-reason-or-another")

    def test_responseDone(self):
        """A valid response body with a ``ResponseDone`` should result in
        ``RecaptchaResponse.is_valid`` which is ``True``.
        """
        responseBody = "true\nsome-reason-or-another\n"
        response = self._test(responseBody, ResponseDone)
        self.assertIsInstance(response, txrecaptcha.RecaptchaResponse)
        self.assertTrue(response.is_valid)
        self.assertEqual(response.error_code, "some-reason-or-another")

2. thee

    def test_cbRequest(self):
        """Send a :class:`MockResponse` and check that thee resulting protocol

I'm unable to test it, but as soon as the remainder of the patch with an implementation in CaptchaProtectedResource looks good, and think it's mergable.

Replying to sysrqb:

I guess I should actually tell you that I reviewed it!

Thanks! :D

It looks good, I don't have any blockers on the code. A couple minor comments on the unit test.

1. These appear to be the same test, but maybe I'm missing something:

Oopsies. That is not supposed to be the same. One of those is supposed to be something else, like a twisted.web.client.ConnectionLost instead of ResponseDone...

Good catch.

2. thee {{{ def test_cbRequest(self): """Send a :class:MockResponse and check that thee resulting protocol }}}

It's expecting an aristocratic protocol, you know? Like twisted.protocols.stateful.HerRoyalHighnessTCP.

Kidding! Thanks, I'll fix that one too. :)

I'm unable to test it, but as soon as the remainder of the patch with an implementation in CaptchaProtectedResource looks good, and think it's mergable.

There's now a [ branch] which is exactly these commits, unchanged, but they are rebased on top of the other CAPTCHA branch for #10809 (moved). There is one additional commit to use bridgedb.txrecaptcha in bridgedb.HTTPServer and bridgedb.captcha.

All the tests still pass, but we currently have like zero testing for bridgedb.HTTPServer. I am unsure if I should focus time on that before getting started with the database backend improvements.

Aristocratic typo and duplicated unittest fixed.

Merged for version 0.1.5 in this commit.

Trac:
Resolution: N/A to fixed
Keywords: N/A deleted, bridgedb-0.1.5 added
Status: needs_review to closed

closed

mentioned in issue #11219 (moved)

moved to tpo/anti-censorship/bridgedb#11127 (closed)

mentioned in issue tpo/anti-censorship/bridgedb#11219 (closed)