Opened 18 years ago

Closed 16 years ago

Last modified 6 years ago

#302 enhancement closed fixed (fixed)

client certificate verification (move sslverify into twisted.internet)

Reported by: jknight Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: Jean-Paul Calderone, itamarst, jknight, Ralph Meijer, Glyph Branch:


Attachments (2)

ssl.patch (3.6 KB) - added by jknight 18 years ago.
ssl.2.patch (3.7 KB) - added by jknight 18 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 18 years ago by jknight

This adds options to make a DefaultOpenSSLContextFactory to have it 
verify client certs.  Also sets ssl options SSL_OP_ALL 
SSL_OP_SINGLE_DH_USE, the first because compatibility with broken 
clients is good, the second because temporary keys are good. (matches 
apache mod_ssl).

Changed 18 years ago by jknight

Attachment: ssl.patch added

comment:2 Changed 18 years ago by Moshe Zadka

I don't like this patch for several reasons

* The "HACK" is possibly OK, but only if in parallel there's a patch
  to PyOpenSSL to have it exposed at some point.
* verifyPeer seems kind of arbitrary. Could you do this with an enum
  so that people wouldn't have to litter their sources with 0s, 1s
  and 2s?
* Compare to None with "is", not "=="
* It's not clear what the session-id code is related to. Why are
  the two changes linked? What's wrong with the default? etc.
* What is a use-case for allowing arbitrary callbacks? This is the
  thing that scares me the most, because this kind of thing is
  frequently an API promise we have to support for a loooong time.
* No unit-tests.

comment:3 Changed 18 years ago by jknight

point 4: it is required for session reuse. (That is -- OpenSSL can do a shortened negotiation
which takes less time when you've already opened one session to that host.) This only works if you 
set the session id. This is desirable with or without client cert verification. However, for some 
unknown-to-me reason, it is *required* if you have client cert verification. 

point 5: I'm not using it. The code I started with that was posted to the twisted ml by someone else 
had a callback, and the callback is trivial to expose via the openssl API, so I didn't eliminate it. One 
possible use is to implement certificate revocation lists, but that probably ought to go into 
twisted-proper (at some point) in any case.

comment:4 Changed 18 years ago by Moshe Zadka

I understand the callback is *easy* to expose. It's also, probably,
easy to expose incorrectly, and be left with a crap API. Until
you have use case for two clear callbacks, at least, it's better
to skip it.

Re: session: it's better, then, to commit it as an unrelated
patch, and mention in this one that it needs that patch.
Because session-id doesn't introduce new API, it is easier
to commit it.

Also, you didn't comment on my other points.
Are you going to write an updated patch?

Changed 18 years ago by jknight

Attachment: ssl.2.patch added

comment:5 Changed 18 years ago by jknight

I haven't even looked at the test framework and won't have a chance to for a while. If that means
this patch must stay here until then, I understand. If someone else wants to have a go at it, that'd 
be much appreciated, though. :)

T'would be nice for someone who already has connections with Pyopenssl people/is on their 
mailing list/whatever to suggest that change, if such a person exists.

I'm not quite sure how the callback could be exposed incorrectly, but if it makes you feel better, 
it's removed. 

As for the other comments I believe they're fixed.

If someone wants to commit the non-api-changing parts before the api-changing parts, I'd 
suggest doing both the ctx.setOptions and the session id code.

comment:6 Changed 18 years ago by itamarst

I will look at this when I have some free time. The callback thing might or
might not work, I need to look at pyOpenSSL docs (and think about stuff like how
we do blocking verification and so on.)

comment:7 Changed 17 years ago by Jean-Paul Calderone

An entirely new context factory which Quotient will probably be using:

  It would be good to fold this back into Twisted at some point.  I think it
covers almost all of the current functionality, the functionality in foom's
patch, and adds a couple more things.

  No tests yet, but these will follow soon, and before it gets put in Twisted
obviously.  Any comments, criticisms?

comment:8 Changed 17 years ago by jknight

I just looked at the session id stuff again, and found the issue. 
pyOpenSSL's set_session_id is really SSL_CTX_set_session_id_context. At 
first I was confused, because the OpenSSL docs for setting session id 
note that it has a default using a random number generator, and that 
should be just fine for us. Session ID *Context*, on the other hand, 
must be set.

> If the session id context is not set on an SSL/TLS server and client 
> certificates are used, stored sessions will not be reused but a fatal 
> error will be flagged and the handshake will fail.
>  If a server returns a different session id context to an OpenSSL 
> client when reusing a session, an error will be flagged and the 
> handshake will fail. OpenSSL servers will always return the correct 
> session id context, as an OpenSSL server checks the session id context 
> itself before reusing a session as described above.

The bit about the fatal error is the problem. It doesn't *just* not 
reuse the stored session, but instead, fails with a fatal error. So, I 
think it's right to always set it. But the comment should probably be 
updated to note that not setting it is probably a very bad idea. Or 
else just remove the option. :)

Also, I think your comment for verifyOnce is wrong. It appears to 
really mean don't reverify when you renegotiate session parameters in a 
connection. Which we don't do anyways, but if we did, we'd probably 
want to be able to change the certificate. So, I don't think that's a 
good thing to set.

typo: "for cert ion self.clientCACertsList", also it doesn't appear to 
be adding to the CA list.

method=SSL.TLSv1_METHOD is the wrong default. The default should be 
SSLv23_METHOD, as that allows all protocol versions to be used.

Might consider the following code from apache if you want more control 
over protocol version. I'm not sure that's really necessary though.
>     if (sc->nProtocol == SSL_PROTOCOL_SSLV2)
>         ctx = SSL_CTX_new(SSLv2_server_method());  /* only SSLv2 is 
> left */
>     else
>         ctx = SSL_CTX_new(SSLv23_server_method()); /* be more flexible 
> */
>     if (!(sc->nProtocol & SSL_PROTOCOL_SSLV2))
>         SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv2);
>     if (!(sc->nProtocol & SSL_PROTOCOL_SSLV3))
>         SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv3);
>     if (!(sc->nProtocol & SSL_PROTOCOL_TLSV1))
>         SSL_CTX_set_options(ctx, SSL_OP_NO_TLSv1);

> Always using SSL_OP_SINGLE_DH_USE has an impact on the computer time 
> needed during negotiation, but it is not very large, so application 
> authors/users should consider to always enable this option.
That should be re-added.

>         self.ctx.set_verify(verifyFlags)
actually work? set_verify looks like it requries two arguments to me, 
which is why I used "lambda conn,cert,errno,depth,retcode: retcode" as 
the 'do-nothing' callback.

The new "OpenSSLClientCertificateOptions" class should not require that 
you set a certificate.

Finally, I'm not sure why you had to make a new class for this. All the 
real changes are additions to the interface, not changes, so I think 
the old class could just have the new functionality added, instead of 
deprecating it and adding a new class.

comment:9 Changed 17 years ago by Jean-Paul Calderone

First, thanks for the _awesome_ feedback :)

RE: Session ID contexts: right, that's my understanding as well.  Upon further
thinking, not is there probably no reason to not set it, but it might make sense
to let the application code select a value for it.  This is really advanced
usage, but I can see scenarios where it would definitely be desirable.  It's
only unfortunate that the session ID cache is hidden deep inside OpenSSL and
not, say, serializable to disk or over a socket ;)

RE: verifyOnce comment: on re-reading the man page, I think your interpretation
is correct.  For the set_verify callback, I think we can pass None which will
cause OpenSSL to continue to use the default verification behavior without
calling up to Python at all.

RE: Adding certs to the X509Store: What makes you think it isn't adding CAs to
the list?  If this is the case, it must be a PyOpenSSL bug?  I'll be sure to pay
attention to this when writing tests.

RE: method: Why is TLSv1 the wrong default?  v23 sucks and is vulnerable to all
of the SSLv2 security problems.  It's easily enough overriden by users who want
maximum compatibility without regard for real security.  This is open for
discussion, but I prefer the secure default with documentation explaining why it
may not work for everyone.  Looking at the Apache code, perhaps this is an
answer.  It looks like OpenSSL can be directed to use TLSv1 or SSLv3 for a
connection?  I think I'd be happy with that as a the default.

RE: SSL_OP_SINGLE_DH_USE: I'll re-add.

RE: Certificate requirement: Mmm, right.

RE: New class: Simplicity, mostly.  After gaining more familiarity with the API,
I'd now also like to make a base class that doesn't do anything with filenames.
 A subclass can add this functionality.  The current interface presented by the
DefaultOpenSSLContextFactory (a stupid name, another reason I didn't try to work
with it) might be preserved with such a subclass.

I'll post again when I've actually made some changes.  It might be a couple days
before I get back to this part of the code.  Thanks again for the comments.

comment:10 Changed 17 years ago by jknight

Yes, session ID context will need to be user-settable, if/when we support session serialization to 
disk. External session lookup/saving callbacks *ARE* part of the OpenSSL API, but I'm not sure that's 
exposed via pyOpenSSL. Until that's possible, there's not much point in exposing it as a parameter 

I submitted a pyOpenSSL patch to allow set_verify to accept None, but as I've got no feedback on 
patches sent 2 weeks ago yet, I don't know when it'll get put in.

Notice there are 2 lists: the list of CAs sent to the client and the list of CAs to verify against. Notice that 
the file case adds to both and the in memory case looks like it adds to only one (the verify-against list)

I'd be happy with v3 & TLSv1 as default. v3 is supported by all clients at this point.

comment:11 Changed 16 years ago by paniq

what's with this ticket? i need this functionality, is it provided now somewhere?

comment:12 Changed 16 years ago by Jean-Paul Calderone

There's a random copy of a random version of sslverify in twisted.pb. This is not the ultimate resolution for this ticket.

comment:13 Changed 16 years ago by Ralph Meijer

Cc: Ralph Meijer added

I would like to use sslverify for Jabber/XMPP connections. I suppose we need to agree on a better location for putting sslverify?

comment:14 Changed 16 years ago by itamarst

twisted.internet.sslverify seems the obvious place. If you move it, make sure you have latest version from Divmod, and that you also copy the test_sslverify module over.

comment:15 Changed 16 years ago by Glyph

Summary: DefaultOpenSSLContextFactory client certificate verification patchclient certificate verification (move sslverify into twisted.internet)

comment:16 Changed 16 years ago by Jean-Paul Calderone

Cc: Glyph added
Keywords: review added
Owner: Jean-Paul Calderone deleted
Priority: normalhighest

Review in source:branches/sslverify-302

comment:17 Changed 16 years ago by Stephen Thorne

Owner: set to Stephen Thorne

comment:18 Changed 16 years ago by Stephen Thorne

Owner: changed from Stephen Thorne to Jean-Paul Calderone

testNameToLabel doesn't include the simple cases of invalid input. spaces in the input result in spaces in the output. Unicode input results in unicode output. Is this desired behaviour? Please add a unit test asserting whatever behaviour is sanest.

There is a bug on line 232, _x509namecrap (which is a name that should be changed)'s last element is a list of length 1, not 2. Even if this is intended, it should be documented and tested.

Assign the ticket back to me for re-review after you've done the above.

comment:19 Changed 16 years ago by Stephen Thorne

Keywords: review removed

comment:20 Changed 16 years ago by Jean-Paul Calderone

Keywords: review added
Owner: changed from Jean-Paul Calderone to Stephen Thorne

Got rid of _x509namecrap and added some stuff to nameToLabel's docstring. Also added some other docstrings.

comment:21 Changed 16 years ago by Stephen Thorne

Keywords: review removed
Owner: changed from Stephen Thorne to Jean-Paul Calderone

Great, it's all looking a lot better now. Please merge. :)

comment:22 Changed 16 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [17414]) Merge sslverify-302

Author: exarkun Reviewer: Jerub Fixes #302

This relocates twisted.pb.sslverify to a private implementation module in twisted.internet and publishes the public parts of its interface through the twisted.internet.ssl module. sslverify provides support for somewhat more featureful ssl and tls certificate interactions, including setting various security requirements, managing certificate chains and certificate authority chains, creating certificates, certificate requests, keys, signing certificate requests, and translating ssl errors into something slightly less opaque.

comment:23 Changed 11 years ago by <automation>

Owner: Jean-Paul Calderone deleted

comment:24 Changed 6 years ago by GitHub <noreply@…>

In 4ef41e6f:

Error: Processor CommitTicketReference failed
 does not appear to be a Git repository. See the log for more information.
Note: See TracTickets for help on using tickets.