> 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 > >
