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

Verify TLS certificate during XMPP connection. #1084

Conversation

@goffi-contrib
Copy link

commented Nov 6, 2018

A new "check_certificate" attribute has been introduced in t.w.p.j.xmlstream.TLSInitiatingInitializer.
A new "check_certificate" init argument has been introduced in
t.w.p.j.client.BasicAuthenticator and t.w.p.j.client.XMPPAuthenticator.

Remove this paragraph

Please have a look at our developer documentation before submitting your Pull Request.

https://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch

Contributor Checklist:

@goffi-contrib goffi-contrib force-pushed the goffi-contrib:9561-goffi-contrib-xmpp-tls-certificate branch from 76be5c4 to 0a30d0b Nov 7, 2018

@codecov

This comment has been minimized.

Copy link

commented Nov 7, 2018

Codecov Report

Merging #1084 into trunk will decrease coverage by 0.4%.
The diff coverage is 89.13%.

@@            Coverage Diff             @@
##            trunk    #1084      +/-   ##
==========================================
- Coverage   91.98%   91.57%   -0.41%     
==========================================
  Files         844      844              
  Lines      151099   151390     +291     
  Branches    13162    13252      +90     
==========================================
- Hits       138989   138640     -349     
- Misses      10007    10624     +617     
- Partials     2103     2126      +23

@goffi-contrib goffi-contrib force-pushed the goffi-contrib:9561-goffi-contrib-xmpp-tls-certificate branch from 0a30d0b to 4cecff0 Nov 7, 2018

Verify TLS certificate during XMPP connection.
A new "check_certificate" attribute has been introduced in t.w.p.j.xmlstream.TLSInitiatingInitializer.
A new "check_certificate" init argument has been introduced in
t.w.p.j.client.BasicAuthenticator and t.w.p.j.client.XMPPAuthenticator.

@goffi-contrib goffi-contrib force-pushed the goffi-contrib:9561-goffi-contrib-xmpp-tls-certificate branch from 4cecff0 to adbe5db Nov 7, 2018

@wiml

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

This seems like a good change, I certainly agree that the current default behavior is surprising in a bad way, and the default behavior should be to verify the server's cert against platformTrust.

However, if I don't want the default behavior, I probably don't want my only alternative to be not to verify anything at all. Most likely I want to trust some private CA or a pinned certificate. Maybe I want to supply a client cert or something.

What about letting the user supply a IOpenSSLContextFactory implementation instead of a boolean flag? If it's default (or None) then create a verifying CertificateOptions.

@goffi-contrib

This comment has been minimized.

Copy link
Author

commented Nov 17, 2018

Hello @wiml yes this make sense. I'll do an update later today or this week. Thanks for your feedback.

@goffi-contrib

This comment has been minimized.

Copy link
Author

commented May 5, 2019

hello, just a ping to show I haven't forgotten about this PR, I have been overwhelmed lastly. I'll update my PR hopefully soon.

@ralphm

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@goffi-contrib I'm looking at this here at the PyCon spints. Besides feelings of regret towards some design decisions I made when this code was created (including subclassing instead of composition), not verifying is a result of me misunderstanding how this is supposed to work. Initial observations below.

  1. I agree with @wiml that instead of a boolean, being able to provide certificate options to TLSInitiatingInitializer would be a better approach, as it allows more flexibility when you need to do something more advanced, like providing a client certificate. The name of the (new) argument should use camelCase.

  2. Instead of creating a CertificateOptions object directly, optionsForClientTLS should be used instead. It allows for setting the target hostname, so that can be cross-referenced with the certificate.

  3. I think BasicAuthenticator should maybe just be deprecated. Non-SASL authentication has been deprecated 13 years ago, and nobody should be doing this anymore. This ticket might not be the best place to do this, though.

  4. Instead of providing for a way to pass arbitrary arguments to the initializers and using setattr to make that work, I think it would be better to pass a lambda function that takes xs and creates a TLSInitiatingInitializer with the certificate options provided to the authenticator, or the result of the call to optionsForClientTLS by default.

@ralphm

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

Closed by merging #1147.

@ralphm ralphm closed this Jul 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.