Qpid
  1. Qpid
  2. QPID-1403

Add SSL support for C++ Windows broker/client

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: M4
    • Fix Version/s: 0.7
    • Component/s: C++ Broker, C++ Client
    • Labels:
      None
    • Environment:

      Windows XP+

      Description

      The recently added SSL support in the C++ side should be available to Windows as well. Not yet sure how much architectural work this may be.

      1. ssl-windows.patch
        66 kB
        Steve Huston

        Activity

        Steve Huston created issue -
        Rafael H. Schloming made changes -
        Field Original Value New Value
        Affects Version/s M4 [ 12313279 ]
        Hide
        Steve Huston added a comment -

        This won't be done for M4.

        Show
        Steve Huston added a comment - This won't be done for M4.
        Steve Huston made changes -
        Fix Version/s M4 [ 12313279 ]
        Hide
        Ffrench Mathilde added a comment - - edited

        Do you know when this feature will be available under windows ? For M5 release ?

        Show
        Ffrench Mathilde added a comment - - edited Do you know when this feature will be available under windows ? For M5 release ?
        Hide
        Steve Huston added a comment -

        This feature is not currently scheduled for implementation, so which release it will appear in is unknown.

        Show
        Steve Huston added a comment - This feature is not currently scheduled for implementation, so which release it will appear in is unknown.
        Hide
        Steve Huston added a comment -

        This is in progress and nearly done; must be in 0.6

        Show
        Steve Huston added a comment - This is in progress and nearly done; must be in 0.6
        Steve Huston made changes -
        Fix Version/s 0.6 [ 12313728 ]
        Priority Major [ 3 ] Blocker [ 1 ]
        Hide
        Andrew Stitcher added a comment -

        Why is this a blocking bug? It is a new feature - I don't see how a new feature could block a release.

        Show
        Andrew Stitcher added a comment - Why is this a blocking bug? It is a new feature - I don't see how a new feature could block a release.
        Andrew Stitcher made changes -
        Fix Version/s 0.6 [ 12313728 ]
        Priority Blocker [ 1 ] Major [ 3 ]
        Hide
        Steve Huston added a comment -

        Patch for addition of SSL support on Windows.

        The major approach difference from the Linux/NSS mechanism is that on Windows Schannel (their SSL facility) the I/O is done by normal socket calls. The negotiation/token processing, as well as encrypt/decrypt operations, are function calls that are made with data to/from the socket. This is in contrast to the NSS way where there is a parallel set of SSL-enabled socket calls.

        Show
        Steve Huston added a comment - Patch for addition of SSL support on Windows. The major approach difference from the Linux/NSS mechanism is that on Windows Schannel (their SSL facility) the I/O is done by normal socket calls. The negotiation/token processing, as well as encrypt/decrypt operations, are function calls that are made with data to/from the socket. This is in contrast to the NSS way where there is a parallel set of SSL-enabled socket calls.
        Steve Huston made changes -
        Attachment ssl-windows.patch [ 12428314 ]
        Steve Huston made changes -
        Attachment ssl-windows.patch [ 12428314 ]
        Hide
        Steve Huston added a comment -

        Revised patch. At this point, the changes applied to the current trunk yield:

        • Windows <-> Windows, ok
        • Windows client -> Linux server, ok
        • Linux client -> Windows server, fail negotiation - certificate chain not trusted

        I assume the failure is more of a setup issue than a code correctness issue. I'd like to get interested people together for a review of the attached code.

        Show
        Steve Huston added a comment - Revised patch. At this point, the changes applied to the current trunk yield: Windows <-> Windows, ok Windows client -> Linux server, ok Linux client -> Windows server, fail negotiation - certificate chain not trusted I assume the failure is more of a setup issue than a code correctness issue. I'd like to get interested people together for a review of the attached code.
        Steve Huston made changes -
        Attachment ssl-windows.patch [ 12429692 ]
        Hide
        Andrew Stitcher added a comment -

        Comments on the patch of 8 Jan 2010:

        Naming:

        I think you should rename SslIoShim/ClientSslIoShim/ServerSslIoShim to reflect that these are actually all AsynchIO implementations:
        So maybe use names: SslAsynchIO/ClientSslAsynchIO etc.

        I like the implementation in SslProtocolFactory except separating the Client and Server IO is somewhat clunky.

        I really don't like the way that SslConnector works though, there's a lot of code just to put in place a few necessary interceptions. Also the Connectors were never intended to be inherited from just to implement an interface.

        I think this could be handled better by modifying TCPConnector to take an abstract factory that knows how to create AsynchIO/AsyncConnector objects that are relevant for the protocol. Then I'd say it'd be better to recast some of the logic in SslConnector as an implementation of AsynchConnector. Does that make sense? Of course this does mean a change in the protocol plugin API, but an improvement I think.

        Long term I'd like to eliminate the distinction between the ..Connector and ..ProtocolFactory plugins and be able to use just one for both client and server - this would likely be descended from the server code which already does both server and client ends.

        AsyncIOBufferBase::squish() this could be better named? And as you've added it, perhaps you could make the logic in AsynchIO::unread() use it?

        In the windows code: It is the general code convention that all system calls are explicitly called out by being explicitly in the global namespace, viz ::InitializeSecurityContext() to give the first example I came across.

        Personally I'd put both the Client "shim" and the Server "shim" code in the same implementation file (but that's mostly a my own preference)

        Show
        Andrew Stitcher added a comment - Comments on the patch of 8 Jan 2010: Naming: I think you should rename SslIoShim/ClientSslIoShim/ServerSslIoShim to reflect that these are actually all AsynchIO implementations: So maybe use names: SslAsynchIO/ClientSslAsynchIO etc. I like the implementation in SslProtocolFactory except separating the Client and Server IO is somewhat clunky. I really don't like the way that SslConnector works though, there's a lot of code just to put in place a few necessary interceptions. Also the Connectors were never intended to be inherited from just to implement an interface. I think this could be handled better by modifying TCPConnector to take an abstract factory that knows how to create AsynchIO/AsyncConnector objects that are relevant for the protocol. Then I'd say it'd be better to recast some of the logic in SslConnector as an implementation of AsynchConnector. Does that make sense? Of course this does mean a change in the protocol plugin API, but an improvement I think. Long term I'd like to eliminate the distinction between the ..Connector and ..ProtocolFactory plugins and be able to use just one for both client and server - this would likely be descended from the server code which already does both server and client ends. AsyncIOBufferBase::squish() this could be better named? And as you've added it, perhaps you could make the logic in AsynchIO::unread() use it? In the windows code: It is the general code convention that all system calls are explicitly called out by being explicitly in the global namespace, viz ::InitializeSecurityContext() to give the first example I came across. Personally I'd put both the Client "shim" and the Server "shim" code in the same implementation file (but that's mostly a my own preference)
        Hide
        Steve Huston added a comment -

        Re Andrew's comnments 21-Jan-2010:

        • I renamed the SslIoShim stuff to [Client|Server]SslAsynchIO as recommended; also moved to one file, SslAsynchIO. {h cpp}
        • I kind of like squish() as a name - it squishes the remaining bytes to the front.
        • I prefixed the windows API calls with ::
        • As we discussed on the phone with Gordon, the current SslConnector arrangement is better than just doing another copy-edit-drop of this code. There are already multiple variants of this code with slightly different features and fixes; another one is bad. When you get to the point where you want to actively work on a refactoring in this area, let me know and I'll do my best to help out.

        With that, code is committed to trunk r902318

        Show
        Steve Huston added a comment - Re Andrew's comnments 21-Jan-2010: I renamed the SslIoShim stuff to [Client|Server] SslAsynchIO as recommended; also moved to one file, SslAsynchIO. {h cpp} I kind of like squish() as a name - it squishes the remaining bytes to the front. I prefixed the windows API calls with :: As we discussed on the phone with Gordon, the current SslConnector arrangement is better than just doing another copy-edit-drop of this code. There are already multiple variants of this code with slightly different features and fixes; another one is bad. When you get to the point where you want to actively work on a refactoring in this area, let me know and I'll do my best to help out. With that, code is committed to trunk r902318
        Steve Huston made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 0.7 [ 12314455 ]
        Resolution Fixed [ 1 ]
        Justin Ross made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Steve Huston
            Reporter:
            Steve Huston
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development