ca_certs from environment or certifi #117

dirkboye · 2018-09-25T10:02:42Z

This PR is similar to #52 with following differences:

environment variable "HTTPLIB2_CA_CERTS" also overrides certifi.where()

I added some unit tests.

codecov · 2018-09-25T10:06:36Z

Codecov Report

Merging #117 into master will increase coverage by 0.31%.
The diff coverage is 87.09%.

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
+ Coverage   73.13%   73.45%   +0.31%     
==========================================
  Files           6        8       +2     
  Lines        2479     2535      +56     
==========================================
+ Hits         1813     1862      +49     
- Misses        666      673       +7

Impacted Files	Coverage Δ
python2/httplib2/__init__.py	`82.29% <100%> (+0.04%)`	⬆️
python3/httplib2/__init__.py	`83.26% <100%> (+0.01%)`	⬆️
python3/httplib2/certs.py	`86.2% <86.2%> (ø)`
python2/httplib2/certs.py	`86.2% <86.2%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc73888...0014592. Read the comment docs.

temoto

I like environ over certifi better than #52

temoto · 2018-09-25T18:41:56Z

python3/httplib2/certs.py

+import os.path
+
+try:
+    from certifi import where as get_cacerts


ca_certs_locater is not mentioned here. Sorry, just found that it has same py2/3 difference in approved #52

Should work same in py2/3.

temoto · 2018-09-25T18:49:55Z

tests/test_cacerts_from_env.py

+import httplib2
+
+try:
+    import certifi


Test requirements don't specify certifi explicitly, but it may be installed as dependency of another packet in future. So seemingly unrelated change will affect how CA is tested.

Solution: separate certifi import and usage. That allows to mock presence of certifi in test. Otherwise to test with/out certifi you'd have to run subtest in another Python process or patch/reload sys.modules[httplib2.certs] or something even less reliable.

# httplib2/certs.py import os # import os.path - this is not needed really, previous line does the job, sorry it also slipped in original review ca_certs_locater = None try: import ca_certs_locater except ImportError: pass certifi = None try: import certifi except ImportError: pass BUILTIN_CA_CERTS = os.path.join(...) def where(): env = os.environ.get('HTTPLIB2_CA_CERTS') if env is not None: return env if ca_certs_locater: return ca_certs_locater.get() if certifi: return certifi.where() return BUILTIN_CA_CERTS

dirkboye · 2018-09-26T05:21:11Z

python2/httplib2/certs.py

+    pass
+
+
+def certifi_where():


I am using a method definition here which I try to override with the import later as this is easier to use with mock.

temoto · 2018-09-26T07:03:14Z

Thanks. ca_certs_loader is not used in py3.

dirkboye · 2018-09-26T09:30:49Z

You're right. Now the docs reflect the correct order of return value of where() and in python3 docs
there is no mention of ca_certs_locater

temoto · 2018-09-26T10:53:40Z

and in python3 docs there is no mention of ca_certs_locater
Do you refer to one of web published docs? It's a bit messy right now.

Suppose you have application using httplib2 and ca_certs_loader, then you upgrade from py2 to py3 and bam it stops loading certs. It's also possible that this part is not covered by tests so you find that httplib2/py3 ignores ca_certs_loader in production.

And yes it's broken right now and not part of environ variable support. IMHO still has to be done so why not now.

temoto · 2018-09-26T11:03:41Z

@httplib2/maintainers review ping and see below

Important API design to decide: what to do with empty string HTTPLIB2_CA_CERTS="" python app

Right now it means "validate against empty CA list", so all TLS will fail. Arguably such behavior could be invoked via value like /dev/null or none in favor of more useful behavior like "proceed with loader/certifi/default".

@dirkboye please have your say here too

temoto · 2018-09-26T11:06:50Z

Vote here for HTTPLIB2_CA_CERTS="" python app means user wants default TLS fail except explicit Http(ca_certs=...)

temoto · 2018-09-26T11:07:34Z

Vote here for HTTPLIB2_CA_CERTS="" python app means proceed with loader/certifi/bundled as if no env variable specified.

temoto · 2018-09-26T11:10:47Z

Vote here for HTTPLIB2_CA_CERTS="" python app is error with helpful message.

dirkboye · 2018-09-26T12:09:20Z

should I put in the import of ca_certs_locator in py3 and add a test for it?

temoto · 2018-09-26T12:15:51Z

Yes, please.

dirkboye · 2018-09-26T12:45:48Z

python2/httplib2/certs.py

+
+def where():
+    env = os.environ.get("HTTPLIB2_CA_CERTS")
+    if env: # if not None and != ""


if env: to be changed based on design vote

python2/httplib2/certs.py

python3/httplib2/certs.py

stinky2nine · 2018-09-26T20:28:01Z

python3/httplib2/certs.py

+
+
+def certifi_where():
+    return None


Maybe this should raise NotImplementedError

If certifi is available, this function will be replaced by import below. If not available, this function will not be called.

@dirkboye so maybe foo=None ; import mod ; foo = mod.where is more readable version afterall.

python2/httplib2/certs.py

tests/test_cacerts_from_env.py

dirkboye · 2018-09-27T04:26:42Z

As a side note, this PR would also solve #114

temoto · 2018-09-27T09:53:45Z

tests/test_cacerts_from_env.py

+
+
+def test_certs_file_from_builtin():
+    httplib2.certs.certifi_available = False


This assignment affects other tests, so should be returned to previous value.

Something like this will reliably return global state.

with mock.patch("httplib2.certs.certifi_available", True): assert where()...

Done. I used @mock.patch for the available flags

Should I use them for the where methods as well? Then I'll need to define them as None before try except block so that they are defined and I can patch them.

Yes, it's not about any particular flag. Rule is: tests must either pass or fail only because main code is valid or not.

Tests must behave same way ...

when certifi/custom_ca_loader is installed and not, unless test suite installs package in controllable manner

when some environment variable is set or not, unless test runner controls environment

when tests order were shuffled - this excludes modification of any global variables

Please have patience mate, I'm sorry this is not ideal one step review like change in these places then merged. You're doing important work.

cdent · 2018-09-27T11:21:09Z

I've voted for:

Vote here for HTTPLIB2_CA_CERTS="" python app is error with helpful message.

because of the two options, the alternative is not good. If the variable is set in the environment, in any form, it should mean something and not be ignored. If the impact of the empty string is undefined, then that should result in an error. The correct way for HTTPLIB2_CA_CERTS to mean "ignore this var" is to unset it in the environment.

A third option for var set with the empty string is that there are no certs. I think this makes most semantic sense in terms of the var itself, but if it makes no sense in terms of the behavior of the system, then having an error on empty string is the reasonable next choice.

Otherwise: The code, while feeling a bit cumbersome, makes sense and seems to do the desired job.

stinky2nine · 2018-09-27T13:12:20Z

In shell, "unset" environment variables is the right thing to do. So +1 to error in Python.

ca_certs = os.environ.get('HTTPLIB2_CA_CERTS')
if ca_certs is not None and not ca_certs:
  raise SomeException

temoto · 2018-09-27T14:04:57Z

It's if ca_certs=="" right?

stinky2nine · 2018-09-27T14:49:12Z

Or something like if not os.path.isfile(ca_certs)

dirkboye · 2018-09-28T15:44:13Z

I addressed the issues regarding wrong filename in environment variable by
throwing a RuntimeError if no file exists

Also decorator mocks are now used everywhere

temoto · 2018-09-29T18:46:36Z

@cdent @stinky2nine ping

temoto · 2018-10-06T05:29:56Z

@dirkboye do you use httplib2 with this patch somewhere? I want to release it, hopefully with more confidence from field usage.

dirkboye · 2018-10-10T12:17:14Z

Yes I am using it together with google/containerregistry.
There's only a little caveat:
They guess the scheme (http/https) based on the endpoint.
Our registry endpoint has ".local" in it.
https://github.com/google/containerregistry/blob/657fcea7f1206de849058517bd0a0b5bdc92d325/client/v2_2/docker_http_.py#L415
So with our DNS entry I'll always get a http scheme (so i have to add port 443 to the endpoint -> request to http://xxx.local:443) ... and that works with all released versions of httplib2.
But this change here: 1641f7b#diff-72a22fe2efc9771bbe023ba1ffa07173
and it doesn't work anymore :-)

So my workaround right now is to use IP or a host remapping

But this PR here works very well, I can set HTTPLIB2_CA_CERTS and google/containerregistry pushes to our docker-registry :)

dirkboye · 2018-10-10T12:19:19Z

You can also close
#52
and
#114
after closing this PR.

dirkboye · 2018-10-15T04:17:17Z

@temoto when is pypi release from master planned?

dirkboye and others added 2 commits September 25, 2018 11:56

ca_certs from environment or certifi

8e7940d

Merge branch 'master' into feature/ca_certs_from_env

c4d3f5a

temoto requested review from stinky2nine, cdent, dims and sigmavirus24 September 25, 2018 18:23

temoto reviewed Sep 25, 2018

View reviewed changes

better unittests

d09aa07

dirkboye commented Sep 26, 2018

View reviewed changes

correct order certs return documentation

d9f81cc

add custom ca_certs_get to py3

03e7629

dirkboye commented Sep 26, 2018

View reviewed changes

small refactoring

53e26c7

stinky2nine reviewed Sep 26, 2018

View reviewed changes

python2/httplib2/certs.py Outdated Show resolved Hide resolved

stinky2nine suggested changes Sep 26, 2018

View reviewed changes

more readable way of checking certs availability

4f434ab

dirkboye and others added 2 commits September 27, 2018 06:28

Merge branch 'master' into feature/ca_certs_from_env

f558b27

change module description

f163f94

temoto reviewed Sep 27, 2018

View reviewed changes

use mock patch for available flags

9ef6a38

mocks for where methods and clean env var handling

567ad58

Merge branch 'master' into feature/ca_certs_from_env

a3598a4

temoto approved these changes Sep 29, 2018

View reviewed changes

stinky2nine added 5 commits September 29, 2018 16:32

Update certs.py

adcea87

Update certs.py

71a9465

Update certs.py

3411618

Update certs.py

ec0d4ae

Update certs.py

0014592

stinky2nine approved these changes Sep 29, 2018

View reviewed changes

temoto merged commit 8b65b52 into httplib2:master Oct 6, 2018

temoto added enhancement feature-security labels Oct 6, 2018

kapilt mentioned this pull request Oct 30, 2018

allow specifying cacerts #114

Closed

gertvdijk mentioned this pull request Jan 4, 2019

Fail to validate HTTPS server while using custom CA file #2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ca_certs from environment or certifi #117

ca_certs from environment or certifi #117

dirkboye commented Sep 25, 2018

codecov bot commented Sep 25, 2018 •

edited

Loading

temoto left a comment

temoto Sep 25, 2018

temoto Sep 25, 2018

dirkboye Sep 26, 2018

temoto commented Sep 26, 2018

dirkboye commented Sep 26, 2018

temoto commented Sep 26, 2018

temoto commented Sep 26, 2018

temoto commented Sep 26, 2018

temoto commented Sep 26, 2018

temoto commented Sep 26, 2018

dirkboye commented Sep 26, 2018

temoto commented Sep 26, 2018

dirkboye Sep 26, 2018

stinky2nine Sep 26, 2018

temoto Sep 26, 2018

dirkboye commented Sep 27, 2018

temoto Sep 27, 2018

dirkboye Sep 27, 2018

temoto Sep 27, 2018

temoto Sep 27, 2018

cdent commented Sep 27, 2018

stinky2nine commented Sep 27, 2018

temoto commented Sep 27, 2018

stinky2nine commented Sep 27, 2018

dirkboye commented Sep 28, 2018

temoto commented Sep 29, 2018

temoto commented Oct 6, 2018

dirkboye commented Oct 10, 2018

dirkboye commented Oct 10, 2018

dirkboye commented Oct 15, 2018



		def test_certs_file_from_builtin():
		httplib2.certs.certifi_available = False

ca_certs from environment or certifi #117

ca_certs from environment or certifi #117

Conversation

dirkboye commented Sep 25, 2018

codecov bot commented Sep 25, 2018 • edited Loading

Codecov Report

temoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

temoto commented Sep 26, 2018

dirkboye commented Sep 26, 2018

temoto commented Sep 26, 2018

temoto commented Sep 26, 2018

temoto commented Sep 26, 2018

temoto commented Sep 26, 2018

temoto commented Sep 26, 2018

dirkboye commented Sep 26, 2018

temoto commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirkboye commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdent commented Sep 27, 2018

stinky2nine commented Sep 27, 2018

temoto commented Sep 27, 2018

stinky2nine commented Sep 27, 2018

dirkboye commented Sep 28, 2018

temoto commented Sep 29, 2018

temoto commented Oct 6, 2018

dirkboye commented Oct 10, 2018

dirkboye commented Oct 10, 2018

dirkboye commented Oct 15, 2018

codecov bot commented Sep 25, 2018 •

edited

Loading