Qpid
  1. Qpid
  2. QPID-4676

[Java Broker] SSL Client Authentication with username constructed in the same way as on C++ broker

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.22
    • Component/s: Java Broker
    • Labels:
      None

      Description

      The current versions of both the Java broker and the C++ broker support EXTERNAL authentication mechanism / SSL Client Authentication. Nevertheless the implementations are not fully identical. One of the differences is the way the usernames are constructed for the authenticated connections.

      The C++ broker takes the CN of the certificate (+ eventually the domain components / DCs) and creates a username as <CN>@<DC1>.<DC2>.<DC3>....<DCN>. In case there are no DCs, only the CN is used. On the other hand, the Java broker takes the full distinguished name as the username.

      Example 1:
      DN: CN=person
      C++ username: person
      Java username: CN=person

      Example 2:
      DN: CN=person,DC=example,DC=com
      C++ username: person@example.com
      Java username: CN=person,DC=example,DC=com

      Example 3:
      DN: CN=person,DC=example,DC=com,O=My Company Ltd,L=Newbury,ST=Berkshire,C=GB
      C++ username: person@example.com
      Java username: CN=person,DC=example,DC=com,O=My Company Ltd,L=Newbury,ST=Berkshire,C=GB

      This difference between C++ and Java broker makes it more complicated to migrate between the different brokers. Also, the particular implementation in Java can make it a bit more complicated to use the SSL Client Authentication in some cases.

      Therefore I implemented an enhancement which adds a possibility to the Java broker to construct the usernames in the same way the C++ broker does. I added a new configuration attribute "useCNAsUsername" for the ExternalAuthenticationProvider / Manager which allows switching between the current DN based usernames and the CN based usernames. By default, the old behaviour is used. The setting is passed from the ExternalAuthenticationManager to the ExternalSaslServer, which (in case the useCNAsUsername is switched ON) constructs the new username and returns it using the UsernamePrinciple. I also added some unit tests to cover the new functionality.

      It would be great if someone can have a look at the attached patch and eventually provide me with some review comments. One of the areas where I'm not entirely sure about the patch is the way it handles the configuration parameter from the JSON configuration. It is also no entirely clear to me what is the strategy for naming the parameters - it seems to be kind of a mixture of short names, long names, lower-case only, camel-case etc.

      I didn't added any new systests to cover this feature - I have not really an idea how to implement them. Especially how to get the authenticated username once connected. If you point me in some direction, I can have a look at it.

      1. QPID-4676-systest.patch
        5 kB
        JAkub Scholz
      2. QPID-4676.patch
        12 kB
        JAkub Scholz

        Activity

        Hide
        Robbie Gemmell added a comment - - edited

        Systest tweaked on trunk in order to ensure the certificate used is deterministic regardless of JVM in use
        http://svn.apache.org/r1464938

        This is to fix the test failures when running on the IBM JDK on Jenkins. The client keystore has 2 certs, and it is selecting the default differently depending on the JVM, where 'app1' cert gets selected by default on the IBM JDK on Jenkins rather than 'app2' which has been the case on all the JVMs used thus far to run the test.

        Show
        Robbie Gemmell added a comment - - edited Systest tweaked on trunk in order to ensure the certificate used is deterministic regardless of JVM in use http://svn.apache.org/r1464938 This is to fix the test failures when running on the IBM JDK on Jenkins. The client keystore has 2 certs, and it is selecting the default differently depending on the JVM, where 'app1' cert gets selected by default on the IBM JDK on Jenkins rather than 'app2' which has been the case on all the JVMs used thus far to run the test.
        Hide
        Robbie Gemmell added a comment -

        Systest merged to 0.22 branch in: http://svn.apache.org/r1464456

        Show
        Robbie Gemmell added a comment - Systest merged to 0.22 branch in: http://svn.apache.org/r1464456
        Hide
        Justin Ross added a comment -

        System tests reviewed by Robbie and approved for 0.22.

        Show
        Justin Ross added a comment - System tests reviewed by Robbie and approved for 0.22.
        Hide
        Robbie Gemmell added a comment -

        ..or a little earlier.

        The tests looked good to me and are now on trunk:
        http://svn.apache.org/r1463353

        Show
        Robbie Gemmell added a comment - ..or a little earlier. The tests looked good to me and are now on trunk: http://svn.apache.org/r1463353
        Hide
        Robbie Gemmell added a comment - - edited

        Thanks Jakub, I'll take a look first thing in the morning.

        Show
        Robbie Gemmell added a comment - - edited Thanks Jakub, I'll take a look first thing in the morning.
        Hide
        JAkub Scholz added a comment -

        As promised, here are the system tests. They are just 2 of them - one for the default behaviour and one for the full DN behaviour. As you suggested, I used JMX to get the username. Please let me know if you have any comments.

        Show
        JAkub Scholz added a comment - As promised, here are the system tests. They are just 2 of them - one for the default behaviour and one for the full DN behaviour. As you suggested, I used JMX to get the username. Please let me know if you have any comments.
        Hide
        Robbie Gemmell added a comment -

        r1463074 was merged via http://svn.apache.org/r1463192

        Leaving JIRA In-Progress as reminder about the systest, but moving to 0.22 fix-for.

        (Noticing while merging that I seem to have erroneously cropped off the patch attribution from my original commit message when I editing it at the last minute, I have put one in the merge commit, sorry Jakub ).

        Show
        Robbie Gemmell added a comment - r1463074 was merged via http://svn.apache.org/r1463192 Leaving JIRA In-Progress as reminder about the systest, but moving to 0.22 fix-for. (Noticing while merging that I seem to have erroneously cropped off the patch attribution from my original commit message when I editing it at the last minute, I have put one in the merge commit, sorry Jakub ).
        Hide
        Justin Ross added a comment -

        Reviewed by Robbie. Approved for 0.22.

        Show
        Justin Ross added a comment - Reviewed by Robbie. Approved for 0.22.
        Hide
        Robbie Gemmell added a comment -

        Thanks Jakub. I am going to request this for 0.22 now.

        For Justin: The current SSL client auth system tests are now all exercising this code path and I have verified the behaviour of the config attribute to restore the old behaviour manually prior to the addition of the new system test to do so. The feature itself is complete and I would like it to be in RC1 for other users to test. Leaving the JIRA 'In Progress' to reflect the fact a further test case will be added as protection against future breakage.

        Show
        Robbie Gemmell added a comment - Thanks Jakub. I am going to request this for 0.22 now. For Justin: The current SSL client auth system tests are now all exercising this code path and I have verified the behaviour of the config attribute to restore the old behaviour manually prior to the addition of the new system test to do so. The feature itself is complete and I would like it to be in RC1 for other users to test. Leaving the JIRA 'In Progress' to reflect the fact a further test case will be added as protection against future breakage.
        Hide
        JAkub Scholz added a comment -

        Hi Robbie,

        I tested your commit and it seems to work fine. I will have a look at the systests later ...

        Thanks & Regards
        Jakub

        Show
        JAkub Scholz added a comment - Hi Robbie, I tested your commit and it seems to work fine. I will have a look at the systests later ... Thanks & Regards Jakub
        Hide
        Robbie Gemmell added a comment -

        Hi Jakub,

        I had a chat with Rob about this and we both thought we should make it the default behaviour.

        I have made some changes to the patch to that effect (and some other bits and pieces) and have committed it to trunk:
        http://svn.apache.org/r1463074

        If youd like to try it out and let me know how you get on, we can look to request its inclusion in 0.22.

        One of the changes I made was around handling of the attribute, as the authentication providers are currently a bit lacking in that area since they've only currently had String attributes. I made a slight change (admitedly ugly) to make it deal with either a String or boolean in there as I think the current handling on the web ui will end up sending it Strings I believe.

        For the most part the attribute names are meant to be camel case (though we have just added prefixes to some of them, to indicate the related object type, and those are lower case in keeping with other places we use the type like that). There are admitedly a few that slightly break from camel case though.

        I think the unit testing you have added is sufficient for the most part, I guess the only thing to system test would be whether setting the attribute on the authentication provider makes a difference in the full running system. I did a quick hack of the tests to check via the log that it does indeed have that effect.

        In terms of undertaking that as a proper system test I think I would use one of the management interfaces to query the username of connections when starting the broker with the attribute on the external authentication provider set each way. Sticking such a test in ExternalAuthenticationTest seems reasonable, with ConnectionManagementTest being a good example for the JMX interface since it already does that test in testGetAttributes (there is also ConnectionRestTest as well for the HTTP interface, but I think JMX would be easier to use for this case).

        Time I got some sleep I think

        Robbie

        Show
        Robbie Gemmell added a comment - Hi Jakub, I had a chat with Rob about this and we both thought we should make it the default behaviour. I have made some changes to the patch to that effect (and some other bits and pieces) and have committed it to trunk: http://svn.apache.org/r1463074 If youd like to try it out and let me know how you get on, we can look to request its inclusion in 0.22. One of the changes I made was around handling of the attribute, as the authentication providers are currently a bit lacking in that area since they've only currently had String attributes. I made a slight change (admitedly ugly) to make it deal with either a String or boolean in there as I think the current handling on the web ui will end up sending it Strings I believe. For the most part the attribute names are meant to be camel case (though we have just added prefixes to some of them, to indicate the related object type, and those are lower case in keeping with other places we use the type like that). There are admitedly a few that slightly break from camel case though. I think the unit testing you have added is sufficient for the most part, I guess the only thing to system test would be whether setting the attribute on the authentication provider makes a difference in the full running system. I did a quick hack of the tests to check via the log that it does indeed have that effect. In terms of undertaking that as a proper system test I think I would use one of the management interfaces to query the username of connections when starting the broker with the attribute on the external authentication provider set each way. Sticking such a test in ExternalAuthenticationTest seems reasonable, with ConnectionManagementTest being a good example for the JMX interface since it already does that test in testGetAttributes (there is also ConnectionRestTest as well for the HTTP interface, but I think JMX would be easier to use for this case). Time I got some sleep I think Robbie

          People

          • Assignee:
            Robbie Gemmell
            Reporter:
            JAkub Scholz
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development