ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1759

Adding ability to allow READ operations for authenticated users, versus keeping ACLs wide open for READ

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.4.5
    • Fix Version/s: 3.5.0
    • Component/s: server
    • Labels:
      None
    • Environment:

      Java, SASL authentication, security

      Description

      Today when using SASLAuthenticationProvider to authenticate Zookeeper Clients access to the data based on ACLS set on znodes there is no other choice but to set READ ACLs to be "world", "anyone" with the way how

      public boolean matches(String id,String aclExpr)
      

      is currently implemented. It means that any unauthenticated user can read the data when application needs to make sure that not only creator of a znode can read the content.
      Proposal is to introduce new property: "zookeeper.readUser" that if incoming id matches to the value of that property it will be allowed to proceed in "match" method.
      So creator of a znode instead of

      ACL acl1 = new ACL(Perms.ADMIN | Perms.CREATE | Perms.WRITE | Perms.DELETE, Ids.AUTH_IDS);
      ACL acl2 = new ACL(Perms.READ, Ids.ANYONE_ID_UNSAFE);
      

      will need to do

      ACL acl1 = new ACL(Perms.ADMIN | Perms.CREATE | Perms.WRITE | Perms.DELETE, Ids.AUTH_IDS);
      ACL acl2 = new ACL(Perms.READ, new Id("sasl", "anyone"));
      

      Assuming that value of "zookeeper.readUser" property was "anyone".
      This way at least READ access on corresponding znode has to be authenticated.

      1. TEST-org.apache.zookeeper.test.SaslAuthDesignatedClientTest.txt
        39 kB
        Flavio Junqueira
      2. ZOOKEEPER-1759.patch
        4 kB
        Yuliya Feldman
      3. ZOOKEEPER-1759.patch
        3 kB
        Yuliya Feldman
      4. ZOOKEEPER-1759.patch
        4 kB
        Yuliya Feldman
      5. ZOOKEEPER-1759.patch
        4 kB
        Yuliya Feldman
      6. ZOOKEEPER-1759-1.patch
        2 kB
        Yuliya Feldman
      7. ZOOKEEPER-1759-1.patch
        1 kB
        Yuliya Feldman

        Activity

        Hide
        Yuliya Feldman added a comment -
        Show
        Yuliya Feldman added a comment - ZOOKEEPER-1759 .patch
        Hide
        Yuliya Feldman added a comment -

        Latest

        Show
        Yuliya Feldman added a comment - Latest
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12603932/ZOOKEEPER-1759.patch
        against trunk revision 1524398.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1589//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12603932/ZOOKEEPER-1759.patch against trunk revision 1524398. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1589//console This message is automatically generated.
        Hide
        Camille Fournier added a comment -

        Patch failed, can you try syncing with head and submitting the patch again?

        Show
        Camille Fournier added a comment - Patch failed, can you try syncing with head and submitting the patch again?
        Hide
        Yuliya Feldman added a comment -

        Sure. I thought I did, but apparently it did not work.

        Show
        Yuliya Feldman added a comment - Sure. I thought I did, but apparently it did not work.
        Hide
        Yuliya Feldman added a comment -

        Resubmitted svn patch instead of git one. Hopefully this will work.

        Show
        Yuliya Feldman added a comment - Resubmitted svn patch instead of git one. Hopefully this will work.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12603960/ZOOKEEPER-1759.patch
        against trunk revision 1524398.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1590//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1590//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1590//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12603960/ZOOKEEPER-1759.patch against trunk revision 1524398. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1590//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1590//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1590//console This message is automatically generated.
        Hide
        Yuliya Feldman added a comment -

        I don't quite understand how this failure is related to the patch: /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-Build/trunk/src/c/tests/TestReconfig.cc:488: Assertion: assertion failed [Expression: found != string::npos]

        Show
        Yuliya Feldman added a comment - I don't quite understand how this failure is related to the patch: /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-Build/trunk/src/c/tests/TestReconfig.cc:488: Assertion: assertion failed [Expression: found != string::npos]
        Hide
        Camille Fournier added a comment -

        Let me see what's changed recently...

        Show
        Camille Fournier added a comment - Let me see what's changed recently...
        Hide
        Camille Fournier added a comment -

        Nothing that should obviously cause this. I will look at the patch soon and CR it, at a glance it looks ok to me.

        Show
        Camille Fournier added a comment - Nothing that should obviously cause this. I will look at the patch soon and CR it, at a glance it looks ok to me.
        Hide
        Yuliya Feldman added a comment -

        Is anything else needs to be done on my side to have this patch committed?

        Show
        Yuliya Feldman added a comment - Is anything else needs to be done on my side to have this patch committed?
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12603960/ZOOKEEPER-1759.patch
        against trunk revision 1524398.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1596//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1596//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1596//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12603960/ZOOKEEPER-1759.patch against trunk revision 1524398. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1596//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1596//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1596//console This message is automatically generated.
        Hide
        Camille Fournier added a comment -

        I think this looks ok but I wish someone that used SASL would comment on whether this makes sense to do via a config paramter. Eugene Koontz any comment from you?

        Show
        Camille Fournier added a comment - I think this looks ok but I wish someone that used SASL would comment on whether this makes sense to do via a config paramter. Eugene Koontz any comment from you?
        Hide
        Eugene Koontz added a comment -

        Hi Camille, thanks for bringing this to my attention. Yuliya, two questions:

        1) I don't think the property name "zookeeper.readUser" is meaningful - in this code that you added to matches():

                String readAccessUser = System.getProperty("zookeeper.readUser");
                if ( readAccessUser != null && aclExpr.equals(readAccessUser)) {
                  return true;
                }
        

        Above, there is no a check for whether the user wants to specifically read as opposed to any other action.

        For example, if a) and b) are true:

        a) I add an ACL: ((Perms.READ | Perms.WRITE), new Id("sasl", "anyone"))

        and

        b) the property "zookeeper.readUser" is set to "anyone"

        then this user can read and write to the node. So it seems like you could call the property "zookeeper.x-User" just as well: it's the ACL on the node, not the property, that determines what set of actions x that the user defined by this property can do.

        2. I'm not sure what this change adds any new authorization restrictions - it's seems the same as simply making a node world-readable. What if a user is not SASL-authenticated? Won't the new code that you added in matches():

                String readAccessUser = System.getProperty("zookeeper.readUser");
                if ( readAccessUser != null && aclExpr.equals(readAccessUser)) {
                  return true;
                }
        

        simply return true regardless of whether the client is SASL-authenticated or not, if a given node is set to ACL(Perms.READ, new Id("sasl", "anyone"), and zookeeper.readUser is set to "anyone"?

        I might be wrong - but either way, the question could be resolved with an additional unit test, which clarifies what the permissions are of a non-SASL-authenticated user when the user attempts to read a node which has:

        a) ACL(Perms.READ, new Id("sasl", "anyone")
        b) has no other permissions (e.g. not world-readable).

        -Eugene

        Show
        Eugene Koontz added a comment - Hi Camille, thanks for bringing this to my attention. Yuliya, two questions: 1) I don't think the property name "zookeeper.readUser" is meaningful - in this code that you added to matches(): String readAccessUser = System .getProperty( "zookeeper.readUser" ); if ( readAccessUser != null && aclExpr.equals(readAccessUser)) { return true ; } Above, there is no a check for whether the user wants to specifically read as opposed to any other action. For example, if a) and b) are true: a) I add an ACL: ((Perms.READ | Perms.WRITE), new Id("sasl", "anyone")) and b) the property "zookeeper.readUser" is set to "anyone" then this user can read and write to the node. So it seems like you could call the property "zookeeper.x-User" just as well: it's the ACL on the node, not the property, that determines what set of actions x that the user defined by this property can do. 2. I'm not sure what this change adds any new authorization restrictions - it's seems the same as simply making a node world-readable. What if a user is not SASL-authenticated? Won't the new code that you added in matches(): String readAccessUser = System .getProperty( "zookeeper.readUser" ); if ( readAccessUser != null && aclExpr.equals(readAccessUser)) { return true ; } simply return true regardless of whether the client is SASL-authenticated or not, if a given node is set to ACL(Perms.READ, new Id("sasl", "anyone"), and zookeeper.readUser is set to "anyone"? I might be wrong - but either way, the question could be resolved with an additional unit test, which clarifies what the permissions are of a non-SASL-authenticated user when the user attempts to read a node which has: a) ACL(Perms.READ, new Id("sasl", "anyone") b) has no other permissions (e.g. not world-readable). -Eugene
        Hide
        Yuliya Feldman added a comment -

        Thank you Eugene for your comments
        Regarding #1 - I agree that name "readUser" may not be a good one - because it is up to the ACL creator to "expose" particular operation.

        Regarding #2 - It does add authentication restrictions, not authorization ones. Today only superUser can do whatever with znode that has restricted ACLs. If I put "world" "anyone" for readable - no Authentication will be involved at all. Here is the snippet from PrepRequestProcessor:

                for (ACL a : acl) {
                    Id id = a.getId();
                    if ((a.getPerms() & perm) != 0) {
                        if (id.getScheme().equals("world")
                                && id.getId().equals("anyone")) {
                            return;
                        }
                        AuthenticationProvider ap = ProviderRegistry.getProvider(id
                                .getScheme());
                        if (ap != null) {
                            for (Id authId : ids) {                        
                                if (authId.getScheme().equals(id.getScheme())
                                        && ap.matches(authId.getId(), id.getId())) {
                                    return;
                                }
                            }
                        }
                    }
                }
        

        As you can see if it is "world" "anyone" - No authentication is checked at all.

        Regarding unit tests - I did add them - should be in the patch

        Show
        Yuliya Feldman added a comment - Thank you Eugene for your comments Regarding #1 - I agree that name "readUser" may not be a good one - because it is up to the ACL creator to "expose" particular operation. Regarding #2 - It does add authentication restrictions, not authorization ones. Today only superUser can do whatever with znode that has restricted ACLs. If I put "world" "anyone" for readable - no Authentication will be involved at all. Here is the snippet from PrepRequestProcessor: for (ACL a : acl) { Id id = a.getId(); if ((a.getPerms() & perm) != 0) { if (id.getScheme().equals( "world" ) && id.getId().equals( "anyone" )) { return ; } AuthenticationProvider ap = ProviderRegistry.getProvider(id .getScheme()); if (ap != null ) { for (Id authId : ids) { if (authId.getScheme().equals(id.getScheme()) && ap.matches(authId.getId(), id.getId())) { return ; } } } } } As you can see if it is "world" "anyone" - No authentication is checked at all. Regarding unit tests - I did add them - should be in the patch
        Hide
        Eugene Koontz added a comment -

        Hi Yuliya,
        Regarding #2, yes, I think you are right: a non-SASL-authenticated user will not have a authId for the SASL AuthenticationProvider, so the matches() will not be called, and the user will not be granted access, which is expected and correct behavior. So thanks for correcting me on this.

        I still think it would be good to test this feature with a non-SASL-authenticated user to test that this behavior is working as expected, however. Your test is good but only tests a SASL authenticated user.

        Show
        Eugene Koontz added a comment - Hi Yuliya, Regarding #2, yes, I think you are right: a non-SASL-authenticated user will not have a authId for the SASL AuthenticationProvider, so the matches() will not be called, and the user will not be granted access, which is expected and correct behavior. So thanks for correcting me on this. I still think it would be good to test this feature with a non-SASL-authenticated user to test that this behavior is working as expected, however. Your test is good but only tests a SASL authenticated user.
        Hide
        Yuliya Feldman added a comment -

        Sure. I will add test for non-SASL authenticated user.
        Thanks.

        Show
        Yuliya Feldman added a comment - Sure. I will add test for non-SASL authenticated user. Thanks.
        Hide
        Yuliya Feldman added a comment -

        I have updated the patch with additional test for non-SASL-authenticated user, or rather SASL unauthenticated client.
        Thanks.

        Show
        Yuliya Feldman added a comment - I have updated the patch with additional test for non-SASL-authenticated user, or rather SASL unauthenticated client. Thanks.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12604976/ZOOKEEPER-1759.patch
        against trunk revision 1524398.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1599//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1599//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1599//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12604976/ZOOKEEPER-1759.patch against trunk revision 1524398. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1599//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1599//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1599//console This message is automatically generated.
        Hide
        Eugene Koontz added a comment -

        Looks good to me, Yuliya. +1 (nonbinding).

        Show
        Eugene Koontz added a comment - Looks good to me, Yuliya. +1 (nonbinding).
        Hide
        Camille Fournier added a comment -

        Awesome, I will check this in when I have a few moments. Thanks Yuliya and Eugene!

        Show
        Camille Fournier added a comment - Awesome, I will check this in when I have a few moments. Thanks Yuliya and Eugene!
        Hide
        Eugene Koontz added a comment -

        Well, I forgot, there is the matter of the name of the property "zookeeper.readUser" as we discussed with regard to my question #1 above. "zookeeper.letAnySaslUserDoX" perhaps?

        Show
        Eugene Koontz added a comment - Well, I forgot, there is the matter of the name of the property "zookeeper.readUser" as we discussed with regard to my question #1 above. "zookeeper.letAnySaslUserDoX" perhaps?
        Hide
        Camille Fournier added a comment -

        Oh yes, I checked this in but I agree with that. Also do we need documentation somewhere about this new property?

        Show
        Camille Fournier added a comment - Oh yes, I checked this in but I agree with that. Also do we need documentation somewhere about this new property?
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in ZooKeeper-trunk #2067 (See https://builds.apache.org/job/ZooKeeper-trunk/2067/)
        ZOOKEEPER-1759. Adding ability to allow READ operations for authenticated users,
        versus keeping ACLs wide open for READ. (Yuliya Feldman via camille) (camille: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1526219)

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java
        Show
        Hudson added a comment - SUCCESS: Integrated in ZooKeeper-trunk #2067 (See https://builds.apache.org/job/ZooKeeper-trunk/2067/ ) ZOOKEEPER-1759 . Adding ability to allow READ operations for authenticated users, versus keeping ACLs wide open for READ. (Yuliya Feldman via camille) (camille: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1526219 ) /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java
        Hide
        Flavio Junqueira added a comment -

        I'd like to ask you guys to do a config file property as well. Actually, I'd rather have the config file property, but if you want to have both, fine with me. If you need a reference, check how I have done it in ZOOKEEPER-1552.

        Show
        Flavio Junqueira added a comment - I'd like to ask you guys to do a config file property as well. Actually, I'd rather have the config file property, but if you want to have both, fine with me. If you need a reference, check how I have done it in ZOOKEEPER-1552 .
        Hide
        Yuliya Feldman added a comment -

        Sorry, Could not reply earlier - was out whole day.
        @Eugene - Since this one is committed should I submit another JIRA to change name of the property, or submit another patch on this JIRA?

        @Flavio I am not exactly following. In the JIRA you are referring to you were proposing to put property into QuorumConfig object, is it? I did not find anything the patch(es) that was related to configuration

        Show
        Yuliya Feldman added a comment - Sorry, Could not reply earlier - was out whole day. @Eugene - Since this one is committed should I submit another JIRA to change name of the property, or submit another patch on this JIRA? @Flavio I am not exactly following. In the JIRA you are referring to you were proposing to put property into QuorumConfig object, is it? I did not find anything the patch(es) that was related to configuration
        Hide
        Flavio Junqueira added a comment -

        Yuliya Feldman, Camille Fournier, SaslAuthDesignatedClientTest is failing and I suspect it is because of this commit. I'm going to upload a log file.

        With respect to configuration, I was indeed referring to adding a property to QuorumPeerConfig and have it exposed through QuorumPeer like we do with others. My sense is that some significant fraction of this community prefers to configure through a config file. If you still want to have a system property, then just use both. ZOOKEEPER-1552 is just an example in which I have used both.

        Show
        Flavio Junqueira added a comment - Yuliya Feldman , Camille Fournier , SaslAuthDesignatedClientTest is failing and I suspect it is because of this commit. I'm going to upload a log file. With respect to configuration, I was indeed referring to adding a property to QuorumPeerConfig and have it exposed through QuorumPeer like we do with others. My sense is that some significant fraction of this community prefers to configure through a config file. If you still want to have a system property, then just use both. ZOOKEEPER-1552 is just an example in which I have used both.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12605346/TEST-org.apache.zookeeper.test.SaslAuthDesignatedClientTest.txt
        against trunk revision 1526454.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 8 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1602//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12605346/TEST-org.apache.zookeeper.test.SaslAuthDesignatedClientTest.txt against trunk revision 1526454. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 8 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1602//console This message is automatically generated.
        Hide
        Yuliya Feldman added a comment -

        I am looking into failure

        Show
        Yuliya Feldman added a comment - I am looking into failure
        Hide
        Yuliya Feldman added a comment -

        I have retested it again and I don't see any test failure. Plus all trunk builds were successful after this check in. On top of it during znode creation code from this JIRA's patch is not even executed. Do you consistently see the failure or it one of?

        Show
        Yuliya Feldman added a comment - I have retested it again and I don't see any test failure. Plus all trunk builds were successful after this check in. On top of it during znode creation code from this JIRA's patch is not even executed. Do you consistently see the failure or it one of?
        Hide
        Yuliya Feldman added a comment -

        Ok, I was finally able to repro your failure - problem in tests ordering. Your order was different from others that caused failure. Since I was adding a test case on Eugene's request to test unauthenticated Client I set "zookeeper.sasl.client" to "false" and did not reenable it back at the end of the test. This caused Client from following test (testAuth) to be not authenticated and subsequently failing.
        I fixed test to set "zookeeper.sasl.client" to "true" at the end of my test and it should fix your issue. Will submit patch shortly.

        Show
        Yuliya Feldman added a comment - Ok, I was finally able to repro your failure - problem in tests ordering. Your order was different from others that caused failure. Since I was adding a test case on Eugene's request to test unauthenticated Client I set "zookeeper.sasl.client" to "false" and did not reenable it back at the end of the test. This caused Client from following test (testAuth) to be not authenticated and subsequently failing. I fixed test to set "zookeeper.sasl.client" to "true" at the end of my test and it should fix your issue. Will submit patch shortly.
        Hide
        Camille Fournier added a comment -

        Yuliya if you think you will have something soon just attach a new patch to this ticket that addresses these issues I'll put it in on the same ticket number. Otherwise I'll roll the whole thing back and put it all in when it's done.

        Show
        Camille Fournier added a comment - Yuliya if you think you will have something soon just attach a new patch to this ticket that addresses these issues I'll put it in on the same ticket number. Otherwise I'll roll the whole thing back and put it all in when it's done.
        Hide
        Yuliya Feldman added a comment -

        Additional to fix testx

        Show
        Yuliya Feldman added a comment - Additional to fix testx
        Hide
        Yuliya Feldman added a comment -

        Latest one that includes test fix and property name change

        Show
        Yuliya Feldman added a comment - Latest one that includes test fix and property name change
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12605395/ZOOKEEPER-1759-1.patch
        against trunk revision 1526454.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1603//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1603//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1603//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12605395/ZOOKEEPER-1759-1.patch against trunk revision 1526454. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1603//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1603//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1603//console This message is automatically generated.
        Hide
        Eugene Koontz added a comment -

        Yuliya, thanks for the explanation of the problem that Flavio found, your new patch looks good. It's good that the tests run in a random order; seems to shake out bugs that otherwise would not be seen.

        I'm not entirely satisfied with the new name "letAnySaslUserDoX", it sounds awkward, but it's good enough for now.

        Show
        Eugene Koontz added a comment - Yuliya, thanks for the explanation of the problem that Flavio found, your new patch looks good. It's good that the tests run in a random order; seems to shake out bugs that otherwise would not be seen. I'm not entirely satisfied with the new name "letAnySaslUserDoX", it sounds awkward, but it's good enough for now.
        Hide
        Hudson added a comment -

        FAILURE: Integrated in ZooKeeper-trunk #2072 (See https://builds.apache.org/job/ZooKeeper-trunk/2072/)
        ZOOKEEPER-1759. Adding ability to allow READ operations for authenticated users,
        versus keeping ACLs wide open for READ. (Yuliya Feldman via camille) (camille: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1527398)

        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java
        Show
        Hudson added a comment - FAILURE: Integrated in ZooKeeper-trunk #2072 (See https://builds.apache.org/job/ZooKeeper-trunk/2072/ ) ZOOKEEPER-1759 . Adding ability to allow READ operations for authenticated users, versus keeping ACLs wide open for READ. (Yuliya Feldman via camille) (camille: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1527398 ) /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java

          People

          • Assignee:
            Unassigned
            Reporter:
            Yuliya Feldman
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development