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

        Robbie Gemmell created issue -
        Philip Harvey made changes -
        Field Original Value New Value
        Assignee Philip Harvey [ philharveyonline ]
        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.
        Philip Harvey made changes -
        Philip Harvey made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Keith Wall made changes -
        Status In Progress [ 3 ] Ready To Review [ 10006 ]
        Keith Wall made changes -
        Assignee Philip Harvey [ philharveyonline ] Keith Wall [ k-wall ]
        Hide
        Philip Harvey added a comment -

        attached docbook patch (not yet complete)

        Show
        Philip Harvey added a comment - attached docbook patch (not yet complete)
        Philip Harvey made changes -
        Attachment 0001-QPID-4334-updated-the-Java-ACL-docbook-to-mention-it.patch [ 12546970 ]
        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)
        Philip Harvey made changes -
        Attachment 0001-QPID-4334-updated-the-Java-ACL-docbook-to-mention-it.patch [ 12546970 ]
        Hide
        Philip Harvey added a comment -

        attached docbook patch

        Show
        Philip Harvey added a comment - attached docbook patch
        Philip Harvey made changes -
        Attachment 0001-QPID-4334-updated-the-Java-ACL-docbook-to-mention-it.patch [ 12546974 ]
        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.
        Philip Harvey made changes -
        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.
        Keith Wall made changes -
        Assignee Keith Wall [ k-wall ] Philip Harvey [ philharveyonline ]
        Keith Wall made changes -
        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.
        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.
        Philip Harvey made changes -
        Attachment 0001-QPID-4334-updated-the-Java-ACL-docbook-to-mention-it.patch [ 12546974 ]
        Hide
        Philip Harvey added a comment -

        attached docbook patch

        Show
        Philip Harvey added a comment - attached docbook patch
        Philip Harvey made changes -
        Attachment 0001-QPID-4334-updated-the-Java-ACL-docbook-to-mention-it.patch [ 12546991 ]
        Philip Harvey made changes -
        Attachment 0001-QPID-4334-updated-the-Java-ACL-docbook-to-mention-it.patch [ 12546991 ]
        Hide
        Philip Harvey added a comment -

        attached docbook patch

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

        Patch applied. No comments.

        Show
        Keith Wall added a comment - Patch applied. No comments.
        Keith Wall made changes -
        Status Ready To Review [ 10006 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Keith Wall made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development