Qpid
  1. Qpid
  2. QPID-2360

declaring virtualhost level firewall configuration in virtualhosts.xml leads to NPE on startup

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.6
    • Fix Version/s: 0.7
    • Component/s: Java Broker
    • Labels:
      None

      Description

      Declaring virtualhost level firewall configuration in virtualhosts.xml leads to NPE on startup.

      After moving the same configuration into config.xml the broker then loads ok and the configuration works as expected.

      1. 0001-QPID-2630-QPID-2631-branch.patch
        127 kB
        Andrew Kennedy
      2. 0001-QPID-2630-QPID-2631-trunk.patch
        127 kB
        Andrew Kennedy

        Issue Links

          Activity

          Hide
          Andrew Kennedy added a comment -

          Patch for ServerConfiguration.java and ServerConfigurationTest.java that separates virtualhosts configuration mechanisms.

          Show
          Andrew Kennedy added a comment - Patch for ServerConfiguration.java and ServerConfigurationTest.java that separates virtualhosts configuration mechanisms.
          Hide
          Andrew Kennedy added a comment - - edited

          Patch also fixes QPID-2361 issue.

          Show
          Andrew Kennedy added a comment - - edited Patch also fixes QPID-2361 issue.
          Hide
          Andrew Kennedy added a comment -

          Patch fixes QPID-2360 and QPID-2361 by changing virtualhost s configuration mechanisms to be mutually exclusive. Patch includes changes to config files.

          Show
          Andrew Kennedy added a comment - Patch fixes QPID-2360 and QPID-2361 by changing virtualhost s configuration mechanisms to be mutually exclusive. Patch includes changes to config files.
          Hide
          Andrew Kennedy added a comment -

          Patches for system tests affected by virtualhosts configuration changes.

          Show
          Andrew Kennedy added a comment - Patches for system tests affected by virtualhosts configuration changes.
          Hide
          Andrew Kennedy added a comment -

          Patch for trunk, all files together, because 0.5 branch patches do not apply correctly.

          Show
          Andrew Kennedy added a comment - Patch for trunk, all files together, because 0.5 branch patches do not apply correctly.
          Hide
          Robbie Gemmell added a comment -

          The trunk patch does not compile because it also contains various other (unfinished?) changes to classes unrelated to the update of the
          server/virtualhost configuration and their tests, eg: reorganised imports, addition of generic types, spacing, annotations etc. MockAMQQueue changes also add a tab character that should be removed. It would be best to split these types of change and any others which aren't related to the configuration updates out into a new JIRA and patch, as it will make both a lot easier to digest.

          The changes to ServeConfiguration itself in order to address the issues raised look good, however there are some points to address with the changes:

          ServerConfigurationTest: L904 is now a function definition that does nothing other than call itself, and so gets stuck in an infinite recursion when used by the Firewall related test methods, leading to a StackOverFlowException when they are run.

          ServerConfiguration: L165 The exception should be updated to say "Only one external virtualhosts configuration file allowed, multiple filenames found" to avoid users thinking they have too many virtualhosts should the exception ever be thrown, rather than that they have just specified multiple strings that we then assume to be file locations.

          ServerConfiguration: L192 This comment has been made invalid by the patch changes and should be removed: "// Add the keys of the virtual host to the main config"

          I think that the broker/etc/log4j.xml configuration should not be modified to suppress warnings regarding optional config files not being present, with the associated logging level left as-is. Primarily only the tests use the ConfigurationFactory abilities and then the optional ability on top of that, so this will rarely if ever be logged in actual use, but it should be output when it is.

          broker/etc/transient_config.xml and broker/etc/persistent_config.xml should just be removed (they are not really used at all, at worst some tests use them and if so they should just be updated to use overrides where required.) rather than updating them and adding new individualised virtualhosts files for each and causing more clutter in the repository and Apache release bundles.

          Related to the above, the new virtualhost files referenced in the patch changes are not actually included in the patch and so are missing. Similarly for the main new broker/etc/virtualhosts.xml file and various new virtualhosts files for the Systests module.

          Show
          Robbie Gemmell added a comment - The trunk patch does not compile because it also contains various other (unfinished?) changes to classes unrelated to the update of the server/virtualhost configuration and their tests, eg: reorganised imports, addition of generic types, spacing, annotations etc. MockAMQQueue changes also add a tab character that should be removed. It would be best to split these types of change and any others which aren't related to the configuration updates out into a new JIRA and patch, as it will make both a lot easier to digest. The changes to ServeConfiguration itself in order to address the issues raised look good, however there are some points to address with the changes: ServerConfigurationTest: L904 is now a function definition that does nothing other than call itself, and so gets stuck in an infinite recursion when used by the Firewall related test methods, leading to a StackOverFlowException when they are run. ServerConfiguration: L165 The exception should be updated to say "Only one external virtualhosts configuration file allowed, multiple filenames found " to avoid users thinking they have too many virtualhosts should the exception ever be thrown, rather than that they have just specified multiple strings that we then assume to be file locations. ServerConfiguration: L192 This comment has been made invalid by the patch changes and should be removed: "// Add the keys of the virtual host to the main config" I think that the broker/etc/log4j.xml configuration should not be modified to suppress warnings regarding optional config files not being present, with the associated logging level left as-is. Primarily only the tests use the ConfigurationFactory abilities and then the optional ability on top of that, so this will rarely if ever be logged in actual use, but it should be output when it is. broker/etc/transient_config.xml and broker/etc/persistent_config.xml should just be removed (they are not really used at all, at worst some tests use them and if so they should just be updated to use overrides where required.) rather than updating them and adding new individualised virtualhosts files for each and causing more clutter in the repository and Apache release bundles. Related to the above, the new virtualhost files referenced in the patch changes are not actually included in the patch and so are missing. Similarly for the main new broker/etc/virtualhosts.xml file and various new virtualhosts files for the Systests module.
          Hide
          Andrew Kennedy added a comment -

          Updated patches for branch and trunk, with changes as suggested.

          Show
          Andrew Kennedy added a comment - Updated patches for branch and trunk, with changes as suggested.
          Hide
          Robbie Gemmell added a comment -

          The configuration changes dont fully account for the Security configuration, particularly the ability to reload the security configuration at runtime (eg to update the Firewall configuration). ServerConfiguration.reparseConfigFIleSecuritySections() expects the virtualhost configuration properties to be part of the main configuration, which is not the case now when an external virtualhosts configuration file is used.

          As such this needs updated to determine whether an external file was used to configure the virtualhost such that the configuration sections are properly reparsed. This is not detected by the firewall related unit/system tests in ServerConfiguration and FirewallConfigTest, as some utilise only broker level security configuration, and the others utilise virtualhost level configuration but when the main config is reparsed and the virtualhosts file isnt, it will return an empty subset of virtualhost security and the firewall will then deny by default, which is what the tests expect to occur and so no problem is found.

          The updated (trunk) patch contains a number of .orig files, presumably from 'unclean' patch runs. It also still contains some seemingly unrelated modifications (eg in AlertingTest, which now fails when run). If these are unrelated but still required changes, a new JIRA should be raised for them to allow proper tracking of the issue.

          Show
          Robbie Gemmell added a comment - The configuration changes dont fully account for the Security configuration, particularly the ability to reload the security configuration at runtime (eg to update the Firewall configuration). ServerConfiguration.reparseConfigFIleSecuritySections() expects the virtualhost configuration properties to be part of the main configuration, which is not the case now when an external virtualhosts configuration file is used. As such this needs updated to determine whether an external file was used to configure the virtualhost such that the configuration sections are properly reparsed. This is not detected by the firewall related unit/system tests in ServerConfiguration and FirewallConfigTest, as some utilise only broker level security configuration, and the others utilise virtualhost level configuration but when the main config is reparsed and the virtualhosts file isnt, it will return an empty subset of virtualhost security and the firewall will then deny by default, which is what the tests expect to occur and so no problem is found. The updated (trunk) patch contains a number of .orig files, presumably from 'unclean' patch runs. It also still contains some seemingly unrelated modifications (eg in AlertingTest, which now fails when run). If these are unrelated but still required changes, a new JIRA should be raised for them to allow proper tracking of the issue.
          Hide
          Andrew Kennedy added a comment -

          Updated patches

          Show
          Andrew Kennedy added a comment - Updated patches
          Hide
          Andrew Kennedy added a comment -

          Removed unused tests

          Show
          Andrew Kennedy added a comment - Removed unused tests
          Hide
          Robbie Gemmell added a comment -

          Patches commited

          Show
          Robbie Gemmell added a comment - Patches commited
          Hide
          Robbie Gemmell added a comment -

          Patches were commited in revisions 929136 and 929138. The commit log statement was incorrect at the time, but has since been updated with the correct issue numbers.

          Show
          Robbie Gemmell added a comment - Patches were commited in revisions 929136 and 929138. The commit log statement was incorrect at the time, but has since been updated with the correct issue numbers.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development