Added support ssl cert file environment by cansarigol · Pull Request #307 · encode/httpx

cansarigol · 2019-09-02T19:54:15Z

Closes #306

sethmlarson · 2019-09-02T20:42:08Z

@StephenBrown2 You took the review comments right out of my mouth! 🎉 Thanks @cansarigol for tackling this issue. :)

sethmlarson · 2019-09-02T20:43:05Z

Also this would benefit from #308 landing. :)

sethmlarson · 2019-09-02T21:20:03Z

Btw #308 landed so I'd rebase your branch on master so that testing this is easier. :)

cansarigol · 2019-09-02T21:42:19Z

@StephenBrown2 @sethmlarson thanks for review.

lovelydinosaur

Great stuff - nice bit of progress towards this.
I've got some thoughts and questions above that probably need addressing.

We should also review if we really want all of SSL_CERT_FILE, CURL_CA_BUNDLE, and REQUESTS_CA_BUNDLE, since that seems like quite a large exposure area to various environment variables, which I think we should prefer to avoid if possible.

Perhaps(?) we should only start with SSL_CERT_FILE support?

cansarigol · 2019-09-03T13:57:10Z

Thanks @tomchristie , I applied your comments. I got the order from this pr Bwt it wasn't merged

sethmlarson · 2019-09-03T18:01:33Z

Shouldn't we be supporting (but I agree not recommending) REQUESTS_CA_BUNDLE for the sake of requests compatibility? I'm fine with dropping curls.

cansarigol · 2019-09-03T18:31:25Z

Hi, I dropped REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE envs support. is it ok?

lovelydinosaur · 2019-09-04T09:19:05Z

Shouldn't we be supporting (but I agree not recommending) REQUESTS_CA_BUNDLE for the sake of requests compatibility? I'm fine with dropping curls.

I don't know. I think we should possibly make that call seperately, once we're closing in on 1.0, when we're assessing and drawing up our requests compat. guide.

My rough feel at this point, is that this is probably one of the points where we ought to allow ourselves to diverge, but it depends on a more general question of just how closely we want to align.

In summary: I'd probably rather we make that specific call within the context of a more general review of our requests compat & divergences, rather than as part of this "add the functionality" PR.

Seem reasonable?

sethmlarson

Some more review comments

cansarigol · 2019-09-05T21:16:10Z

Hi, @sethmlarson thanks for your review.

sethmlarson · 2019-09-05T21:19:14Z

@cansarigol by the way we squash all commits when we merge so you don't have to amend and force push, you can just add a commits. Easier to review so we can see the incremental changes you're making.

cansarigol · 2019-09-05T21:24:45Z

Ah sorry, you're right. this is my habit. I will pay attention.

sethmlarson · 2019-09-06T12:41:32Z

+ context = ssl_config.load_ssl_context()
+ assert context.verify_mode == ssl.VerifyMode.CERT_REQUIRED
+ assert context.check_hostname is True
+ assert ssl_config.verify == os.environ[config]


Verify that certs are loaded via assert len(context.get_ca_certs) > 0

get_ca_certs returns empty if use context.load_verify_locations(capath=str(ca_bundle_path)).
To get a result, we should create an SSLSocket and call getpeercert like below
if config == "SSL_CERT_DIR": HOST = "example.org" PORT = 443 conn: ssl.SSLSocket = ssl_config.ssl_context.wrap_socket( socket.socket(socket.AF_INET, socket.SOCK_STREAM), server_hostname=HOST ) conn.connect((HOST, PORT)) conn.getpeercert()

Maybe we can use https_server.url.host and https_server.url.port to accomplish that. Then our certificate will verify as well :)

I couldn't verify with cert_pem_file for https_server.url. Please help :)

Has this been resolved? :)

I'm very sorry that I didn't do anything but i will asap

This is blocked by #354, just gave it a spin myself. I think we should maybe merge this now?

#354 is closed now, I tried changing the test to look like this right now:
os.environ[config] = ( ca_cert_pem_fileifconfig.endswith("_FILE") elsestr(Path(ca_cert_pem_file).parent) ) ssl_config=httpx.SSLConfig(trust_env=True) context=ssl_config.load_ssl_context() assertcontext.verify_mode==ssl.VerifyMode.CERT_REQUIREDassertcontext.check_hostnameisTrueassertssl_config.verify==os.environ[config] host=https_server.url.hostport=https_server.url.portconn=socket.create_connection((host, port)) context.wrap_socket(conn, server_hostname=host) assertlen(context.get_ca_certs()) ==1
But I'm still failing on the SSL_CERT_DIR test case and I don't know why, pretty annoying!

I added a test to be able to compare with below traceback.
tests/test_config.py ...FSSL error in data received protocol: <asyncio.sslproto.SSLProtocol object at 0x103c044a8> transport: <_SelectorSocketTransport closing fd=19 read=idle write=<idle, bufsize=0>> Traceback (most recent call last): File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/asyncio/sslproto.py", line 526, in data_received ssldata, appdata = self._sslpipe.feed_ssldata(data) File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/asyncio/sslproto.py", line 189, in feed_ssldata self._sslobj.do_handshake() File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/ssl.py", line 763, in do_handshake self._sslobj.do_handshake() ssl.SSLError: [SSL: TLSV1_ALERT_UNKNOWN_CA] tlsv1 alert unknown ca (_ssl.c:1045)

Spoke with @florimondmanca and I think we're just going to skip the SSL_CERT_DIR case here and if we run into issues with it we'll fix them then. Until then this PR is good to go. Thank you so much @cansarigol for following this one all the way through. :)

* from load_ssl_context * to load_ssl_context_verify

sethmlarson · 2019-09-06T18:19:23Z

After we get this merged I think we're ready to tag 0.7.3! @yeraydiazdiaz or @florimondmanca should do the release PR so we all get practice :)

sethmlarson

I went through and verified all the outstanding comments and made one small change to the logic in load_ssl_context_verify because verify isn't None by that point.

Would like one more set of eyes to look and merge this PR. Thanks @cansarigol!

florimondmanca

This is looking good! :-) I think I caught a possible bug — if confirmed we definitely need to address it before merging.

* seprated SSL_CERT_FILE and SSL_CERT_DIR * removed SSLConfig into trust_env part.

…o issue-33-ca

cansarigol · 2019-09-12T08:22:59Z

Hi @florimondmanca, thanks for your review and help.

florimondmanca

Looking good, provided the last open discussion is resolved. Great work @cansarigol!

sethmlarson · 2019-09-23T15:23:46Z

+ context = ssl_config.load_ssl_context()
+ assert context.verify_mode == ssl.VerifyMode.CERT_REQUIRED
+ assert context.check_hostname is True
+ assert ssl_config.verify == os.environ[config]


Spoke with @florimondmanca and I think we're just going to skip the SSL_CERT_DIR case here and if we run into issues with it we'll fix them then. Until then this PR is good to go. Thank you so much @cansarigol for following this one all the way through. :)

rehevkor5 · 2025-07-09T20:37:41Z

FYI, this was removed in #3022

StephenBrown2 reviewed Sep 2, 2019
View reviewed changes

Comment threadhttpx/utils.py Outdated

StephenBrown2 reviewed Sep 2, 2019
View reviewed changes

Comment threadtests/test_utils.py

StephenBrown2 reviewed Sep 2, 2019
View reviewed changes

Comment threaddocs/environment_variables.md Outdated

cansarigol force-pushed the issue-33-ca branch from 7a72b0e to 9a33deeCompare September 2, 2019 21:38

lovelydinosaur requested changes Sep 3, 2019
View reviewed changes

Comment threaddocs/environment_variables.md Outdated
Comment threaddocs/environment_variables.md Outdated
Comment threaddocs/environment_variables.md Outdated
Comment threadhttpx/config.py Outdated

cansarigol force-pushed the issue-33-ca branch from 9a33dee to 35c5153Compare September 3, 2019 13:49

cansarigol force-pushed the issue-33-ca branch from 35c5153 to 62cb6a6Compare September 3, 2019 18:26

StephenBrown2 reviewed Sep 3, 2019
View reviewed changes

Comment threadhttpx/utils.py Outdated

cansarigol force-pushed the issue-33-ca branch 2 times, most recently from 7008bf4 to 34375adCompare September 3, 2019 18:59

sethmlarson suggested changes Sep 5, 2019
View reviewed changes

Comment threadhttpx/utils.py Outdated
Comment threadtests/test_config.py Outdated
Comment threaddocs/environment_variables.md
Comment threaddocs/environment_variables.md Outdated
Comment threaddocs/environment_variables.md Outdated
Comment threaddocs/environment_variables.md Outdated

Added SSL_CERT_FILE and SSL_CERT_DIR environment support
54ee42d

cansarigol force-pushed the issue-33-ca branch from 74c3cea to 54ee42dCompare September 5, 2019 21:14

canand others added 4 commits September 6, 2019 09:34

use cert_pem_file fixture to validate ssl_cert env
73cbc9d

Merge branch 'master' into issue-33-ca
03e45c5

merge and fix test assert
3907406

Update the copy in documentation
bb46114

sethmlarson suggested changes Sep 6, 2019
View reviewed changes

moved ssl_cert_file
e9b5f34
* from load_ssl_context * to load_ssl_context_verify

lovelydinosaur reviewed Sep 9, 2019
View reviewed changes

Comment threaddocs/environment_variables.md Outdated

canand others added 2 commits September 9, 2019 22:48

fix for SSL_CERT docs
bb2dad4

Update config.py
4cac5d4

sethmlarson approved these changes Sep 11, 2019
View reviewed changes

florimondmanca self-requested a review September 11, 2019 20:51

florimondmanca suggested changes Sep 11, 2019
View reviewed changes

Comment threaddocs/environment_variables.md Outdated
Comment threaddocs/environment_variables.md
Comment threadhttpx/utils.py Outdated
Comment threadtests/test_utils.py

can added 4 commits September 12, 2019 10:57

changed docs
00fd839
* seprated SSL_CERT_FILE and SSL_CERT_DIR * removed SSLConfig into trust_env part.

fixed reading SSL_CERT method
0ca5798

added new tests for get_ssl_cert
b8fce82

Merge branch 'issue-33-ca' ofhttps://github.com/cansarigol/httpxint…
25b16a9
…o issue-33-ca

cansarigol requested a review from florimondmanca September 12, 2019 08:23

florimondmanca approved these changes Sep 12, 2019
View reviewed changes

canand others added 4 commits September 19, 2019 10:20

merge commit
e36f79e

merge commit 2
59817c7

added get_ca_certs
ce62cd0

Update test_config.py
7aa1625

florimondmanca added the help wanted Extra attention is needed label Sep 21, 2019

sethmlarson added 2 commits September 21, 2019 11:17

Merge branch 'master' into issue-33-ca
f3505f4

Update test_config.py
a9e8c5a

sethmlarson approved these changes Sep 23, 2019
View reviewed changes

sethmlarson merged commit c9810a7 into encode:masterSep 23, 2019

cansarigol deleted the issue-33-ca branch September 23, 2019 17:12

This was referenced May 11, 2026
Add support for SSL_CERT_FILE, REQUESTS_CA_BUNDLE, and CURL_CA_BUNDLE pydantic/httpx2#106
Closed
Docs are incorrect about SSL_CERT_FILE behavior pydantic/httpx2#903
Closed

Uh oh!

Uh oh!

Conversation

cansarigol commented Sep 2, 2019• edited by sethmlarson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sethmlarson commented Sep 2, 2019

Uh oh!

sethmlarson commented Sep 2, 2019

Uh oh!

sethmlarson commented Sep 2, 2019

Uh oh!

cansarigol commented Sep 2, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lovelydinosaur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cansarigol commented Sep 3, 2019

Uh oh!

sethmlarson commented Sep 3, 2019

Uh oh!

Uh oh!

cansarigol commented Sep 3, 2019

Uh oh!

lovelydinosaur commented Sep 4, 2019

Uh oh!

sethmlarson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cansarigol commented Sep 5, 2019

Uh oh!

sethmlarson commented Sep 5, 2019

Uh oh!

cansarigol commented Sep 5, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sethmlarson commented Sep 6, 2019

Uh oh!

Uh oh!

sethmlarson left a comment

Choose a reason for hiding this comment

Uh oh!

florimondmanca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cansarigol commented Sep 2, 2019•
edited by sethmlarson
Loading

cansarigol commented Sep 2, 2019•
edited
Loading