Qpid
  1. Qpid
  2. QPID-4334

[Java broker] move the Firewall functionality into the ACL plugin

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.19
    • Component/s: Java Broker
    • Labels:
      None

      Description

      Firewall rules are currently expressed separately from ACLs, but this plugin is effectively an access control plugin and it is felt that the functionality would thus be better expressed using ACL rules. Such request has been made in the past, including a patch for the C++ broker as in use by a user.

      The firewall functionality will be moved into the ACL plugin, and the firewall plugin removed.

        Activity

        Hide
        Keith Wall added a comment -

        Patch applied. No comments.

        Show
        Keith Wall added a comment - Patch applied. No comments.
        Hide
        Philip Harvey added a comment -

        attached docbook patch

        Show
        Philip Harvey added a comment - attached docbook patch
        Hide
        Philip Harvey added a comment -

        attached docbook patch

        Show
        Philip Harvey added a comment - attached docbook patch
        Hide
        Keith Wall added a comment -

        Patch applied. Reassigning back to Phil for the completion of the docs.

        Show
        Keith Wall added a comment - Patch applied. Reassigning back to Phil for the completion of the docs.
        Hide
        Keith Wall added a comment -

        Patch applied. Reassigning back to Phil for the completion of the docs.

        Show
        Keith Wall added a comment - Patch applied. Reassigning back to Phil for the completion of the docs.
        Hide
        Philip Harvey added a comment -

        Thanks for the review comments Keith. Here is a further patch that addresses your feedback.

        Show
        Philip Harvey added a comment - Thanks for the review comments Keith. Here is a further patch that addresses your feedback.
        Hide
        Philip Harvey added a comment -

        attached docbook patch

        Show
        Philip Harvey added a comment - attached docbook patch
        Hide
        Keith Wall added a comment -

        Hi Phil

        Looks good, I just have a couple of comments, I think most of which are actually against the original implementation.

        AclRulePredicates.java
        64 - Guard debug logging

        Action.java
        77 - public setOperation, setObjectType, setProperties could be private and members final
        107 - comment re compareTo seems spurious
        112 - the refactoring means we now always evaluate operation, objectType and properties rather than potentially short circuiting earlier. Also could properties ever end up being null?

        PlainConfiguration.jav
        75 - load should be closing the underlying reader

        HostnameFirewallRule.java
        68 - Add the IP that failed to lookup to the text of the AccessControlFirewallException
        100 - I think we should be logging at warn the case where rDNS lookups are failing/timing out. I worry that a sporadic DNS failure might lead to mysterious Java Broker defect reports.

        
        public String call()
        {
          boolean success = false;
          try 
          {
           String hn =  remote.getCanonicalHostName();
           success = true;
           return hn;
          }
          finally
          {
             if (!success)
             { 
                log.warn("Failed to get canonical for " + ip + ", DNS timeout or error.");.
             }
          } 
        }
        

        InetAddress.java
        100 - confusing braces
        177 - seemingly unnecessary use of reflection. InetAddress.getByAddress has been part of the API since 1.4

        AccessControlFirewallException.java/InetNetwork.java
        code style (brace positions)

        Show
        Keith Wall added a comment - Hi Phil Looks good, I just have a couple of comments, I think most of which are actually against the original implementation. AclRulePredicates.java 64 - Guard debug logging Action.java 77 - public setOperation, setObjectType, setProperties could be private and members final 107 - comment re compareTo seems spurious 112 - the refactoring means we now always evaluate operation, objectType and properties rather than potentially short circuiting earlier. Also could properties ever end up being null? PlainConfiguration.jav 75 - load should be closing the underlying reader HostnameFirewallRule.java 68 - Add the IP that failed to lookup to the text of the AccessControlFirewallException 100 - I think we should be logging at warn the case where rDNS lookups are failing/timing out. I worry that a sporadic DNS failure might lead to mysterious Java Broker defect reports. public String call() { boolean success = false ; try { String hn = remote.getCanonicalHostName(); success = true ; return hn; } finally { if (!success) { log.warn( "Failed to get canonical for " + ip + ", DNS timeout or error." );. } } } InetAddress.java 100 - confusing braces 177 - seemingly unnecessary use of reflection. InetAddress.getByAddress has been part of the API since 1.4 AccessControlFirewallException.java/InetNetwork.java code style (brace positions)
        Hide
        Philip Harvey added a comment -

        attached docbook patch (not yet complete)

        Show
        Philip Harvey added a comment - attached docbook patch (not yet complete)
        Hide
        Philip Harvey added a comment -

        Attached patch for review.

        Note that the corresponding docbook updates are outstanding. I will attach a patch for these separately.

        Show
        Philip Harvey added a comment - Attached patch for review. Note that the corresponding docbook updates are outstanding. I will attach a patch for these separately.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development