Qpid
  1. Qpid
  2. QPID-3514

Allow SSL and non-SSL connections on the same port

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.12
    • Fix Version/s: 0.13
    • Component/s: C++ Broker
    • Labels:
      None

      Description

      The Matahari Project (http:://matahariproject.org for the uninitiated) has run into an issue with our use of Qpid in that IANA policy is now to refuse to assign separate TCP ports for SSL/TLS-wrapped versions of protocols, which leaves us with only a single port assigned to Matahari.

      We would like to be able to accept both SSL and non-SSL connections on the same port.

      1. ssl-mux.patch
        27 kB
        Zane Bitter

        Issue Links

          Activity

          Hide
          Zane Bitter added a comment -

          The attached patch implements this when the SSL Plugin is enabled and both AMQP and AMQPS are configured to use the same port (i.e. port == ssl-port).

          Show
          Zane Bitter added a comment - The attached patch implements this when the SSL Plugin is enabled and both AMQP and AMQPS are configured to use the same port (i.e. port == ssl-port).
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Review request for Andrew Stitcher, michael goulish and Chug Rolke.

          Summary
          -------

          Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project.

          This addresses bug QPID-3514.
          https://issues.apache.org/jira/browse/QPID-3514

          Diffs


          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157
          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157
          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157
          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157
          /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157
          /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157
          /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157

          Diff: https://reviews.apache.org/r/2206/diff

          Testing
          -------

          make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522)

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2206/ ----------------------------------------------------------- Review request for Andrew Stitcher, michael goulish and Chug Rolke. Summary ------- Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project. This addresses bug QPID-3514 . https://issues.apache.org/jira/browse/QPID-3514 Diffs /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157 /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157 Diff: https://reviews.apache.org/r/2206/diff Testing ------- make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522 ) Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Ship it!

          Patch looks fine to me in general. Only comment is the use of a dummy option to communicate between SSL and TCP transports seems a little odd. However there isn't really any better mechanism as yet and changes to allow a 'nicer' approach - i.e. richer configuration of transports - are probably better done as a separate patch anyway.

          • Gordon

          On 2011-10-05 13:34:16, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2206/

          -----------------------------------------------------------

          (Updated 2011-10-05 13:34:16)

          Review request for Andrew Stitcher, michael goulish and Chug Rolke.

          Summary

          -------

          Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project.

          This addresses bug QPID-3514.

          https://issues.apache.org/jira/browse/QPID-3514

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157

          Diff: https://reviews.apache.org/r/2206/diff

          Testing

          -------

          make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522)

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2206/#review2339 ----------------------------------------------------------- Ship it! Patch looks fine to me in general. Only comment is the use of a dummy option to communicate between SSL and TCP transports seems a little odd. However there isn't really any better mechanism as yet and changes to allow a 'nicer' approach - i.e. richer configuration of transports - are probably better done as a separate patch anyway. Gordon On 2011-10-05 13:34:16, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2206/ ----------------------------------------------------------- (Updated 2011-10-05 13:34:16) Review request for Andrew Stitcher, michael goulish and Chug Rolke. Summary ------- Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project. This addresses bug QPID-3514 . https://issues.apache.org/jira/browse/QPID-3514 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157 /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157 Diff: https://reviews.apache.org/r/2206/diff Testing ------- make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522 ) Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2011-10-05 13:36:58.192640)

          Review request for Andrew Stitcher, michael goulish, Ted Ross, and Chug Rolke.

          Summary
          -------

          Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project.

          This addresses bug QPID-3514.
          https://issues.apache.org/jira/browse/QPID-3514

          Diffs


          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157
          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157
          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157
          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157
          /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157
          /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157
          /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157

          Diff: https://reviews.apache.org/r/2206/diff

          Testing
          -------

          make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522)

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2206/ ----------------------------------------------------------- (Updated 2011-10-05 13:36:58.192640) Review request for Andrew Stitcher, michael goulish, Ted Ross, and Chug Rolke. Summary ------- Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project. This addresses bug QPID-3514 . https://issues.apache.org/jira/browse/QPID-3514 Diffs /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157 /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157 Diff: https://reviews.apache.org/r/2206/diff Testing ------- make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522 ) Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

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

            • We can't accept this as it stands as it will break IPv6 support if turned on. **

          On the whole I think it is moving in the correct direction, but it is only partly formed:

          • This works in effect by adding in another transport type - the muxed SSL transport. We are trying to reduce and simplify the number of transports not increase them, this will make the code harder to maintain over all.
          • It's not clear to me why we wouldn't want this always turned on if we have the capability. In other words what is the benefit of ever turning it off. When we can do this switch then we should have it on for all listening sockets, the only option should be to allow a port to only accept encrypted connections.
          • This code is tactical, not strategic in that it works for this particular case, but as far as I can tell won't help in the upcoming amqp 1.0 case where we would need to switch to TLS processing sometime during the amqp protocol processing.
          • The code that selects between encrypted and non encrypted code paths should be unified with the code that checks the protocol version, currently the accept code does this and blocks whilst reading some bytes from the connection. I really don't like this approach.
          • Actually this code makes me wonder if we should change the architecture of the socket code to have an explicit piece of code which is run just after accepting a socket to select which bits of code get run next.
          • Andrew

          On 2011-10-05 13:36:58, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2206/

          -----------------------------------------------------------

          (Updated 2011-10-05 13:36:58)

          Review request for Andrew Stitcher, michael goulish, Ted Ross, and Chug Rolke.

          Summary

          -------

          Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project.

          This addresses bug QPID-3514.

          https://issues.apache.org/jira/browse/QPID-3514

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157

          Diff: https://reviews.apache.org/r/2206/diff

          Testing

          -------

          make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522)

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2206/#review2342 ----------------------------------------------------------- We can't accept this as it stands as it will break IPv6 support if turned on. ** On the whole I think it is moving in the correct direction, but it is only partly formed: This works in effect by adding in another transport type - the muxed SSL transport. We are trying to reduce and simplify the number of transports not increase them, this will make the code harder to maintain over all. It's not clear to me why we wouldn't want this always turned on if we have the capability. In other words what is the benefit of ever turning it off. When we can do this switch then we should have it on for all listening sockets, the only option should be to allow a port to only accept encrypted connections. This code is tactical, not strategic in that it works for this particular case, but as far as I can tell won't help in the upcoming amqp 1.0 case where we would need to switch to TLS processing sometime during the amqp protocol processing. The code that selects between encrypted and non encrypted code paths should be unified with the code that checks the protocol version, currently the accept code does this and blocks whilst reading some bytes from the connection. I really don't like this approach. Actually this code makes me wonder if we should change the architecture of the socket code to have an explicit piece of code which is run just after accepting a socket to select which bits of code get run next. Andrew On 2011-10-05 13:36:58, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2206/ ----------------------------------------------------------- (Updated 2011-10-05 13:36:58) Review request for Andrew Stitcher, michael goulish, Ted Ross, and Chug Rolke. Summary ------- Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project. This addresses bug QPID-3514 . https://issues.apache.org/jira/browse/QPID-3514 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157 /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157 Diff: https://reviews.apache.org/r/2206/diff Testing ------- make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522 ) Thanks, Gordon
          Hide
          Zane Bitter added a comment -

          It's true that this is a rather tactical patch. The goal from the point of view of the Matahari project was to come up with a patch that can be applied on top of 0.12 in Fedora with minimal impact to existing users (i.e. not breaking any ABIs, &c.).

          Of course I'm more than happy to work with y'all to come up with a better long-term solution for 0.14 and above, where I assume it's OK to change stuff like the plugin interface. I agree with Andrew's comments about what that should look like: accept the connection first and only decide on the type as part of the handshake code.

          Show
          Zane Bitter added a comment - It's true that this is a rather tactical patch. The goal from the point of view of the Matahari project was to come up with a patch that can be applied on top of 0.12 in Fedora with minimal impact to existing users (i.e. not breaking any ABIs, &c.). Of course I'm more than happy to work with y'all to come up with a better long-term solution for 0.14 and above, where I assume it's OK to change stuff like the plugin interface. I agree with Andrew's comments about what that should look like: accept the connection first and only decide on the type as part of the handshake code.
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-10-05 15:39:54, Andrew Stitcher wrote:

          > ** We can't accept this as it stands as it will break IPv6 support if turned on. **

          >

          > On the whole I think it is moving in the correct direction, but it is only partly formed:

          >

          > * This works in effect by adding in another transport type - the muxed SSL transport. We are trying to reduce and simplify the number of transports not increase them, this will make the code harder to maintain over all.

          >

          > * It's not clear to me why we wouldn't want this always turned on if we have the capability. In other words what is the benefit of ever turning it off. When we can do this switch then we should have it on for all listening sockets, the only option should be to allow a port to only accept encrypted connections.

          >

          > * This code is tactical, not strategic in that it works for this particular case, but as far as I can tell won't help in the upcoming amqp 1.0 case where we would need to switch to TLS processing sometime during the amqp protocol processing.

          >

          > * The code that selects between encrypted and non encrypted code paths should be unified with the code that checks the protocol version, currently the accept code does this and blocks whilst reading some bytes from the connection. I really don't like this approach.

          >

          > - Actually this code makes me wonder if we should change the architecture of the socket code to have an explicit piece of code which is run just after accepting a socket to select which bits of code get run next.

          >

          Is IPv6 supported for SSL at present? If not, and if you view this as adding the (optional) ability to serve plain sockets on the SSL enabled port, then one could argue its not really a regression. We would need to add IPv6 support to SSL anyway and it would seem to be to be largely independent of this change. If however we do support IPv6 on SSL and this breaks it then I would certainly agree with you.

          While I think its fair to characterise this as tactical, I don't think it makes a strategic solution any harder. Though it adds an additional protocol factory it re-uses code very well (better indeed than what is there at present). It would certainly need modifications for AMQP 1.0, but compared to the rest of the work required there I don't think that is particularly significant, nor do I think it makes things any worse.

          The strategic changes around this part of the code are more fundamental. Until there is something concrete planned there I think it would be a real shame to bar adding useful new features, providing they don't aggravate the situation (and I would argue this patch does not).

          Your point around the blocking in the accept is a good one. It would be much nicer to avoid blocking if possible. Given the dearth of documentation in this area, would you be willing to flesh out your preferred approach in a bit more detail?

          • Gordon

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

          On 2011-10-05 13:36:58, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2206/

          -----------------------------------------------------------

          (Updated 2011-10-05 13:36:58)

          Review request for Andrew Stitcher, michael goulish, Ted Ross, and Chug Rolke.

          Summary

          -------

          Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project.

          This addresses bug QPID-3514.

          https://issues.apache.org/jira/browse/QPID-3514

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157

          Diff: https://reviews.apache.org/r/2206/diff

          Testing

          -------

          make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522)

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-10-05 15:39:54, Andrew Stitcher wrote: > ** We can't accept this as it stands as it will break IPv6 support if turned on. ** > > On the whole I think it is moving in the correct direction, but it is only partly formed: > > * This works in effect by adding in another transport type - the muxed SSL transport. We are trying to reduce and simplify the number of transports not increase them, this will make the code harder to maintain over all. > > * It's not clear to me why we wouldn't want this always turned on if we have the capability. In other words what is the benefit of ever turning it off. When we can do this switch then we should have it on for all listening sockets, the only option should be to allow a port to only accept encrypted connections. > > * This code is tactical, not strategic in that it works for this particular case, but as far as I can tell won't help in the upcoming amqp 1.0 case where we would need to switch to TLS processing sometime during the amqp protocol processing. > > * The code that selects between encrypted and non encrypted code paths should be unified with the code that checks the protocol version, currently the accept code does this and blocks whilst reading some bytes from the connection. I really don't like this approach. > > - Actually this code makes me wonder if we should change the architecture of the socket code to have an explicit piece of code which is run just after accepting a socket to select which bits of code get run next. > Is IPv6 supported for SSL at present? If not, and if you view this as adding the (optional) ability to serve plain sockets on the SSL enabled port, then one could argue its not really a regression. We would need to add IPv6 support to SSL anyway and it would seem to be to be largely independent of this change. If however we do support IPv6 on SSL and this breaks it then I would certainly agree with you. While I think its fair to characterise this as tactical, I don't think it makes a strategic solution any harder. Though it adds an additional protocol factory it re-uses code very well (better indeed than what is there at present). It would certainly need modifications for AMQP 1.0, but compared to the rest of the work required there I don't think that is particularly significant, nor do I think it makes things any worse. The strategic changes around this part of the code are more fundamental. Until there is something concrete planned there I think it would be a real shame to bar adding useful new features, providing they don't aggravate the situation (and I would argue this patch does not). Your point around the blocking in the accept is a good one. It would be much nicer to avoid blocking if possible. Given the dearth of documentation in this area, would you be willing to flesh out your preferred approach in a bit more detail? Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2206/#review2342 ----------------------------------------------------------- On 2011-10-05 13:36:58, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2206/ ----------------------------------------------------------- (Updated 2011-10-05 13:36:58) Review request for Andrew Stitcher, michael goulish, Ted Ross, and Chug Rolke. Summary ------- Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project. This addresses bug QPID-3514 . https://issues.apache.org/jira/browse/QPID-3514 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157 /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157 Diff: https://reviews.apache.org/r/2206/diff Testing ------- make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522 ) Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-10-05 15:39:54, Andrew Stitcher wrote:

          > ** We can't accept this as it stands as it will break IPv6 support if turned on. **

          >

          > On the whole I think it is moving in the correct direction, but it is only partly formed:

          >

          > * This works in effect by adding in another transport type - the muxed SSL transport. We are trying to reduce and simplify the number of transports not increase them, this will make the code harder to maintain over all.

          >

          > * It's not clear to me why we wouldn't want this always turned on if we have the capability. In other words what is the benefit of ever turning it off. When we can do this switch then we should have it on for all listening sockets, the only option should be to allow a port to only accept encrypted connections.

          >

          > * This code is tactical, not strategic in that it works for this particular case, but as far as I can tell won't help in the upcoming amqp 1.0 case where we would need to switch to TLS processing sometime during the amqp protocol processing.

          >

          > * The code that selects between encrypted and non encrypted code paths should be unified with the code that checks the protocol version, currently the accept code does this and blocks whilst reading some bytes from the connection. I really don't like this approach.

          >

          > - Actually this code makes me wonder if we should change the architecture of the socket code to have an explicit piece of code which is run just after accepting a socket to select which bits of code get run next.

          >

          Gordon Sim wrote:

          Is IPv6 supported for SSL at present? If not, and if you view this as adding the (optional) ability to serve plain sockets on the SSL enabled port, then one could argue its not really a regression. We would need to add IPv6 support to SSL anyway and it would seem to be to be largely independent of this change. If however we do support IPv6 on SSL and this breaks it then I would certainly agree with you.

          While I think its fair to characterise this as tactical, I don't think it makes a strategic solution any harder. Though it adds an additional protocol factory it re-uses code very well (better indeed than what is there at present). It would certainly need modifications for AMQP 1.0, but compared to the rest of the work required there I don't think that is particularly significant, nor do I think it makes things any worse.

          The strategic changes around this part of the code are more fundamental. Until there is something concrete planned there I think it would be a real shame to bar adding useful new features, providing they don't aggravate the situation (and I would argue this patch does not).

          Your point around the blocking in the accept is a good one. It would be much nicer to avoid blocking if possible. Given the dearth of documentation in this area, would you be willing to flesh out your preferred approach in a bit more detail?

          I disagree in principle with the idea that adding more code when we really need to simplify the code doesn't make our job going forward any harder.

          The code in this area is just within my ability to comprehend, adding more just makes it harder for me. So while this is a reasonably neat tactical job I strongly suggest that including this code before simplifying what already there will make it harder to simplify, I know it will make it harder for me to simplify.

          • Andrew

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

          On 2011-10-05 13:36:58, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2206/

          -----------------------------------------------------------

          (Updated 2011-10-05 13:36:58)

          Review request for Andrew Stitcher, michael goulish, Ted Ross, and Chug Rolke.

          Summary

          -------

          Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project.

          This addresses bug QPID-3514.

          https://issues.apache.org/jira/browse/QPID-3514

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157

          Diff: https://reviews.apache.org/r/2206/diff

          Testing

          -------

          make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522)

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-10-05 15:39:54, Andrew Stitcher wrote: > ** We can't accept this as it stands as it will break IPv6 support if turned on. ** > > On the whole I think it is moving in the correct direction, but it is only partly formed: > > * This works in effect by adding in another transport type - the muxed SSL transport. We are trying to reduce and simplify the number of transports not increase them, this will make the code harder to maintain over all. > > * It's not clear to me why we wouldn't want this always turned on if we have the capability. In other words what is the benefit of ever turning it off. When we can do this switch then we should have it on for all listening sockets, the only option should be to allow a port to only accept encrypted connections. > > * This code is tactical, not strategic in that it works for this particular case, but as far as I can tell won't help in the upcoming amqp 1.0 case where we would need to switch to TLS processing sometime during the amqp protocol processing. > > * The code that selects between encrypted and non encrypted code paths should be unified with the code that checks the protocol version, currently the accept code does this and blocks whilst reading some bytes from the connection. I really don't like this approach. > > - Actually this code makes me wonder if we should change the architecture of the socket code to have an explicit piece of code which is run just after accepting a socket to select which bits of code get run next. > Gordon Sim wrote: Is IPv6 supported for SSL at present? If not, and if you view this as adding the (optional) ability to serve plain sockets on the SSL enabled port, then one could argue its not really a regression. We would need to add IPv6 support to SSL anyway and it would seem to be to be largely independent of this change. If however we do support IPv6 on SSL and this breaks it then I would certainly agree with you. While I think its fair to characterise this as tactical, I don't think it makes a strategic solution any harder. Though it adds an additional protocol factory it re-uses code very well (better indeed than what is there at present). It would certainly need modifications for AMQP 1.0, but compared to the rest of the work required there I don't think that is particularly significant, nor do I think it makes things any worse. The strategic changes around this part of the code are more fundamental. Until there is something concrete planned there I think it would be a real shame to bar adding useful new features, providing they don't aggravate the situation (and I would argue this patch does not). Your point around the blocking in the accept is a good one. It would be much nicer to avoid blocking if possible. Given the dearth of documentation in this area, would you be willing to flesh out your preferred approach in a bit more detail? I disagree in principle with the idea that adding more code when we really need to simplify the code doesn't make our job going forward any harder. The code in this area is just within my ability to comprehend, adding more just makes it harder for me. So while this is a reasonably neat tactical job I strongly suggest that including this code before simplifying what already there will make it harder to simplify, I know it will make it harder for me to simplify. Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2206/#review2342 ----------------------------------------------------------- On 2011-10-05 13:36:58, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2206/ ----------------------------------------------------------- (Updated 2011-10-05 13:36:58) Review request for Andrew Stitcher, michael goulish, Ted Ross, and Chug Rolke. Summary ------- Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project. This addresses bug QPID-3514 . https://issues.apache.org/jira/browse/QPID-3514 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157 /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157 Diff: https://reviews.apache.org/r/2206/diff Testing ------- make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522 ) Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-10-05 15:39:54, Andrew Stitcher wrote:

          > ** We can't accept this as it stands as it will break IPv6 support if turned on. **

          >

          > On the whole I think it is moving in the correct direction, but it is only partly formed:

          >

          > * This works in effect by adding in another transport type - the muxed SSL transport. We are trying to reduce and simplify the number of transports not increase them, this will make the code harder to maintain over all.

          >

          > * It's not clear to me why we wouldn't want this always turned on if we have the capability. In other words what is the benefit of ever turning it off. When we can do this switch then we should have it on for all listening sockets, the only option should be to allow a port to only accept encrypted connections.

          >

          > * This code is tactical, not strategic in that it works for this particular case, but as far as I can tell won't help in the upcoming amqp 1.0 case where we would need to switch to TLS processing sometime during the amqp protocol processing.

          >

          > * The code that selects between encrypted and non encrypted code paths should be unified with the code that checks the protocol version, currently the accept code does this and blocks whilst reading some bytes from the connection. I really don't like this approach.

          >

          > - Actually this code makes me wonder if we should change the architecture of the socket code to have an explicit piece of code which is run just after accepting a socket to select which bits of code get run next.

          >

          Gordon Sim wrote:

          Is IPv6 supported for SSL at present? If not, and if you view this as adding the (optional) ability to serve plain sockets on the SSL enabled port, then one could argue its not really a regression. We would need to add IPv6 support to SSL anyway and it would seem to be to be largely independent of this change. If however we do support IPv6 on SSL and this breaks it then I would certainly agree with you.

          While I think its fair to characterise this as tactical, I don't think it makes a strategic solution any harder. Though it adds an additional protocol factory it re-uses code very well (better indeed than what is there at present). It would certainly need modifications for AMQP 1.0, but compared to the rest of the work required there I don't think that is particularly significant, nor do I think it makes things any worse.

          The strategic changes around this part of the code are more fundamental. Until there is something concrete planned there I think it would be a real shame to bar adding useful new features, providing they don't aggravate the situation (and I would argue this patch does not).

          Your point around the blocking in the accept is a good one. It would be much nicer to avoid blocking if possible. Given the dearth of documentation in this area, would you be willing to flesh out your preferred approach in a bit more detail?

          Andrew Stitcher wrote:

          I disagree in principle with the idea that adding more code when we really need to simplify the code doesn't make our job going forward any harder.

          The code in this area is just within my ability to comprehend, adding more just makes it harder for me. So while this is a reasonably neat tactical job I strongly suggest that including this code before simplifying what already there will make it harder to simplify, I know it will make it harder for me to simplify.

          As a note my plan for making SSL run over IPv6 does indeed feature some of the simplification I'm talking about, specifically I'm introducing a Socket interface implemented by both SSL and regular TCP sockets so that the higher level code can be reduced to a single implementation.

          This approach also allows for handing over a socket to the SSL code from the TCP code during the negotiation if required.

          So overall my approach to this would be opposite make the TCP code detect SSL connections and create the SSL wiring, rather than have a specific option to make the SSL code allow for regular TCP connections.

          • Andrew

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

          On 2011-10-05 13:36:58, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2206/

          -----------------------------------------------------------

          (Updated 2011-10-05 13:36:58)

          Review request for Andrew Stitcher, michael goulish, Ted Ross, and Chug Rolke.

          Summary

          -------

          Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project.

          This addresses bug QPID-3514.

          https://issues.apache.org/jira/browse/QPID-3514

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157

          /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157

          /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157

          Diff: https://reviews.apache.org/r/2206/diff

          Testing

          -------

          make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522)

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-10-05 15:39:54, Andrew Stitcher wrote: > ** We can't accept this as it stands as it will break IPv6 support if turned on. ** > > On the whole I think it is moving in the correct direction, but it is only partly formed: > > * This works in effect by adding in another transport type - the muxed SSL transport. We are trying to reduce and simplify the number of transports not increase them, this will make the code harder to maintain over all. > > * It's not clear to me why we wouldn't want this always turned on if we have the capability. In other words what is the benefit of ever turning it off. When we can do this switch then we should have it on for all listening sockets, the only option should be to allow a port to only accept encrypted connections. > > * This code is tactical, not strategic in that it works for this particular case, but as far as I can tell won't help in the upcoming amqp 1.0 case where we would need to switch to TLS processing sometime during the amqp protocol processing. > > * The code that selects between encrypted and non encrypted code paths should be unified with the code that checks the protocol version, currently the accept code does this and blocks whilst reading some bytes from the connection. I really don't like this approach. > > - Actually this code makes me wonder if we should change the architecture of the socket code to have an explicit piece of code which is run just after accepting a socket to select which bits of code get run next. > Gordon Sim wrote: Is IPv6 supported for SSL at present? If not, and if you view this as adding the (optional) ability to serve plain sockets on the SSL enabled port, then one could argue its not really a regression. We would need to add IPv6 support to SSL anyway and it would seem to be to be largely independent of this change. If however we do support IPv6 on SSL and this breaks it then I would certainly agree with you. While I think its fair to characterise this as tactical, I don't think it makes a strategic solution any harder. Though it adds an additional protocol factory it re-uses code very well (better indeed than what is there at present). It would certainly need modifications for AMQP 1.0, but compared to the rest of the work required there I don't think that is particularly significant, nor do I think it makes things any worse. The strategic changes around this part of the code are more fundamental. Until there is something concrete planned there I think it would be a real shame to bar adding useful new features, providing they don't aggravate the situation (and I would argue this patch does not). Your point around the blocking in the accept is a good one. It would be much nicer to avoid blocking if possible. Given the dearth of documentation in this area, would you be willing to flesh out your preferred approach in a bit more detail? Andrew Stitcher wrote: I disagree in principle with the idea that adding more code when we really need to simplify the code doesn't make our job going forward any harder. The code in this area is just within my ability to comprehend, adding more just makes it harder for me. So while this is a reasonably neat tactical job I strongly suggest that including this code before simplifying what already there will make it harder to simplify, I know it will make it harder for me to simplify. As a note my plan for making SSL run over IPv6 does indeed feature some of the simplification I'm talking about, specifically I'm introducing a Socket interface implemented by both SSL and regular TCP sockets so that the higher level code can be reduced to a single implementation. This approach also allows for handing over a socket to the SSL code from the TCP code during the negotiation if required. So overall my approach to this would be opposite make the TCP code detect SSL connections and create the SSL wiring, rather than have a specific option to make the SSL code allow for regular TCP connections. Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2206/#review2342 ----------------------------------------------------------- On 2011-10-05 13:36:58, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2206/ ----------------------------------------------------------- (Updated 2011-10-05 13:36:58) Review request for Andrew Stitcher, michael goulish, Ted Ross, and Chug Rolke. Summary ------- Allow SSL and non-SSL connections on the same port. Patch contributed by Zane Bitter for use by the Matahari Project. This addresses bug QPID-3514 . https://issues.apache.org/jira/browse/QPID-3514 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1179157 /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1179157 /trunk/qpid/cpp/src/qpid/sys/Socket.h 1179157 /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1179157 Diff: https://reviews.apache.org/r/2206/diff Testing ------- make check passes; tested with and without the multiplexing; tested client authentication; tested SASL EXTERNAL (requires reverting a recent change unrelated to this, see QPID-3522 ) Thanks, Gordon

            People

            • Assignee:
              Gordon Sim
              Reporter:
              Zane Bitter
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development