[STORM-2627] The annotation of storm.zookeeper.topology.auth.scheme in Config.java is wrong #2208
Conversation
|
@HeartSaVioR |
|
Can one of the admins verify this patch? |
|
@haitaoyao |
|
@liu-zhaokun I think the comment there meant to say by default it will be "No Authentication". I.e Its users responsibility to set to digest in a secure clusters. But since the default settings for non-secure the comment looks ok to me. |
|
@harshach |
|
I think the best way to avoid misleading is just removing the configuration and set the value as constant. We don't need to provide an option to user while the value is actually fixed. |
|
@HeartSaVioR |
liu-zhaokun
force-pushed
the
master07131533
branch
2 times, most recently
from
d374644
to
d8cd89e
Compare
|
@HeartSaVioR |
|
@HeartSaVioR wouldn't that be an issue incase of non-secure cluster if we are defaulting to "digest"? |
|
@harshach |
|
@liu-zhaokun I didn't mean we should replace configuration with normal string. I meant if the value is constant, don't add it to Storm configuration (even config map) and use the constant value directly from all usages. |
|
@harshach @liu-zhaokun From topology side it's always set to "digest" from Storm Submitter, so it is not effectively configurable from topology level. From Daemon (Nimbus, Supervisor, etc) side, it determines whether the cluster is using ZK auth via STORM_ZOOKEEPER_AUTH_SCHEME (cluster-wise ZK auth). Especially, Nimbus just removes STORM_ZOOKEEPER_TOPOLOGY_AUTH_SCHEME (topology-wise ZK auth) from topology config map when topology is submitted but STORM_ZOOKEEPER_AUTH_SCHEME is not set. The tricky part is a ZookeeperAuthInfo class. If STORM_ZOOKEEPER_TOPOLOGY_AUTH_SCHEME is set to cluster configuration, ZookeeperAuthInfo uses this. That means, even if ZookeeperAuthInfo class is used from Daemon side, topology-wise configuration will be used first. (I feel this behavior could be misleading.) So it shouldn't be set to "digest" with no auth. Long story in short, javadoc comment is effectively right even though it is really tricky. I'm not an expert of ZK auth and wondering why cluster wise configuration and topology wise configuration co-exist, given that it connects to same ZK. Could someone knowing about ZK auth explain this? |
|
Sorry I have been out of town or I would have joined the conversation sooner. Yes @HeartSaVioR is correct. The code is really confusing and overly complicated. I comes from when we were adding in security to storm. It is a long story where we ran into several bugs/limitations in ZK itself that prevented us from doing what we initial wanted to do. It also comes from the really confusing way that ZK handles authentication. ZK supports both SASL authentication which is per connection, and a special digest authentication with can be per command and can override the connection authentication if any. (don't confuse this special built in ZK digest authentication with the SASL digest authentication which ZK also supports, as they are different) SASL authentication with ZK is handled through the jaas conf. There is a "Client" section that says how you want to authenticate with the zookeeper servers. Using kerberos for this is the recommended way to authenticate ZK as it very secure. STORM_ZOOKEEPER_AUTH_SCHEME allows you to use the built in ZK digest auth scheme in the daemons. This is really only there for testing purposes. Although it could work in production if you wanted it to, it just will be no where near as secure as kerberos is. One of the issues we ran into with allowing the workers to communicate with ZK was that the workers needed some way to authenticate with ZK that didn't expire. We could force everyone to send a TGT to the worker and then use that to authenticate with ZK, but TGTs expire and so users would have to push a new one periodically. So we made the decision that we would use the built in ZK digest like a ticket. We would randomly generate a 128 bit number as the secret, and then let nimbus setup the ACLs in ZK to only allow a user with that 128 bit secret to access the data. That is how it currently works. We didn't really like that and wanted to leave open the possibility of using a different auth scheme with ZK in the future. That is why we have the configs as a string and not a boolean. If we want to update the documentation for the cofnigs we can. But I would say for now just mark them as being internal configs relating to security that the end user should not actually set. |
| @isString | ||
| public static final String STORM_ZOOKEEPER_TOPOLOGY_AUTH_SCHEME="storm.zookeeper.topology.auth.scheme"; | ||
|
|
||
| /** |
There was a problem hiding this comment.
-1 the problem with deleting these is that we don't get any config checks that it is a string.
We could move it to a different file so that it is hidden from end users.
|
@revans2 Thanks for the detailed explanation. I'm OK to leave this as it is, and maybe better to just add comment that it is the internal config and user shouldn't set it. @liu-zhaokun Sorry to guide to the wrong way. Could you roll back the change and follow @revans2 suggestion? |
|
@HeartSaVioR |
…n Config.java is wrong
liu-zhaokun
force-pushed
the
master07131533
branch
from
d8cd89e
to
67d41fc
Compare
|
@HeartSaVioR |
|
+1 for the change. @revans2 How about the last change? |
|
@Ethanlm |
|
@liu-zhaokun @Ethanlm is not a committer yet but I will try to pull it in. |
https://issues.apache.org/jira/browse/STORM-2627
The annotation of "storm.zookeeper.topology.auth.scheme" in Config.java is wrong.As StormSubmitter.java,line 91, says "STORM_ZOOKEEPER_TOPOLOGY_AUTH_SCHEME is should always be set to digest.", the annotation of "storm.zookeeper.topology.auth.scheme" in Config.java should be consistent with it.