Platform specific TLS/SSL configuration context #53
Conversation
| FriendlyName, | ||
| ) | ||
|
|
||
| os.chdir(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) |
There was a problem hiding this comment.
Pycharm likes to set the working directory to tests, this sets it to the project root so it can find .ci/certs.
| if err < 0: | ||
| exc = EXCEPTIONS.get(err, SSLException) | ||
| raise exc("SSL failure.") | ||
| raise exc("SSL failure.", err) |
There was a problem hiding this comment.
This was worth its weight in gold during debugging.
https://github.com/apache/qpid-proton/blob/main/c/src/ssl/schannel.cpp#L1967 resulting in an qpid protonTimeout instance is misleading. But at least the string mentions SSL.
| self._ssl_domain = SSLDomain(SSLDomain.MODE_CLIENT) | ||
| if self._ssl_domain is not None: | ||
| self._ssl_domain.set_trusted_ca_db(self._conf_ssl_context.ca_cert) | ||
| assert self._ssl_domain |
There was a problem hiding this comment.
I don't know why mypy needs this assert in order to assume it can't be None given that it is initialized to an instance of SSLDomain above, or never reaching this line due to the constructor raising an exception. But is does :(
| def ssl_context(pytestconfig): | ||
| if sys.platform == "win32": | ||
| return WinSslConfigurationContext( | ||
| ca_store=PKCS12Store(path=".ci/certs/ca.p12"), |
There was a problem hiding this comment.
I did test CurrentUserStore("Personal") locally
|
HI @Darsstar thanks a lot for your contribution! It is really appreciated, I'll have a look in the next days! |
|
Hi @Darsstar I tested both on Linux and Windows and it looks good. Also the code looks good! I'd suggest to create an example for windows too before merging! Thanks! |
Done |
DanielePalaia
left a comment
DanielePalaia
left a comment
There was a problem hiding this comment.
Looks good to me! Many thanks for the nice work.
@Gsantomaggio if for you is fine too I'm going to merge it
|
@Darsstar thank you, Can you please fix: https://github.com/rabbitmq/rabbitmq-amqp-python-client/actions/runs/13587051950/job/37994219395 |
Darsstar
force-pushed
the
platform-specific-SslConfigurationContext
branch
2 times, most recently
from
58c1699 to
92bfa64
Compare
|
oops, fixed. |
Darsstar
force-pushed
the
platform-specific-SslConfigurationContext
branch
from
92bfa64 to
0c7f845
Compare
3 participants
The test were failing on Windows due to TLS/SSL not working.
Figuring out why, and how to make it work wasn't that easy, so I figured platform specific SslConfigurationContext are probably worth the effort of implementing.
I chose
Posixas the non-windows prefix based on https://github.com/apache/qpid-proton/blob/main/c/src/ssl/PLATFORM_NOTES.md, I will hapily change it if desired.I had to rip the Certificate Revocation List extension out of the server certificate since Windows does try to access
http://crl-server:????/<snip>in order to verify the validity of the server certificate. (I don't remember the exact url.) I ripped it out the the client certificate before realizing the server certificate was being validated, not the client certificate. I didn't undo the client certificate changes.Anyway, now that I can run the full test suite I'll look into the async API. Thanks for reading, have a good day!
PS. I checked the
Allow edits and access to secrets by maintainerscheckbox, feel free to push to my branch to your heart's content.