Qpid
  1. Qpid
  2. QPID-4636

[Java Broker] add a 'peerstore' to enhance SSL Client Authentication functionality, provide similar 'trusted peer' support with self-signed certs as the C++ broker

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21
    • Component/s: Java Broker, Java Common
    • Labels:
      None

      Description

      Motivation:

      The C++ broker implementation is based on the NSS library from Mozilla. The user creates a certificate database and configures the broker to load the database at start-up. The NSS certificate database can store the private keys used by the broker as well as the public keys related to the connecting clients. The public keys can be divided into several groups - the keys of trusted CAs and the keys of trusted peers. The difference between the trusted CA and trusted peer is that the trusted CA allows logging in even to clients who have a certificate signed by the CA, while the peers allow logging in only to clients who have the certificate exactly matching the certificate loaded in the certificate database.

      The SSL Client Authentication in the Java broker is based on the Java JSSE implementation. The certificates are stored in the JKS format. The JKS format doesn't distinguish between trusted peers and trusted CA. Therefore all public keys behave as trusted CAs. As a result, the current implementation cannot be used together with self signed certificates. As we found out, there seems to be no support for the trusted peers in the JKS truststores.

      Proposed solution:

      The current configuration for the SSL Client Authentication supports only one truststore . We can add a second configuration entry which would allow to specify "peerstore" . When creating the SSL context, the existing truststore would be handled as it is handled today. If the "peerstore" is specified, the new TrustManager will be added to the SSL context. The custom TrustManager will use the peerstore to verify the peers only as peers. The client will pass the authentication if it is authenticated either with the original Trustmanager against the keystore or by the custom trust manager against the peer store.

      Configuration and patch explanation:

      The current configuration model for the broker (trunk) is based on JSON. We have added two optional attributes (peerStorePath, peerStorePassword):


      "keyStorePath": "...",
      "keyStorePassword": "123456",
      "trustStorePath": "...",
      "trustStorePassword": "123456",

      "peerStorePath": "...",
      "peerStorePassword": "123456",

      Internally, the broker is prepared to handle multiple truststores, since it can store them in the collection. If the above new attributes were specified, the additional truststore is added into the collection (BrokerAdapter.java). A new truststore will have optional flag TrustStore.PEERS_ONLY set to True. The SSLContextFactory was extended for collection configuration. The Broker creates the SSLContext using the collection of truststores (either only single truststore or with a new peerstore). The SSLFactory parses the collection and depending on the TrustStore.PEERS_ONLY flag creates either regular JSSE trustmanager or uses a newly introduced one QpidPeersOnlyTrustManager.

      The new QpidPeersOnlyTrustManager works as a wrapper around standard JSSE trustmanager. When client connects, the check is delegated to the underlying JSSE verification and if it succeeds, the additional check is done, whether the peer’s certificate (in the chain index 0) is present in the keystore file. Only then the client is authenticated, otherwise the CertificateException is thrown.

      Since SSLContext.init method from the array of trustmanagers uses only the first one which is an instance of the X509TrustManager class, we have created also the extension. Otherwise, it would not be possible to use simultaneously trustmanager (JSSE implementation) and peermanager (our new implementation) because both implements X509TrustManager and only the first from the array would be considered. Therefore, we have introduced the QpidMultipleTrustManager which is doing nothing else but delegating the check to its underlying X509TrustManagers and only if all fails, the check itself fails. If some underlying manager succeeds, the check itself succeeds as well. This QpidMultipleTrustManager is loaded with truststore and peerstore manager and added into the array which is further passed to the SSLContext.init method.

      The implementation attached in the patch seems to be working fine and adds the above requirements for peers only truststore. It is also backwards compatible- anyone without interest for peerstores will not see any change.

      1. ssl_new_trustmanager_test_extended.patch
        8 kB
        Michal Zerola
      2. java_client_untrusted_keystore.jks
        2 kB
        Michal Zerola
      3. ssl_new_trustmanager_test.patch
        9 kB
        Michal Zerola
      4. ssl_new_trustmanager.patch
        23 kB
        Michal Zerola
      5. java_broker_peerstore.jks
        1 kB
        Michal Zerola

        Issue Links

          Activity

          Hide
          Robbie Gemmell added a comment -

          Hi Michal,

          Thanks for the patch, I will try to take a proper look at it in the next couple of days with aim of getting it onto trunk. In the mean time I noticed a couple of things while skimming over it quickly:

          • There are some new files which need a licence header added at the top.
          • There doesn't seem to be any tests and it would be good to have some, if only to help prevent the functionality bring broken by accident at some point. Some related existing system tests that spring to mind are ExternalAuthenticationTest and SSLTest, and some unit tests might also be good for e.g the trust managers.

          Robbie

          Show
          Robbie Gemmell added a comment - Hi Michal, Thanks for the patch, I will try to take a proper look at it in the next couple of days with aim of getting it onto trunk. In the mean time I noticed a couple of things while skimming over it quickly: There are some new files which need a licence header added at the top. There doesn't seem to be any tests and it would be good to have some, if only to help prevent the functionality bring broken by accident at some point. Some related existing system tests that spring to mind are ExternalAuthenticationTest and SSLTest, and some unit tests might also be good for e.g the trust managers. Robbie
          Hide
          Michal Zerola added a comment -

          Hi Robbie,

          thank you. I have uploaded the updated version with needed licence headers at the top of two new files. I will take a look at possible tests which can be extended/added for the new peer manager support.

          Michal

          Show
          Michal Zerola added a comment - Hi Robbie, thank you. I have uploaded the updated version with needed licence headers at the top of two new files. I will take a look at possible tests which can be extended/added for the new peer manager support. Michal
          Hide
          Michal Zerola added a comment - - edited

          Hi Robbie,

          I have uploaded a second patch covering some tests. Inside SSLContextFactoryTest.java we have added two tests, one for testing the peer manager truststore and second for the multiple managers truststores. Along with this I have created also the JKS file which needs to be placed into the test-profiles/test_resources/ssl/. This store contains the public keys from the client (stored under the alias app1 and app2 respectively).

          The first test (testPeerStoreClientAuthentication) creates the new QpidPeersOnlyTrustManager and loads it with the new peerstore content. Then the client authentication is tested against this manager and should succeed (the client provides the chain of its certificate (app1) and since it is stored in the peer store all should pass). Later, the authentication is tested if the peer manager is loaded with the broker's store and doesn't contain the client's certificate. This test should fail.

          The second test (testMultipleStoreClientAuthentication) creates the new QpidMultipleTrustManager and loads it with both peermanager and "regular" trustmanager. The client's authentication should succeed.

          I have added also the check for the newly added attribute TrustStore.PEERS_ONLY into TrustStoreRecovererTest.java.

          Do you think it is enough?

          Michal

          Show
          Michal Zerola added a comment - - edited Hi Robbie, I have uploaded a second patch covering some tests. Inside SSLContextFactoryTest.java we have added two tests, one for testing the peer manager truststore and second for the multiple managers truststores. Along with this I have created also the JKS file which needs to be placed into the test-profiles/test_resources/ssl/. This store contains the public keys from the client (stored under the alias app1 and app2 respectively). The first test (testPeerStoreClientAuthentication) creates the new QpidPeersOnlyTrustManager and loads it with the new peerstore content. Then the client authentication is tested against this manager and should succeed (the client provides the chain of its certificate (app1) and since it is stored in the peer store all should pass). Later, the authentication is tested if the peer manager is loaded with the broker's store and doesn't contain the client's certificate. This test should fail. The second test (testMultipleStoreClientAuthentication) creates the new QpidMultipleTrustManager and loads it with both peermanager and "regular" trustmanager. The client's authentication should succeed. I have added also the check for the newly added attribute TrustStore.PEERS_ONLY into TrustStoreRecovererTest.java. Do you think it is enough? Michal
          Hide
          Robbie Gemmell added a comment - - edited

          Hi Michal,

          I took a look and the patches, and ran the default profile of the test suite against them while I was doing so. Unfortunately, a large number of the existing system tests which use SSL failed or threw errors. I didn't actually spot anything obvious wrong with the patches while I was looking them over, but its been a looong week so far (which is why im doing this after 11pm ) so I might have missed something. It's also possible the tests and/or previous implementation themselves are at issue, but eihter way it appears that it needs a further look into.

          Were you able to run the tests successfully? You can run them all by using 'ant test' in the java folder, and optionally specifying a particular test profile to run and a particular test name( or pattern of names/packages) to run. The default test profile is java-mms.0-10, which uses AMQP 0-10 and the Memory message store. You can see a full list of the profiles in the java/test-profiles folder. Typically its a good idea to run an AMQP 0-10 and 0-9-1 profile since their behaviour can differ fairly substantially, though I wouldnt really expect them to in the case of SSL. If you were to use any of the bdb profiles (which use the persistent bdb message store instead of the memory message store) you would also have to add -Doptional=true to allow the optional bdbstore module (and any other optional modules) to build. E.g:

          ant build test [[[-Dprofile=java-mms.0-9-1] -Dtest=SSLTest] -Doptional=true]

          Although I didn't spot anything with the functionality itself, I do think the tests could use a bit more work to ensure robustness. In testPeerStoreClientAuthentication() and testMultipleStoreClientAuthentication(), while posibly unlikely it seems as if the test could actually skip the important calls on the trust managers and still pass (due to the for and if statements, e.g empty list or unexpected key manager types) and we should possibly verify that hasn't occurred. I think it would still be useful to add some more tests to ensure the functionality isn't unwittingly broken in future. For example, checking that the QpidPeersOnlyTrustManager never returns trusted CAs, and the QpidMultipleTrustManager does indeed throw a CertificateException if presented with a cert that neither the QpidPeersOnlyTrustManager or a regular keystore TrustManager trusted. It doesn't seem like SSLContextFactory is actually used in the unit tests, so it might be best to move the tests to their own file in the package. Beyond that, adding even a single system test (e.g to ExternalAuthenticationTest) that shows the peerStore with the broker in operation still seems wise, since the unit tests dont strictly guarantee the peer store is actually hooked up properly within the broker.

          Robbie

          Show
          Robbie Gemmell added a comment - - edited Hi Michal, I took a look and the patches, and ran the default profile of the test suite against them while I was doing so. Unfortunately, a large number of the existing system tests which use SSL failed or threw errors. I didn't actually spot anything obvious wrong with the patches while I was looking them over, but its been a looong week so far (which is why im doing this after 11pm ) so I might have missed something. It's also possible the tests and/or previous implementation themselves are at issue, but eihter way it appears that it needs a further look into. Were you able to run the tests successfully? You can run them all by using 'ant test' in the java folder, and optionally specifying a particular test profile to run and a particular test name( or pattern of names/packages) to run. The default test profile is java-mms.0-10, which uses AMQP 0-10 and the Memory message store. You can see a full list of the profiles in the java/test-profiles folder. Typically its a good idea to run an AMQP 0-10 and 0-9-1 profile since their behaviour can differ fairly substantially, though I wouldnt really expect them to in the case of SSL. If you were to use any of the bdb profiles (which use the persistent bdb message store instead of the memory message store) you would also have to add -Doptional=true to allow the optional bdbstore module (and any other optional modules) to build. E.g: ant build test [[ [-Dprofile=java-mms.0-9-1] -Dtest=SSLTest] -Doptional=true] Although I didn't spot anything with the functionality itself, I do think the tests could use a bit more work to ensure robustness. In testPeerStoreClientAuthentication() and testMultipleStoreClientAuthentication(), while posibly unlikely it seems as if the test could actually skip the important calls on the trust managers and still pass (due to the for and if statements, e.g empty list or unexpected key manager types) and we should possibly verify that hasn't occurred. I think it would still be useful to add some more tests to ensure the functionality isn't unwittingly broken in future. For example, checking that the QpidPeersOnlyTrustManager never returns trusted CAs, and the QpidMultipleTrustManager does indeed throw a CertificateException if presented with a cert that neither the QpidPeersOnlyTrustManager or a regular keystore TrustManager trusted. It doesn't seem like SSLContextFactory is actually used in the unit tests, so it might be best to move the tests to their own file in the package. Beyond that, adding even a single system test (e.g to ExternalAuthenticationTest) that shows the peerStore with the broker in operation still seems wise, since the unit tests dont strictly guarantee the peer store is actually hooked up properly within the broker. Robbie
          Hide
          Michal Zerola added a comment -

          Hi Robbie,

          thanks for the comment. I spotted the reason why SSLTest was failing (missing return in checkServerTrusted). We were not experiencing this error because we tested the patched broker with client's libraries from the (yet unpatched) trunk and were not running the full test suite (only the newly added tests).

          I can still see errors with ExternalAuthenticationTest, but I think it is not caused by the above changes. It seems like the client tries to connect to the non-SSL port and expects the SSL communication. Can you perhaps look at it, whether the test configuration for this test is properly constructed together with the JSON config file?

          I have placed the tests into the new class TrustManagerTest in the same package. I have extended the test for the peer store, so it requires the store to contain only the clients certificate (app1 or app2). The QpidPeersOnlyTrustManager check has to be called at least on one manager instance as well.

          I can add the test for QpidMultipleTrustManager to check that it fails if the client's certificate is not valid. Do you suggest to create a client's fail store for this? Generating the random certificate for the fail test would require some X509 library which I do not think is necessary.

          The ExternalAuthenticationTest can be easily extended for simple SSL check with broker setup to use peerstore, however I am experiencing the above mentioned problem. If you can take a look at it, it would be great.

          Thank you,

          Michal

          Show
          Michal Zerola added a comment - Hi Robbie, thanks for the comment. I spotted the reason why SSLTest was failing (missing return in checkServerTrusted). We were not experiencing this error because we tested the patched broker with client's libraries from the (yet unpatched) trunk and were not running the full test suite (only the newly added tests). I can still see errors with ExternalAuthenticationTest, but I think it is not caused by the above changes. It seems like the client tries to connect to the non-SSL port and expects the SSL communication. Can you perhaps look at it, whether the test configuration for this test is properly constructed together with the JSON config file? I have placed the tests into the new class TrustManagerTest in the same package. I have extended the test for the peer store, so it requires the store to contain only the clients certificate (app1 or app2). The QpidPeersOnlyTrustManager check has to be called at least on one manager instance as well. I can add the test for QpidMultipleTrustManager to check that it fails if the client's certificate is not valid. Do you suggest to create a client's fail store for this? Generating the random certificate for the fail test would require some X509 library which I do not think is necessary. The ExternalAuthenticationTest can be easily extended for simple SSL check with broker setup to use peerstore, however I am experiencing the above mentioned problem. If you can take a look at it, it would be great. Thank you, Michal
          Hide
          Robbie Gemmell added a comment -

          Hi Michal,

          Thanks for the update. We applied the new patches and ran the tests, which all passed except a couple of the Broker REST tests. I fixed those up and have now committed the patches to trunk:
          http://svn.apache.org/viewvc?view=revision&revision=1456554
          http://svn.apache.org/viewvc?view=revision&revision=1456555
          http://svn.apache.org/viewvc?view=revision&revision=1456556

          I think adding the untrusted cert in its own store (e.g java_untrusted_cert_store.jks) in order to do the unit test is the way to go. We should really have more tests in future requiring such a cert, so adding it seems a better idea than trying to generate one each time within the test.

          I'm not sure why you were seeing issues running ExternalAuthenticationTest, we tried it on a couple of machines with your new patches and had no issue. If you can, try again with the updated trunk and a clean build ('ant clean' in the java directory wipes out the entire existing build) and see if you are still having an issue, then attach the test output (java/build/results/systests/ExternalAuthenticationTest ) if you are and we will take a closer look.

          Thanks,
          Robbie

          Show
          Robbie Gemmell added a comment - Hi Michal, Thanks for the update. We applied the new patches and ran the tests, which all passed except a couple of the Broker REST tests. I fixed those up and have now committed the patches to trunk: http://svn.apache.org/viewvc?view=revision&revision=1456554 http://svn.apache.org/viewvc?view=revision&revision=1456555 http://svn.apache.org/viewvc?view=revision&revision=1456556 I think adding the untrusted cert in its own store (e.g java_untrusted_cert_store.jks) in order to do the unit test is the way to go. We should really have more tests in future requiring such a cert, so adding it seems a better idea than trying to generate one each time within the test. I'm not sure why you were seeing issues running ExternalAuthenticationTest, we tried it on a couple of machines with your new patches and had no issue. If you can, try again with the updated trunk and a clean build ('ant clean' in the java directory wipes out the entire existing build) and see if you are still having an issue, then attach the test output (java/build/results/systests/ ExternalAuthenticationTest ) if you are and we will take a closer look. Thanks, Robbie
          Hide
          Michal Zerola added a comment -

          Hi Robbie,

          I am glad for the patch. The 'problems' I have seen with ExternalAuthenticationTest were just my misinterpretation of the *.out files, since I saw errors in them. However, as I later realized, that errors were triggered deliberately from the client and were expected to occur (e.g. client do not provide any store on SSL port). So all seems to be fine on my side as well.

          I have uploaded a client's store with untrusted cert for the extended tests. I have also uploaded these tests in the new patch, in particular:

          • testMultipleStoreClientAuthentication tests also that if client presents this untrusted cert, then validation fails
          • I have added peer store configuration into the config-systests.json, so when the test broker is setup, the QpidMultipleTrustManager will be loaded also with the peer store (so implicitly used/tested by all current tests)
          • ExternalAuthenticationTest was extended for testUntrustedExternalAuthenticationManagerOnSSLPort. It simply tests the broker with untrusted client (expected to fail)

          Let me know if it seems fine from your side.

          Thank you,

          Michal

          Show
          Michal Zerola added a comment - Hi Robbie, I am glad for the patch. The 'problems' I have seen with ExternalAuthenticationTest were just my misinterpretation of the *.out files, since I saw errors in them. However, as I later realized, that errors were triggered deliberately from the client and were expected to occur (e.g. client do not provide any store on SSL port). So all seems to be fine on my side as well. I have uploaded a client's store with untrusted cert for the extended tests. I have also uploaded these tests in the new patch, in particular: testMultipleStoreClientAuthentication tests also that if client presents this untrusted cert, then validation fails I have added peer store configuration into the config-systests.json, so when the test broker is setup, the QpidMultipleTrustManager will be loaded also with the peer store (so implicitly used/tested by all current tests) ExternalAuthenticationTest was extended for testUntrustedExternalAuthenticationManagerOnSSLPort. It simply tests the broker with untrusted client (expected to fail) Let me know if it seems fine from your side. Thank you, Michal
          Hide
          Robbie Gemmell added a comment -

          Hi Michal,

          I didn't manage to get a chance to look over the patch today as I was finishing up some things before heading for next week off. I'll take a look over the next few days before going into hiding for the rest of the time

          Robbie

          Show
          Robbie Gemmell added a comment - Hi Michal, I didn't manage to get a chance to look over the patch today as I was finishing up some things before heading for next week off. I'll take a look over the next few days before going into hiding for the rest of the time Robbie
          Hide
          Robbie Gemmell added a comment -

          Hi Michal,

          I took a look over the patch, and ended up deciding to expand on it a bit to make the testing a bit more granular.

          For the unit tests, I removed the 'app2' cert from the test broker peer store so it only contains the 'app1' cert, allowing to test behaviour of the QpidPeersOnlyTrustManager with a cert it wouldnt trust but which the broker normally would otherwise (since the CA is in the broker trust store). I also added some tests to ensure behaviour of the QpidMultipleTrustManager is as expected when used different combinations of delegate TrustManagers.

          For the systests, I removed the peer store from config-systests.json so that the existing tests did not use it. Running with only a TrustStore configured actually follows a slightly different codepath at times and so we should really keep tests which only use a TrustStore to ensure that path is also tested. When I did that I noticed that the systest you had added didnt actually fail as I would have expected, so I altered it slightly to use the new 'unustrusted' client keystore as the broker peerstore and showed this allowed clients using that cert to connect, adding a further test to show that if the peerstore wasnt in place that the client would fail to connect (thus demonstrating the effectiveness of adding the peerstore).

          The changes are at http://svn.apache.org/viewvc?view=revision&revision=1457599 if you would like to look them over.

          Robbie

          Show
          Robbie Gemmell added a comment - Hi Michal, I took a look over the patch, and ended up deciding to expand on it a bit to make the testing a bit more granular. For the unit tests, I removed the 'app2' cert from the test broker peer store so it only contains the 'app1' cert, allowing to test behaviour of the QpidPeersOnlyTrustManager with a cert it wouldnt trust but which the broker normally would otherwise (since the CA is in the broker trust store). I also added some tests to ensure behaviour of the QpidMultipleTrustManager is as expected when used different combinations of delegate TrustManagers. For the systests, I removed the peer store from config-systests.json so that the existing tests did not use it. Running with only a TrustStore configured actually follows a slightly different codepath at times and so we should really keep tests which only use a TrustStore to ensure that path is also tested. When I did that I noticed that the systest you had added didnt actually fail as I would have expected, so I altered it slightly to use the new 'unustrusted' client keystore as the broker peerstore and showed this allowed clients using that cert to connect, adding a further test to show that if the peerstore wasnt in place that the client would fail to connect (thus demonstrating the effectiveness of adding the peerstore). The changes are at http://svn.apache.org/viewvc?view=revision&revision=1457599 if you would like to look them over. Robbie
          Hide
          Michal Zerola added a comment -

          Hi Robbie,

          sorry for the late answer - I was on the vacation. I think your last changes look good, thank you for applying patches.

          Michal

          Show
          Michal Zerola added a comment - Hi Robbie, sorry for the late answer - I was on the vacation. I think your last changes look good, thank you for applying patches. Michal
          Hide
          Robbie Gemmell added a comment -

          As per the added of the linked issue, QPID-4739 modifies the SSL configuration to allow defining multiple trust stores and assigning them per-port. The 'peer store' functionality is incorporated into this using the 'peersOnly' attribute on each trust store.

          Show
          Robbie Gemmell added a comment - As per the added of the linked issue, QPID-4739 modifies the SSL configuration to allow defining multiple trust stores and assigning them per-port. The 'peer store' functionality is incorporated into this using the 'peersOnly' attribute on each trust store.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development