fix: always load the default certs on our custom SSL context #196

pyrooka · 2024-07-02T10:15:14Z

This is a follow-up PR of this one. In the linked PR we had to specify and older version of the requests package to avoid a regression.

This PR contains the fix for that, so we can continue using always the latest version. Furthermore, the HTTPS related tests have been improved and moved to the directory of the unit tests, to always include them in our builds.

Signed-off-by: Norbert Biczo <[email protected]>

…t to the `test` folder At this point if `requests 2.32.3` is installed, the issue is reproducable. Signed-off-by: Norbert Biczo <[email protected]>

Signed-off-by: Norbert Biczo <[email protected]>

ibm_cloud_sdk_core/http_adapter.py

test/test_http_adapter.py

pyrooka · 2024-07-02T10:28:14Z

test/test_http_adapter.py

+        service = BaseService(service_url='https://raw.githubusercontent.com', authenticator=NoAuthAuthenticator())
+        assert service.disable_ssl_verification is False
+        prepped = service.prepare_request('GET', url='/IBM/python-sdk-core/main/README.md')


I'm not too happy with this solution, but I found this to be the most reliable way to reproduce the problem. Actually the only way because I was not able to trigger the exception in any of my local experiments. I totally understand if you have concerns about this, so let's discuss.

pyrooka · 2024-07-02T10:29:02Z

test/test_http_adapter.py

-        with pytest.raises(SSLError):
+        with pytest.raises(SSLError, match="certificate verify failed: self-signed certificate"):


Just a small tweak to make sure we catch the exact same error that we want.

Signed-off-by: Norbert Biczo <[email protected]>

ricellis

I think this is a good change to workaround the requests issues. I supect the comment wasn't directed at me, but I had one thought around the external wesbite test, but I wouldn't consider that a blocker to merging this anyway.

ricellis · 2024-07-03T15:02:04Z

test/test_http_adapter.py

+    service = BaseService(service_url='https://raw.githubusercontent.com', authenticator=NoAuthAuthenticator())
+    assert service.disable_ssl_verification is False
+    prepped = service.prepare_request('GET', url='/IBM/python-sdk-core/main/README.md')


I'm not too happy with this solution, but I found this to be the most reliable way to reproduce the problem. Actually the only way because I was not able to trigger the exception in any of my local experiments. I totally understand if you have concerns about this, so let's discuss.

The only other thing I can think of is listing the certs available and checking it is more than 0, but I don't know if we have enough visibility to be able to do that.

Certainly using a website that has a public certificate chain leading to some default root cert is a surefire way to prove that it can successfully negotiate a connection using the loaded default certs. However, whilst connecting to github proves we have some default certs it doesn't necessarily prove we have the certificate we need so perhaps switching to some cloud.ibm.com site would offer more value since it will likely use the same root certifcate chain as our services do? Maybe something like https://cloud.ibm.com/status or with a filter like https://cloud.ibm.com/status/security?component=ibm-cloud&location=global if it's too large a page otherwise?

The only other thing I can think of is listing the certs available and checking it is more than 0, but I don't know if we have enough visibility to be able to do that.

That's a clever idea but I can't tell either - at least off the to of my head. I think I'll take a look because this sounds interesting.

...perhaps switching to some cloud.ibm.com site would offer more value...

Agreed, it makes sense!

Signed-off-by: Norbert Biczo <[email protected]>

dpopp07

All looks good to me! 👍

padamstx

LGTM, but there are test failures in the Travis build :(

Signed-off-by: Norbert Biczo <[email protected]>

test/test_http_adapter.py

padamstx

LGTM

padamstx · 2024-07-09T14:43:26Z

test/test_http_adapter.py

+    ssl_context = service.http_adapter.poolmanager.connection_pool_kw.get("ssl_context")
+    assert ssl_context is not None
+    # In some cases (especially in Ubuntu containers that we use for testing on Travis)
+    # the default CA certifications are stored in a different place, so let's try to


A really minor nit...

Suggested change

# the default CA certifications are stored in a different place, so let's try to

# the default CA certificates are stored in a different place, so let's try to

Signed-off-by: Norbert Biczo <[email protected]>

## [3.20.2](v3.20.1...v3.20.2) (2024-07-09) ### Bug Fixes * always load the default certs on our custom SSL context ([#196](#196)) ([ff14a4b](ff14a4b))

ibm-devx-sdk · 2024-07-09T19:18:57Z

🎉 This PR is included in version 3.20.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

pyrooka added 7 commits July 2, 2024 11:24

test: update openssl command, re-gen local cert and key

73d03d5

Signed-off-by: Norbert Biczo <[email protected]>

test: add local cert verification

f65e965

Signed-off-by: Norbert Biczo <[email protected]>

test: extend SSL test to reproduce the main issue

ebf7f7b

Signed-off-by: Norbert Biczo <[email protected]>

chore: start using the latest version of requests again

91c241b

Signed-off-by: Norbert Biczo <[email protected]>

chore: move the SSLHTTPAdapter class to a separate file and its tes…

9530e6c

…t to the `test` folder At this point if `requests 2.32.3` is installed, the issue is reproducable. Signed-off-by: Norbert Biczo <[email protected]>

fix: always load the default certs on our custom SSL context

6a110fe

Signed-off-by: Norbert Biczo <[email protected]>

test: make the SSLError check stricter

d772f78

Signed-off-by: Norbert Biczo <[email protected]>

pyrooka commented Jul 2, 2024

View reviewed changes

pyrooka force-pushed the nb/https-test-improvements branch from 99404de to e783e05 Compare July 2, 2024 12:44

test: validate that TLS v1.2 in the minimum required version

d975630

Signed-off-by: Norbert Biczo <[email protected]>

pyrooka force-pushed the nb/https-test-improvements branch from e783e05 to d975630 Compare July 2, 2024 14:31

pyrooka marked this pull request as ready for review July 2, 2024 14:39

pyrooka self-assigned this Jul 2, 2024

pyrooka requested review from padamstx, dpopp07 and ricellis July 2, 2024 15:07

ricellis approved these changes Jul 3, 2024

View reviewed changes

pyrooka added 2 commits July 8, 2024 15:47

test: use more realistic url for testing SSL with external site

c9735f9

Signed-off-by: Norbert Biczo <[email protected]>

test: check the number of the loaded SSL certs

2aaadab

Signed-off-by: Norbert Biczo <[email protected]>

dpopp07 approved these changes Jul 8, 2024

View reviewed changes

padamstx approved these changes Jul 8, 2024

View reviewed changes

test: fix cert loading in builds

296760a

Signed-off-by: Norbert Biczo <[email protected]>

pyrooka commented Jul 9, 2024

View reviewed changes

test/test_http_adapter.py Outdated Show resolved Hide resolved

pyrooka requested a review from padamstx July 9, 2024 10:10

padamstx approved these changes Jul 9, 2024

View reviewed changes

chore: fix typo

c26344f

Signed-off-by: Norbert Biczo <[email protected]>

pyrooka merged commit ff14a4b into main Jul 9, 2024
4 checks passed

pyrooka deleted the nb/https-test-improvements branch July 9, 2024 19:15

ibm-devx-sdk pushed a commit that referenced this pull request Jul 9, 2024

chore(release): 3.20.2 release notes

8e8c3f5

## [3.20.2](v3.20.1...v3.20.2) (2024-07-09) ### Bug Fixes * always load the default certs on our custom SSL context ([#196](#196)) ([ff14a4b](ff14a4b))

ibm-devx-sdk added the released label Jul 9, 2024

pyrooka mentioned this pull request Jul 11, 2024

fix: improve the detection and loading of default certificates #197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: always load the default certs on our custom SSL context #196

fix: always load the default certs on our custom SSL context #196

pyrooka commented Jul 2, 2024

pyrooka Jul 2, 2024

pyrooka Jul 2, 2024

ricellis left a comment

ricellis Jul 3, 2024

pyrooka Jul 5, 2024

dpopp07 left a comment

padamstx left a comment

padamstx left a comment

padamstx Jul 9, 2024

ibm-devx-sdk commented Jul 9, 2024

		with pytest.raises(SSLError):
		with pytest.raises(SSLError, match="certificate verify failed: self-signed certificate"):

	# the default CA certifications are stored in a different place, so let's try to
	# the default CA certificates are stored in a different place, so let's try to

fix: always load the default certs on our custom SSL context #196

fix: always load the default certs on our custom SSL context #196

Conversation

pyrooka commented Jul 2, 2024

pyrooka Jul 2, 2024

Choose a reason for hiding this comment

pyrooka Jul 2, 2024

Choose a reason for hiding this comment

ricellis left a comment

Choose a reason for hiding this comment

ricellis Jul 3, 2024

Choose a reason for hiding this comment

pyrooka Jul 5, 2024

Choose a reason for hiding this comment

dpopp07 left a comment

Choose a reason for hiding this comment

padamstx left a comment

Choose a reason for hiding this comment

padamstx left a comment

Choose a reason for hiding this comment

padamstx Jul 9, 2024

Choose a reason for hiding this comment

ibm-devx-sdk commented Jul 9, 2024