Derby
  1. Derby
  2. DERBY-2363

Add initial handshake on connection setup to determine server's required ssl support level and avoid client side attribute settings.

    Details

    • Urgency:
      Normal
    • Issue & fix info:
      High Value Fix
    • Bug behavior facts:
      Security

      Description

      Based upon some of the discussion in DERBY-2108, it would be useful to have some initial handshake between the client and the server to indicate the required level of ssl support. This would avoid client applications having to setup ssl related JDBC attributes or DataSource properties.
      Thus one could change the server to be ssl enabled without having to change any applications.

        Issue Links

          Activity

          Daniel John Debrunner created issue -
          Hide
          Bernt M. Johnsen added a comment -

          I like this idea. The client could also keep a hashtable of all host/portnumber pairs to keep track of which servers that are plaintext and thus avoid the overhead of trying out SSL on servers that already proven themselves to be plaintext.

          DERBY-2356 proposes three modes for ssl: off, basic and peerAuthentication. If we add negotiable and use that as default for the client we will have all we need.

          Show
          Bernt M. Johnsen added a comment - I like this idea. The client could also keep a hashtable of all host/portnumber pairs to keep track of which servers that are plaintext and thus avoid the overhead of trying out SSL on servers that already proven themselves to be plaintext. DERBY-2356 proposes three modes for ssl: off, basic and peerAuthentication. If we add negotiable and use that as default for the client we will have all we need.
          Bernt M. Johnsen made changes -
          Field Original Value New Value
          Assignee Bernt M. Johnsen [ bernt ]
          Bernt M. Johnsen made changes -
          Link This issue is related to DERBY-2356 [ DERBY-2356 ]
          Bernt M. Johnsen made changes -
          Link This issue is related to DERBY-2108 [ DERBY-2108 ]
          Bernt M. Johnsen made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Bernt M. Johnsen made changes -
          Fix Version/s 10.3.0.0 [ 12310800 ]
          Hide
          Bernt M. Johnsen added a comment -

          Negotiating SSL/vs plaintext will give a higher latency in calls to getConnection. Since an implementation will have to try SSL before it tries plaintext, this latency will hit all clients talking to plaintext servers. The extra latency may be avoided by setting "ssl=off" explicitely.

          This could be avoided by storing a map of which server/port pairs that responds to SSL and which responds to plaintext. Storing this information in the client must be done under the assumption that if a server is restarted with other ssl settings, all clients will have to be restarted too so that this map may be cleared. Is that a safe assumption?

          Show
          Bernt M. Johnsen added a comment - Negotiating SSL/vs plaintext will give a higher latency in calls to getConnection. Since an implementation will have to try SSL before it tries plaintext, this latency will hit all clients talking to plaintext servers. The extra latency may be avoided by setting "ssl=off" explicitely. This could be avoided by storing a map of which server/port pairs that responds to SSL and which responds to plaintext. Storing this information in the client must be done under the assumption that if a server is restarted with other ssl settings, all clients will have to be restarted too so that this map may be cleared. Is that a safe assumption?
          Hide
          Bernt M. Johnsen added a comment -

          Experimenting with this feature, I find that if a client to do an SSL handshake towards a server running a plaintext socket, I get (of course, since the server in that case views the SSL traffic as garbage):

          Execution failed because of a Distributed Protocol Error: DRDA_Proto_SYNTAXRM; CODPNT arg = 0; Error Code Value = 3
          org.apache.derby.impl.drda.DRDAProtocolException: Execution failed because of a Distributed Protocol Error: DRDA_Proto_SYNTAXRM; CODPNT arg = 0; Error Code Value = 3
          at org.apache.derby.impl.drda.DRDAConnThread.throwSyntaxrm(DRDAConnThread.java:469)
          at org.apache.derby.impl.drda.DDMReader.readDssHeader(DDMReader.java:348)
          at org.apache.derby.impl.drda.DRDAConnThread.exchangeServerAttributes(DRDAConnThread.java:1025)
          at org.apache.derby.impl.drda.DRDAConnThread.sessionInitialState(DRDAConnThread.java:619)
          at org.apache.derby.impl.drda.DRDAConnThread.run(DRDAConnThread.java:265)

          This, can't be avoided, but adds to the resource overhead of the handshake, since the DRDAConnThread is terminated, which is wise since we don't know why we got the DRDAProtocolException. Also, I guess the text also should be changed to indicate a possibility of an SSL conection attempt. Any ideas?

          Show
          Bernt M. Johnsen added a comment - Experimenting with this feature, I find that if a client to do an SSL handshake towards a server running a plaintext socket, I get (of course, since the server in that case views the SSL traffic as garbage): Execution failed because of a Distributed Protocol Error: DRDA_Proto_SYNTAXRM; CODPNT arg = 0; Error Code Value = 3 org.apache.derby.impl.drda.DRDAProtocolException: Execution failed because of a Distributed Protocol Error: DRDA_Proto_SYNTAXRM; CODPNT arg = 0; Error Code Value = 3 at org.apache.derby.impl.drda.DRDAConnThread.throwSyntaxrm(DRDAConnThread.java:469) at org.apache.derby.impl.drda.DDMReader.readDssHeader(DDMReader.java:348) at org.apache.derby.impl.drda.DRDAConnThread.exchangeServerAttributes(DRDAConnThread.java:1025) at org.apache.derby.impl.drda.DRDAConnThread.sessionInitialState(DRDAConnThread.java:619) at org.apache.derby.impl.drda.DRDAConnThread.run(DRDAConnThread.java:265) This, can't be avoided, but adds to the resource overhead of the handshake, since the DRDAConnThread is terminated, which is wise since we don't know why we got the DRDAProtocolException. Also, I guess the text also should be changed to indicate a possibility of an SSL conection attempt. Any ideas?
          Hide
          Bernt M. Johnsen added a comment -

          I have now experimented with two possible approaches.

          1) Trying SSL first and fall back to plaintext if the SSL handshake fail. There is two problems with this approach. One is that clients by default will run somewhat slower against plaintext servers (the failed connect will increase the time it takes to connect) and this will probably be the most common scenario (of course this may be avoided by setting the ssl flag explicitely to off). The second problem is that the DRDA_Proto_SYNTAXRM in the initial DRDA handshake should not be displayed to users, but this will also mask any new bugs in the initial DRDA handshake, which I think is a bad thing.

          2) The second approach is to try out plaintext first, e.g. with a PING command. This will add an extra roundtrip to the initial sequence in an connect (this may be avoided by setting the ssl flag explicitely). This approach is anyway faster than approach 1) for plaintext servers. The problem with this approach is that all the admin commands (shutdown, sysinfo, ping etc) are implemented in NetworkServerControlImpl and use the error reporting apparatus available here, which is not quite suitable for this purpose. To make a good implementation for this approach, these admin commands should be split out from NetworkServerControlImpl into a separate class or package (which is wise anyway, since these commands are clients and not really very much related to the server).

          So?

          Show
          Bernt M. Johnsen added a comment - I have now experimented with two possible approaches. 1) Trying SSL first and fall back to plaintext if the SSL handshake fail. There is two problems with this approach. One is that clients by default will run somewhat slower against plaintext servers (the failed connect will increase the time it takes to connect) and this will probably be the most common scenario (of course this may be avoided by setting the ssl flag explicitely to off). The second problem is that the DRDA_Proto_SYNTAXRM in the initial DRDA handshake should not be displayed to users, but this will also mask any new bugs in the initial DRDA handshake, which I think is a bad thing. 2) The second approach is to try out plaintext first, e.g. with a PING command. This will add an extra roundtrip to the initial sequence in an connect (this may be avoided by setting the ssl flag explicitely). This approach is anyway faster than approach 1) for plaintext servers. The problem with this approach is that all the admin commands (shutdown, sysinfo, ping etc) are implemented in NetworkServerControlImpl and use the error reporting apparatus available here, which is not quite suitable for this purpose. To make a good implementation for this approach, these admin commands should be split out from NetworkServerControlImpl into a separate class or package (which is wise anyway, since these commands are clients and not really very much related to the server). So?
          Hide
          Daniel John Debrunner added a comment -

          bernt> To make a good implementation for this approach, these admin commands should be split out from NetworkServerControlImpl into a separate class or package (which is wise anyway, since these commands are clients and not really very much related to the server).

          +1

          In looking at the network startup code (DERBY-1966) it struck me that the NetworkServerControlImpl class is overly complex due to the mixing of client commands and the actual logic to run a network server. Such a split should probably stop using the name NetworkServerControlImpl and instead two classes with names that reflect their purpose, commands and server.

          Show
          Daniel John Debrunner added a comment - bernt> To make a good implementation for this approach, these admin commands should be split out from NetworkServerControlImpl into a separate class or package (which is wise anyway, since these commands are clients and not really very much related to the server). +1 In looking at the network startup code ( DERBY-1966 ) it struck me that the NetworkServerControlImpl class is overly complex due to the mixing of client commands and the actual logic to run a network server. Such a split should probably stop using the name NetworkServerControlImpl and instead two classes with names that reflect their purpose, commands and server.
          Bernt M. Johnsen made changes -
          Link This issue is blocked by DERBY-2412 [ DERBY-2412 ]
          Bernt M. Johnsen made changes -
          Status In Progress [ 3 ] Open [ 1 ]
          Bernt M. Johnsen made changes -
          Fix Version/s 10.3.0.0 [ 12310800 ]
          Kathey Marsden made changes -
          Assignee Bernt M. Johnsen [ bernt ]
          Dag H. Wanvik made changes -
          Derby Categories [Security]
          Dag H. Wanvik made changes -
          Component/s Security [ 11411 ]
          Hide
          Kathey Marsden added a comment -

          Triage for 10.9. If this can be achieved to allow SSL without client side application change and special configuration, that would be I think a big boost to Network Server security in the field. Marking High Value Fix

          Show
          Kathey Marsden added a comment - Triage for 10.9. If this can be achieved to allow SSL without client side application change and special configuration, that would be I think a big boost to Network Server security in the field. Marking High Value Fix
          Kathey Marsden made changes -
          Labels derby_triage10_9
          Urgency Normal [ 10052 ]
          Issue & fix info High Value Fix [ 10422 ]
          Gavin made changes -
          Workflow jira [ 12397894 ] Default workflow, editable Closed status [ 12799353 ]

            People

            • Assignee:
              Unassigned
              Reporter:
              Daniel John Debrunner
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:

                Development