Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for client x509 authentication in Twisted #35

Merged
merged 2 commits into from Aug 23, 2018

Conversation

jeremycline
Copy link
Member

Create the connection with a client certificate, if configured to do so.

@abompard As an aside, what do you think about taking something like this and getting it into upstream pika? It seems like if the SSL context has client certs set, we should automatically be a) setting auth to EXTERNAL, and b) producing the correct twisted connection parameters from the pika parameters.

@codecov-io
Copy link

codecov-io commented Aug 21, 2018

Codecov Report

Merging #35 into master will increase coverage by 0.19%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
+ Coverage   88.93%   89.13%   +0.19%     
==========================================
  Files          11       11              
  Lines         768      782      +14     
  Branches      105      106       +1     
==========================================
+ Hits          683      697      +14     
  Misses         58       58              
  Partials       27       27
Impacted Files Coverage Δ
fedora_messaging/twisted/service.py 93.33% <100%> (+2.02%) ⬆️

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 e8f611f...e6e258d. Read the comment docs.

@abompard
Copy link
Member

It does make sense to have this upstream indeed. Would you rather I submit it?

@jeremycline
Copy link
Member Author

Yeah, if you've got time this week to do it, that'd be great. Otherwise I can get to in Soon™

Create the connection with a client certificate, if configured to do so.

fixes fedora-infra#14

Signed-off-by: Jeremy Cline <jcline@redhat.com>
parameters.host,
trustRoot=ca_cert,
clientCertificate=client_cert,
extraCertificateOptions={"raiseMinimumTo": ssl.TLSVersion.TLSv1_2},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 🎉

@abompard
Copy link
Member

To make it generic I would need to get the certificates from pika's parameter.ssl_options value (we're currently getting them from our config). It's easy on older versions of pika because it's just a dict, but when it's an ssl.SSLContext I can't find a way to extract the certificates and feed them to Twisted's ssl.optionsForClientTLS() function.

Am I missing something? I looks like it's intentional that I don't get access to the loaded certificates. Are you thinking of another way to make this code generic enough to get it into pika?

@jeremycline
Copy link
Member Author

jeremycline commented Aug 23, 2018

Am I missing something? I looks like it's intentional that I don't get access to the loaded certificates. Are you thinking of another way to make this code generic enough to get it into pika?

You're not missing something, I made it no further than thinking "it'd be nice if this Just Worked" and a cursory investigation of the pyOpenSSL object before I bailed and loaded things from the config. Given that pyOpenSSL is supposed to be replaced with cryptography (I don't know if Twisted has started on this or not), I don't think it's worth seeing if it's even possible to extract enough information from the object to construct the Twisted object. It'd be nice to revisit at some point, but this isn't a huge amount of boilerplate.

Maybe a nice middle ground for the present is to document what is necessary in pika. I'll see about making a PR for that.

@abompard
Copy link
Member

Yeah it would be nice to have it documented there. They have a usage example in the examples/twisted_service.py file but it's very basic.
In the meantime, this is good to go.

@mergify mergify bot merged commit f2e44b4 into fedora-infra:master Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants