Allow the user to provide their own IPolicyForHTTPS implementation.

[ldanielburr]

This allows a user to, for example, connect to a server using a self-signed certificate. Could definitely use some guidance as to how best to test this.

[ldanielburr]

So the failed builds indicate that this approach doesn't work for Twisted < 14.0.0, as that version introduced BrowserLikePolicyForHTTPS, upon which the PR relies.

It should be possible to provide the equivalent functionality for older versions of Twisted, by providing a suitably configured instance of twisted.internet.ssl.ClientContextFactory, but that will mean some version-sniffing.

[itamarst]

I would personally suggest dropping support for older versions of Twisted, given their default TLS policy was insecure.

[glyph]

Seeing BEGIN PRIVATE KEY anywhere in a PR is somewhat worrisome, as it provides an opportunity for naive users to re-use publicly known keys. Not to mention the fact that github security scanners will forever be notifying the org admins about this "security problem" :). I'd really rather that this go into a key-bootstrapping script.

[ldanielburr]

My apologies, I wasn't aware that GH would spam admins over this. I naively based this example on some twisted docs that included keys and certs as part of the code, thinking that it would be a familiar presentation, but I realize that probably wasn't the best choice,

I will rework this to issue the requisite OpenSSL commands, etc.

[glyph]

GH doesn't regularly spam admins, but on an inconsistent basis, security consulting type folks will go trawling through repositories looking for matches of that regex (I have to admit I type it in sometimes just to make myself sad; many usages are not so harmless).

The twisted examples that ship .pem files with the code are definitely the bad example I learned from here.

[glyph]

Sorry that it took so long to review this; treq's review queue isn't integrated into Twisted's and so it has often escaped my attention. Would you be willing to address this stuff so we can get it merged?

[glyph]

@ldanielburr that is (apparently I can't assign you because you aren't in the org or something?)

[ldanielburr]

@glyph - Yes, I would be happy to address the review feedback by tomorrow, provided the question of how to set the twisted version requirements is answered. Thanks for reviewing!

[glyph]

No problem! The spurious build failures should be fixed now, so if you want to merge master or rebase you should be able to get a clean run.

[codecov-io]

Current coverage is 95.69%

Merging #80 into master will decrease coverage by -0.13% as of a812056

Coverage Diff

@@            master     #80   diff @@
======================================
  Files           19      19       
  Stmts         1411    1414     +3
  Branches       112     113     +1
  Methods          0       0       
======================================
+ Hit           1352    1353     +1
- Partial         19      20     +1
- Missed          40      41     +1

Powered by Codecov

[ldanielburr]

@glyph - Given that PR #76 was merged a while ago, I think this PR should be closed. Now that a caller can provide their own agent implemention as an argument when invoking treq, any IPolicyForHTTPS concerns can be handled by passing the appropriate object to the agent, in the contextFactory argument.

It might still be useful to revise the documentation in this PR to demonstrate how to work with a self-signed cert, how to work with a client cert, etc.

[glyph]

I'm inclined to agree, since you're the one who originally opened it. As always feel free to make a new PR if you change your mind :).