ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-2014

Only admin should be allowed to reconfig a cluster

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Blocker Blocker
    • Resolution: Unresolved
    • Affects Version/s: 3.5.0
    • Fix Version/s: None
    • Component/s: server
    • Labels:
      None

      Description

      ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We should, at the very least, ensure that only the Admin can reconfigure a cluster. Perhaps restricting access to /zookeeper/config as well, though this is debatable. Surely one could ensure Admin only access via an ACL, but that would leave everyone who doesn't use ACLs unprotected. We could also force a default ACL to make it a bit more consistent (maybe).

      Finally, making reconfig() only available to Admins means they have to run with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure if everyone does, or how would it work with other authentication providers).

      1. ZOOKEEPER-2014.patch
        2 kB
        Raul Gutierrez Segales

        Issue Links

          Activity

          Hide
          Alexander Shraer added a comment -

          Everything you guys are saying about admin-only controls sounds very reasonable. I just want to clarify about the special reconfig znode. IMHO we should not allow write permissions to this node. I don't even see why an admin should have it its set only through the reconfig API.

          I do think that all clients should have read permissions, and here's why - the information they get from this znode is the up-to-date connection string. When the configuration changes this is the bare minimum they need in order not to loose track of the system. When their server crashes they need to know whom to connect to next. The new connection string is exactly the information we exploit for rebalancing. It is even implemented inside the updateServerList method, which is also needed in any case.

          Regarding my suggestion - Hongchao opened a JIRA for it and you can read the discussion there https://issues.apache.org/jira/browse/ZOOKEEPER-2016
          Please especially see Marshall's comment.
          All I'm proposing there is to implement some default behaviour that will save most clients from setting a watch and invoking updateServerList - I suggest that the client-side-library does it for them if they opt-in for the default behaviour. It doesn't change APIs, just adds one more feature, so it doesn't delay 3.5.0.

          Show
          Alexander Shraer added a comment - Everything you guys are saying about admin-only controls sounds very reasonable. I just want to clarify about the special reconfig znode. IMHO we should not allow write permissions to this node. I don't even see why an admin should have it its set only through the reconfig API. I do think that all clients should have read permissions, and here's why - the information they get from this znode is the up-to-date connection string. When the configuration changes this is the bare minimum they need in order not to loose track of the system. When their server crashes they need to know whom to connect to next. The new connection string is exactly the information we exploit for rebalancing. It is even implemented inside the updateServerList method, which is also needed in any case. Regarding my suggestion - Hongchao opened a JIRA for it and you can read the discussion there https://issues.apache.org/jira/browse/ZOOKEEPER-2016 Please especially see Marshall's comment. All I'm proposing there is to implement some default behaviour that will save most clients from setting a watch and invoking updateServerList - I suggest that the client-side-library does it for them if they opt-in for the default behaviour. It doesn't change APIs, just adds one more feature, so it doesn't delay 3.5.0.
          Hide
          Patrick Hunt added a comment -

          Yes, this is a concern for me as well.

          1) mixing the client api and the admin api is not great. It would be better to have them separate. We should fix this asap.

          2) this (controlling access to reconfig) is a big issue from a security perspective IMO.

          A few comments on the comments so far:

          ensure that only the Admin can reconfigure a cluster

          sounds sensible to me

          Perhaps restricting access to /zookeeper/config as well

          in the past we've (ben in particular) tried to limit the amount of information we provide to the client/session. For example we don't tell them which server they are connected to. I see this in the same vein.

          one could ensure Admin only access via an ACL, but that would leave everyone who doesn't use ACLs unprotected.

          well, you're already unprotected in this situation so I don't really see it as a sticking point.

          clients may need read access for example in order to run the new client-side load balancing functionality

          perhaps this argues for pushing this to the the server? encapsulate the information on the service I mean and expose as a specific api.

          Actually perhaps we should open a JIRA to hide the client-side rebalancing from clients (not for 3.5.0).

          yes, that's what I was trying to get at in the previous item in this comment. Although I think we'd need to fix this now - while we can still change the apis w/o worrying about b/w compat (we can change new apis during the alpha period w/o worrying about back compat)

          should this be similar to how updating the quota znode works ? or do you think changing configuration is different ?

          if quotas are "admin" functions we probably need to fix those as well - lock them down to just admin level authz I mean.

          Show
          Patrick Hunt added a comment - Yes, this is a concern for me as well. 1) mixing the client api and the admin api is not great. It would be better to have them separate. We should fix this asap. 2) this (controlling access to reconfig) is a big issue from a security perspective IMO. A few comments on the comments so far: ensure that only the Admin can reconfigure a cluster sounds sensible to me Perhaps restricting access to /zookeeper/config as well in the past we've (ben in particular) tried to limit the amount of information we provide to the client/session. For example we don't tell them which server they are connected to. I see this in the same vein. one could ensure Admin only access via an ACL, but that would leave everyone who doesn't use ACLs unprotected. well, you're already unprotected in this situation so I don't really see it as a sticking point. clients may need read access for example in order to run the new client-side load balancing functionality perhaps this argues for pushing this to the the server? encapsulate the information on the service I mean and expose as a specific api. Actually perhaps we should open a JIRA to hide the client-side rebalancing from clients (not for 3.5.0). yes, that's what I was trying to get at in the previous item in this comment. Although I think we'd need to fix this now - while we can still change the apis w/o worrying about b/w compat (we can change new apis during the alpha period w/o worrying about back compat) should this be similar to how updating the quota znode works ? or do you think changing configuration is different ? if quotas are "admin" functions we probably need to fix those as well - lock them down to just admin level authz I mean.
          Hide
          Raul Gutierrez Segales added a comment -

          (by which I mean, you can really bring a cluster down with this).

          Show
          Raul Gutierrez Segales added a comment - (by which I mean, you can really bring a cluster down with this).
          Hide
          Raul Gutierrez Segales added a comment -

          Alexander Shraer: well, quotas rely on ACLs. But, quotas are (currently) advisory only (i.e.: we don't reject traffic) so no damage can be done by non-admins. So I think we need a stricter enforcement here, no?

          Show
          Raul Gutierrez Segales added a comment - Alexander Shraer : well, quotas rely on ACLs. But, quotas are (currently) advisory only (i.e.: we don't reject traffic) so no damage can be done by non-admins. So I think we need a stricter enforcement here, no?
          Hide
          Alexander Shraer added a comment -

          Raul Gutierrez Segales, should this be similar to how updating the quota znode works ? or do you think changing configuration is different ?

          Show
          Alexander Shraer added a comment - Raul Gutierrez Segales , should this be similar to how updating the quota znode works ? or do you think changing configuration is different ?
          Hide
          Alexander Shraer added a comment -
          Show
          Alexander Shraer added a comment - + Flavio Junqueira , Michi Mutsuzaki
          Hide
          Alexander Shraer added a comment -

          Actually perhaps we should open a JIRA to hide the client-side rebalancing from clients (not for 3.5.0). We may want to implement it inside the client-side library somehow and just let the client specify whether this is enabled or disabled when creating a ZK handle. What do you think ?

          Show
          Alexander Shraer added a comment - Actually perhaps we should open a JIRA to hide the client-side rebalancing from clients (not for 3.5.0). We may want to implement it inside the client-side library somehow and just let the client specify whether this is enabled or disabled when creating a ZK handle. What do you think ?
          Hide
          Alexander Shraer added a comment -

          Thanks for starting this thread, Raul. I'm not sure what's the right way to handle the issue, but just wanted to mention a couple of things.

          1. Regarding access to /zookeeper/config - clients may need read access for example in order to run the new client-side load balancing functionality, they need to detect a config change and get a new connection string. They can use getConfig for this. It is probably a good idea to restrict access for writes. Writing it probably won't break the system but may cause all clients to migrate to any server I choose to mention there, if clients are using the client-side load balancing algorithm.

          2. Notice that when the leader processes reconfig in PrepRequestProcessor it already checks ACL:

          nodeRecord = getRecordForPath(ZooDefs.CONFIG_NODE);
          checkACL(zks, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo);

          Show
          Alexander Shraer added a comment - Thanks for starting this thread, Raul. I'm not sure what's the right way to handle the issue, but just wanted to mention a couple of things. 1. Regarding access to /zookeeper/config - clients may need read access for example in order to run the new client-side load balancing functionality, they need to detect a config change and get a new connection string. They can use getConfig for this. It is probably a good idea to restrict access for writes. Writing it probably won't break the system but may cause all clients to migrate to any server I choose to mention there, if clients are using the client-side load balancing algorithm. 2. Notice that when the leader processes reconfig in PrepRequestProcessor it already checks ACL: nodeRecord = getRecordForPath(ZooDefs.CONFIG_NODE); checkACL(zks, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo);
          Hide
          Hongchao Deng added a comment -

          ensure that only the Admin can reconfigure a cluster.

          This change will reduce much flexibility in reconfig.
          A counter scenario would be I have a process that detects "permanent" failed ZK servers and removes them to make a smaller quorum (better fault tolerance). Does the process have to be Admin? Or would a default ACL be a better option here?

          Show
          Hongchao Deng added a comment - ensure that only the Admin can reconfigure a cluster. This change will reduce much flexibility in reconfig. A counter scenario would be I have a process that detects "permanent" failed ZK servers and removes them to make a smaller quorum (better fault tolerance). Does the process have to be Admin? Or would a default ACL be a better option here?
          Hide
          Raul Gutierrez Segales added a comment -

          this is RFC and very basic.

          cc: Alexander Shraer, Patrick Hunt, Hongchao Deng

          Show
          Raul Gutierrez Segales added a comment - this is RFC and very basic. cc: Alexander Shraer , Patrick Hunt , Hongchao Deng

            People

            • Assignee:
              Raul Gutierrez Segales
              Reporter:
              Raul Gutierrez Segales
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development