KAFKA-2639: Refactoring of ZkUtils #303
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| /** | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.kafka.common.security; | ||
|
|
||
| import java.io.File; | ||
| import java.net.URI; | ||
| import java.security.URIParameter; | ||
| import javax.security.auth.login.Configuration; | ||
| import org.apache.kafka.common.KafkaException; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class JaasUtils { | ||
| private static final Logger LOG = LoggerFactory.getLogger(JaasUtils.class); | ||
| public static final String LOGIN_CONTEXT_SERVER = "KafkaServer"; | ||
| public static final String LOGIN_CONTEXT_CLIENT = "KafkaClient"; | ||
| public static final String SERVICE_NAME = "serviceName"; | ||
| public static final String JAVA_LOGIN_CONFIG_PARAM = "java.security.auth.login.config"; | ||
| public static final String ZK_SASL_CLIENT = "zookeeper.sasl.client"; | ||
| public static final String ZK_LOGIN_CONTEXT_NAME_KEY = "zookeeper.sasl.clientconfig"; | ||
|
|
||
| public static boolean isSecure(String loginConfigFile) { | ||
|
Member
There was a problem hiding this comment. As this is ZK-specific, the method name should make that clear. |
||
| boolean isSecurityEnabled = false; | ||
| boolean zkSaslEnabled = Boolean.getBoolean(System.getProperty(ZK_SASL_CLIENT, "true")); | ||
| String zkLoginContextName = System.getProperty(ZK_LOGIN_CONTEXT_NAME_KEY, "Client"); | ||
|
Contributor
There was a problem hiding this comment. Is the ZK login context name customizable? I thought Zookeeper client library always expects it to be Client.
Contributor
Author
There was a problem hiding this comment. It is customizable. This is not properly documented in ZK. |
||
|
|
||
|
|
||
| if (loginConfigFile != null && loginConfigFile.length() > 0) { | ||
| File configFile = new File(loginConfigFile); | ||
| if (!configFile.canRead()) { | ||
| throw new KafkaException("File " + loginConfigFile + "cannot be read."); | ||
| } | ||
| try { | ||
| URI configUri = configFile.toURI(); | ||
| Configuration loginConf = Configuration.getInstance("JavaLoginConfig", new URIParameter(configUri)); | ||
| isSecurityEnabled = loginConf.getAppConfigurationEntry(zkLoginContextName) != null; | ||
| } catch (Exception e) { | ||
| throw new KafkaException(e); | ||
| } | ||
| if (isSecurityEnabled && !zkSaslEnabled) { | ||
| LOG.error("JAAS file is present, but system property " + | ||
| ZK_SASL_CLIENT + " is set to false, which disables " + | ||
| "SASL in the ZooKeeper client"); | ||
| throw new KafkaException("Exception while determining if ZooKeeper is secure"); | ||
|
Member
There was a problem hiding this comment. I had a look at instances where we call As we discussed, I prefer just throw to avoid duplicate entries in the logs and because it's more composable (a utility method like this can be called from multiple contexts and it's better to let the caller decide how to handle the exception). I would be interested in @junrao's opinion on this.
Contributor
Author
There was a problem hiding this comment. @ijuma I was thinking that we can't guarantee that exceptions are always logged. They can be caught and they might not end up in the log output. The log error message is guaranteed to be in the log output and to indicate that an error occurred.
Member
There was a problem hiding this comment. Yeah, that's the argument for having it there (thanks for mentioning it). However, if the caller can actually handle the exception, then it is perhaps not something that should be logged at error level anymore. Whatever approach we take generally, it is important to be consistent because it breaks down if we use a mixture of approaches (in this method, we are only logging in only one of the exception paths, for example). This is why I asked for @junrao's input.
Contributor
There was a problem hiding this comment. In general, if the caller is already logging the exception, we don't have to log it when the exception is thrown, unless there is more context when the exception is thrown that needs to be logged.
Contributor
There was a problem hiding this comment. Actually, do we need to throw exception here? During the broker migration, it's ok to set the jaas file, but disable setting the acl.
Contributor
Author
|
||
| } | ||
| } | ||
|
|
||
| return isSecurityEnabled; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought on the broker side, we want to add a broker config to enable setting ZK acl. On the client side (this will mostly be for tools), we will just enable ZK acl if the system property "java.security.auth.login.config" is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config parameter is introduced in the PR #313. Here I'm focusing on the refactoring and I just followed the approach in the original PR #93 of using the system property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds good.