qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kenneth Giusti" <kgiu...@apache.org>
Subject Re: Review Request: SSL/TLS support for Proton connections.
Date Wed, 19 Sep 2012 18:38:03 GMT


> On Sept. 19, 2012, 4:43 p.m., Rafael Schloming wrote:
> > /proton/trunk/proton-c/include/proton/ssl.h, line 1
> > <https://reviews.apache.org/r/7171/diff/1/?file=158476#file158476line1>
> >
> >     I notice this file doesn't expose the ssl construtors directly. I think we actually
should so that people can build their own drivers and easily support ssl. I also think it
will be useful in testing since we can directly drive an ssl configured transport from the
tests and create all sorts of different permutations of interaction deterministically rather
than waiting for them to come up by chance via a real driver.

Sounds like a good idea - I'll put the decl in the public ssl.h file.  I'll also add a new
public method to access the transport from the connector, as per your other comment (e.g.
 pn_transport_t *pn_connector_transport(pn_connector_t *))


> On Sept. 19, 2012, 4:43 p.m., Rafael Schloming wrote:
> > /proton/trunk/proton-c/src/ssl/openssl.c, line 385
> > <https://reviews.apache.org/r/7171/diff/1/?file=158481#file158481line385>
> >
> >     For sasl I actually just return the existing one here. My thinking was that
it would be convenient to be able to access the existing sasl layer given a transport, however
I didn't want to have the transport's public API depend on sasl at all. However I can see
how you might want the option to not automatically create the given security layer, so maybe
the best of both worlds would be to have a separate accessor on the sasl side or a flag on
the constructor that controls whether the layer is created automatically. Whatever we do we
should probably be consistent between the two security layers (ssl & sasl).

I'm a bit confused - my intent was to create a new ssl object if one did not exist, else just
return the existing one (on-demand create).  The error you highlight is a bit of an awkward
check to catch the case where the ssl object's mode is treated inconsistently by the caller
(e.g. created as a _client_, then queried again as a _server_, or vice versa). 

So, the way the code is currently designed, an ssl object of a given type (client/server)
is created on first access via the transport.  And, once created, you cannot change the type
of the ssl object.

Alternatively, we can create separate accessor and initialization methods, like:   
   pn_ssl_t *pn_transport_ssl( pn_transport_t * );  // return existing pn_ssl_t, or create
a new one if none present
   int pn_ssl_init( pn_ssl_t *, enum {PN_SSL_MODE_CLIENT, PN_SSL_MODE_SERVER} ).  // Set the
role of this end of the SSL connection


What do you think? 


> On Sept. 19, 2012, 4:43 p.m., Rafael Schloming wrote:
> > /proton/trunk/proton-c/src/ssl/openssl.c, line 425
> > <https://reviews.apache.org/r/7171/diff/1/?file=158481#file158481line425>
> >
> >     General question, have you run proton-tests under valgrind? If not, probably
a good idea just to check all this silly free stuff.

Heh, yes - for kicks.  It basically stopped recording errors after the first 5000 - all of
which came from the openssl libraries :)

But seriously - I will re-run it again with the proper filters to ignore those openssl library
warnings....


- Kenneth


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7171/#review11696
-----------------------------------------------------------


On Sept. 19, 2012, 4:11 p.m., Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7171/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2012, 4:11 p.m.)
> 
> 
> Review request for qpid and Rafael Schloming.
> 
> 
> Description
> -------
> 
> Patch to add SSL/TLS support to proton using OpenSSL.
> 
> 
> This addresses bug PROTON-2.
>     https://issues.apache.org/jira/browse/PROTON-2
> 
> 
> Diffs
> -----
> 
>   /proton/trunk/examples/mailbox/README.txt 1387654 
>   /proton/trunk/examples/mailbox/fetch 1387654 
>   /proton/trunk/examples/mailbox/post 1387654 
>   /proton/trunk/examples/mailbox/server 1387654 
>   /proton/trunk/examples/mailbox/ssl-setup.sh PRE-CREATION 
>   /proton/trunk/proton-c/CMakeLists.txt 1387654 
>   /proton/trunk/proton-c/bindings/php/php.i 1387654 
>   /proton/trunk/proton-c/bindings/python/python.i 1387654 
>   /proton/trunk/proton-c/bindings/ruby/ruby.i 1387654 
>   /proton/trunk/proton-c/include/proton/cproton.i 1387654 
>   /proton/trunk/proton-c/include/proton/driver.h 1387654 
>   /proton/trunk/proton-c/include/proton/ssl.h PRE-CREATION 
>   /proton/trunk/proton-c/pn_config.h.in 1387654 
>   /proton/trunk/proton-c/src/driver.c 1387654 
>   /proton/trunk/proton-c/src/engine/engine-internal.h 1387654 
>   /proton/trunk/proton-c/src/engine/engine.c 1387654 
>   /proton/trunk/proton-c/src/ssl/openssl.c PRE-CREATION 
>   /proton/trunk/proton-c/src/ssl/ssl-internal.h PRE-CREATION 
>   /proton/trunk/proton-c/src/ssl/ssl_stub.c PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/7171/diff/
> 
> 
> Testing
> -------
> 
> "proton-tests" still works :)
> 
> Updated "mailbox" example to support SSL - see mailbox/README.txt for instructions on
how to set up and run with SSL.
> 
> 
> Thanks,
> 
> Kenneth Giusti
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message