[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers. URL: https://github.com/apache/zookeeper/pull/1133#discussion_r344181499 ## File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md ## @@ -956,6 +956,18 @@ property, when available, is noted below. and restart ZooKeeper process so ZooKeeper can continue normal data consistency check during recovery process. Default value is false. +* *audit.enabled* : Review comment: There's a typo here: the property name is `enable` without the "D". This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r344186167
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java
##
@@ -280,6 +306,8 @@ public void processRequest(Request request) {
rsp = new CreateResponse(rc.path);
err = Code.get(rc.err);
requestPathMetricsCollector.registerRequest(request.type,
rc.path);
+String createMode = getCreateMode(request);
Review comment:
Great, thanks!
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers. URL: https://github.com/apache/zookeeper/pull/1133#discussion_r344181499 ## File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md ## @@ -956,6 +956,18 @@ property, when available, is noted below. and restart ZooKeeper process so ZooKeeper can continue normal data consistency check during recovery process. Default value is false. +* *audit.enabled* : Review comment: There's a type here: the property name is `enable` without the "D". This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r344157467
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/audit/Log4jAuditLogger.java
##
@@ -0,0 +1,37 @@
+/*
+ * 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.zookeeper.audit;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Log4j based audit logger
+ */
+public class Log4jAuditLogger implements AuditLogger {
+private static final Logger LOG =
LoggerFactory.getLogger(Log4jAuditLogger.class);
+
+@Override
+public void logAuditEvent(AuditEvent auditEvent) {
+if
(AuditConstants.FAILURE.equals(auditEvent.getValue(AuditEvent.FieldName.RESULT)))
{
Review comment:
Awesome! Thank you.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r343100667
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/audit/AuditEvent.java
##
@@ -0,0 +1,79 @@
+/*
+ * 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.zookeeper.audit;
+
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+public final class AuditEvent {
+private static final char PAIR_SEPARATOR = '\t';
+private static final String KEY_VAL_SEPARATOR = "=";
+// Holds the entries which to be logged.
+private Map logEntries = new LinkedHashMap();
+
+/**
+ * Gives all entries to be logged.
+ *
+ * @return log entries
+ */
+public Set> getLogEntries() {
Review comment:
This getter is not used anywhere.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r343104047
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/audit/ZKAuditLogger.java
##
@@ -0,0 +1,163 @@
+/*
+ * 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.zookeeper.audit;
+
+import static org.apache.zookeeper.audit.AuditEvent.FieldName;
+import java.lang.reflect.Constructor;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZKAuditLogger {
+public static final String SYSPROP_AUDIT_ENABLE = "zookeeper.audit.enable";
+public static final String SYSPROP_AUDIT_LOGGER_IMPL =
"zookeeper.audit.impl.class";
Review comment:
Both package-private ?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r343119363
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java
##
@@ -311,13 +343,19 @@ public void processRequest(Request request) {
((QuorumZooKeeperServer)
zks).self.getQuorumVerifier().toString().getBytes(),
rc.stat);
err = Code.get(rc.err);
+addAuditLog(request, cnxn, AuditConstants.OP_RECONFIG,
rc.path, null, null, err);
break;
}
case OpCode.setACL: {
lastOp = "SETA";
rsp = new SetACLResponse(rc.stat);
err = Code.get(rc.err);
requestPathMetricsCollector.registerRequest(request.type,
rc.path);
+/** Here audit enable check is done to avoid getACLs() call in
case audit is disabled. */
+if (ZKAuditLogger.isAuditEnabled()) {
Review comment:
Similarly pass the `request` object to `addAuditLog()` and extract the
required information inside the `if`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r343119074
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java
##
@@ -280,6 +306,8 @@ public void processRequest(Request request) {
rsp = new CreateResponse(rc.path);
err = Code.get(rc.err);
requestPathMetricsCollector.registerRequest(request.type,
rc.path);
+String createMode = getCreateMode(request);
Review comment:
This `getCreateMode()` is quite expensive. It should be moved inside the if
which checks whether audit logging is enabled. You might want to just pass the
request object to `addAuditLog()` and extract in there if necessary.
Additionally we might not need AuditConstant for every single operation, but
use OpCode directly. In which case audit logging could be called at one place
instead of every case branch.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r343122357
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java
##
@@ -624,4 +662,157 @@ private void updateStats(Request request, String lastOp,
long lastZxid) {
request.cnxn.updateStatsForResponse(request.cxid, lastZxid, lastOp,
request.createTime, currentTime);
}
+private void addSuccessAudit(Request request, ServerCnxn cnxn, String op,
String path) {
+addSuccessAudit(request, cnxn, op, path, null, null);
+}
+
+private void addSuccessAudit(Request request, ServerCnxn cnxn, String op,
String path,
+ String acl, String createMode) {
+if (!ZKAuditLogger.isAuditEnabled()) {
+return;
+}
+ZKAuditLogger
+.logSuccess(request.getUsers(), op, path, acl, createMode,
cnxn.getSessionIdHex(),
+cnxn.getHostAddress());
+}
+
+private void addFailureAudit(Request request, ServerCnxn cnxn, String op,
String path) {
+addFailureAudit(request, cnxn, op, path, null, null);
+}
+
+private void addFailureAudit(Request request, ServerCnxn cnxn, String op,
String path,
+ String acl, String createMode) {
+if (!ZKAuditLogger.isAuditEnabled()) {
+return;
+}
+ZKAuditLogger
+.logFailure(request.getUsers(), op, path, acl, createMode,
cnxn.getSessionIdHex(),
+cnxn.getHostAddress());
+}
+
+private void addAuditLog(Request request, ServerCnxn cnxn, String op,
String path, String acl,
+ String createMode, Code err) {
+if (!ZKAuditLogger.isAuditEnabled()) {
+return;
+}
+if (err == Code.OK) {
+ZKAuditLogger
+.logSuccess(request.getUsers(), op, path, acl, createMode,
cnxn.getSessionIdHex(),
+cnxn.getHostAddress());
+} else {
+ZKAuditLogger
+.logFailure(request.getUsers(), op, path, acl, createMode,
cnxn.getSessionIdHex(),
+cnxn.getHostAddress());
+}
+}
+
+private String getACLs(Request request) {
+ByteBuffer reqData = request.request.slice();
+reqData.rewind();
+SetACLRequest setACLRequest = new SetACLRequest();
+try {
+ByteBufferInputStream.byteBuffer2Record(reqData, setACLRequest);
+} catch (IOException e) {
+e.printStackTrace();
+}
+return ZKUtil.aclToString(setACLRequest.getAcl());
+}
+
+private void addFailedTxnAuditLog(Request request) {
Review comment:
I'd make something very similar to this for success events:
Get the `request` as input, short circuit if audit is disabled or the event
is not eligible for auditing and then do stuff.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r343109530
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/audit/ZKAuditLogger.java
##
@@ -0,0 +1,163 @@
+/*
+ * 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.zookeeper.audit;
+
+import static org.apache.zookeeper.audit.AuditEvent.FieldName;
+import java.lang.reflect.Constructor;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZKAuditLogger {
Review comment:
I'd rename this to `ZKAuditLog` or something like that to distinguish from
implementations of `AuditLogger`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r343100733
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/audit/AuditEvent.java
##
@@ -0,0 +1,79 @@
+/*
+ * 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.zookeeper.audit;
+
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+public final class AuditEvent {
+private static final char PAIR_SEPARATOR = '\t';
+private static final String KEY_VAL_SEPARATOR = "=";
+// Holds the entries which to be logged.
+private Map logEntries = new LinkedHashMap();
+
+/**
+ * Gives all entries to be logged.
+ *
+ * @return log entries
+ */
+public Set> getLogEntries() {
+return logEntries.entrySet();
+}
+
+public void addEntry(FieldName fieldName, String value) {
Review comment:
package-private ?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r343106196
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/audit/ZKAuditLogger.java
##
@@ -0,0 +1,163 @@
+/*
+ * 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.zookeeper.audit;
+
+import static org.apache.zookeeper.audit.AuditEvent.FieldName;
+import java.lang.reflect.Constructor;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZKAuditLogger {
+public static final String SYSPROP_AUDIT_ENABLE = "zookeeper.audit.enable";
+public static final String SYSPROP_AUDIT_LOGGER_IMPL =
"zookeeper.audit.impl.class";
+private static final Logger LOG =
LoggerFactory.getLogger(ZKAuditLogger.class);
+// By default audit logging is disabled
+private static boolean auditEnabled =
Boolean.getBoolean(SYSPROP_AUDIT_ENABLE);
+private static AuditLogger auditLogger;
+
+static {
+if (auditEnabled) {
+//initialise only when audit logging is enabled
+auditLogger = getAuditLogger();
+LOG.info("ZooKeeper audit is enabled.");
+} else {
+LOG.info("ZooKeeper audit is disabled.");
+}
+}
+
+private static AuditLogger getAuditLogger() {
+String auditLoggerClass =
System.getProperty(SYSPROP_AUDIT_LOGGER_IMPL);
+if (auditLoggerClass == null) {
+auditLoggerClass = Log4jAuditLogger.class.getName();
+}
+try {
+Constructor clientCxnConstructor =
Class.forName(auditLoggerClass)
+.getDeclaredConstructor();
+AuditLogger auditLogger = (AuditLogger)
clientCxnConstructor.newInstance();
+auditLogger.initialize();
+return auditLogger;
+} catch (Exception e) {
+throw new RuntimeException("Couldn't instantiate " +
auditLoggerClass, e);
+}
+}
+
+/**
+ * @return true if audit log is enabled
+ */
+public static boolean isAuditEnabled() {
+return auditEnabled;
+}
+
+// @VisibleForTesting
+public static void setAuditEnabled(boolean auditEnabled) {
+ZKAuditLogger.auditEnabled = auditEnabled;
+}
+
+public static void logSuccess(String user, String operation) {
+log(user, operation, AuditConstants.SUCCESS);
+}
+
+public static void logInvoked(String user, String operation) {
+log(user, operation, AuditConstants.INVOKED);
Review comment:
Both could be private.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r343106951
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/audit/Log4jAuditLogger.java
##
@@ -0,0 +1,37 @@
+/*
+ * 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.zookeeper.audit;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Log4j based audit logger
+ */
+public class Log4jAuditLogger implements AuditLogger {
+private static final Logger LOG =
LoggerFactory.getLogger(Log4jAuditLogger.class);
+
+@Override
+public void logAuditEvent(AuditEvent auditEvent) {
+if
(AuditConstants.FAILURE.equals(auditEvent.getValue(AuditEvent.FieldName.RESULT)))
{
Review comment:
Additionally this is currently a string comparison every time an audit event
occurs. I rather use a boolean value for success/failure instead to make it as
fast as possible.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r343102884
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/audit/Log4jAuditLogger.java
##
@@ -0,0 +1,37 @@
+/*
+ * 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.zookeeper.audit;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Log4j based audit logger
+ */
+public class Log4jAuditLogger implements AuditLogger {
+private static final Logger LOG =
LoggerFactory.getLogger(Log4jAuditLogger.class);
+
+@Override
+public void logAuditEvent(AuditEvent auditEvent) {
+if
(AuditConstants.FAILURE.equals(auditEvent.getValue(AuditEvent.FieldName.RESULT)))
{
Review comment:
If RESULT is a mandatory field of an `AuditEvent`, it should be a member
field instead of adding it to the Map. In which case we could replace this part
with a single field comparison.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r343105268
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/audit/ZKAuditLogger.java
##
@@ -0,0 +1,163 @@
+/*
+ * 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.zookeeper.audit;
+
+import static org.apache.zookeeper.audit.AuditEvent.FieldName;
+import java.lang.reflect.Constructor;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZKAuditLogger {
+public static final String SYSPROP_AUDIT_ENABLE = "zookeeper.audit.enable";
+public static final String SYSPROP_AUDIT_LOGGER_IMPL =
"zookeeper.audit.impl.class";
+private static final Logger LOG =
LoggerFactory.getLogger(ZKAuditLogger.class);
+// By default audit logging is disabled
+private static boolean auditEnabled =
Boolean.getBoolean(SYSPROP_AUDIT_ENABLE);
+private static AuditLogger auditLogger;
+
+static {
+if (auditEnabled) {
+//initialise only when audit logging is enabled
+auditLogger = getAuditLogger();
+LOG.info("ZooKeeper audit is enabled.");
+} else {
+LOG.info("ZooKeeper audit is disabled.");
+}
+}
+
+private static AuditLogger getAuditLogger() {
+String auditLoggerClass =
System.getProperty(SYSPROP_AUDIT_LOGGER_IMPL);
+if (auditLoggerClass == null) {
+auditLoggerClass = Log4jAuditLogger.class.getName();
+}
+try {
+Constructor clientCxnConstructor =
Class.forName(auditLoggerClass)
+.getDeclaredConstructor();
+AuditLogger auditLogger = (AuditLogger)
clientCxnConstructor.newInstance();
+auditLogger.initialize();
+return auditLogger;
+} catch (Exception e) {
+throw new RuntimeException("Couldn't instantiate " +
auditLoggerClass, e);
+}
+}
+
+/**
+ * @return true if audit log is enabled
+ */
+public static boolean isAuditEnabled() {
+return auditEnabled;
+}
+
+// @VisibleForTesting
+public static void setAuditEnabled(boolean auditEnabled) {
Review comment:
Unused?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit logging in ZooKeeper servers.
anmolnar commented on a change in pull request #1133: ZOOKEEPER-1260:Audit
logging in ZooKeeper servers.
URL: https://github.com/apache/zookeeper/pull/1133#discussion_r343108218
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/audit/ZKAuditLogger.java
##
@@ -0,0 +1,163 @@
+/*
+ * 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.zookeeper.audit;
+
+import static org.apache.zookeeper.audit.AuditEvent.FieldName;
+import java.lang.reflect.Constructor;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZKAuditLogger {
+public static final String SYSPROP_AUDIT_ENABLE = "zookeeper.audit.enable";
+public static final String SYSPROP_AUDIT_LOGGER_IMPL =
"zookeeper.audit.impl.class";
+private static final Logger LOG =
LoggerFactory.getLogger(ZKAuditLogger.class);
+// By default audit logging is disabled
+private static boolean auditEnabled =
Boolean.getBoolean(SYSPROP_AUDIT_ENABLE);
+private static AuditLogger auditLogger;
+
+static {
+if (auditEnabled) {
+//initialise only when audit logging is enabled
+auditLogger = getAuditLogger();
+LOG.info("ZooKeeper audit is enabled.");
+} else {
+LOG.info("ZooKeeper audit is disabled.");
+}
+}
+
+private static AuditLogger getAuditLogger() {
+String auditLoggerClass =
System.getProperty(SYSPROP_AUDIT_LOGGER_IMPL);
+if (auditLoggerClass == null) {
+auditLoggerClass = Log4jAuditLogger.class.getName();
+}
+try {
+Constructor clientCxnConstructor =
Class.forName(auditLoggerClass)
+.getDeclaredConstructor();
+AuditLogger auditLogger = (AuditLogger)
clientCxnConstructor.newInstance();
+auditLogger.initialize();
+return auditLogger;
+} catch (Exception e) {
+throw new RuntimeException("Couldn't instantiate " +
auditLoggerClass, e);
+}
+}
+
+/**
+ * @return true if audit log is enabled
+ */
+public static boolean isAuditEnabled() {
+return auditEnabled;
+}
+
+// @VisibleForTesting
+public static void setAuditEnabled(boolean auditEnabled) {
+ZKAuditLogger.auditEnabled = auditEnabled;
+}
+
+public static void logSuccess(String user, String operation) {
+log(user, operation, AuditConstants.SUCCESS);
+}
+
+public static void logInvoked(String user, String operation) {
+log(user, operation, AuditConstants.INVOKED);
+}
+
+public static void logSuccess(String user, String operation, String znode,
String acl,
+ String createMode, String session, String
ip) {
+log(user, operation, znode, acl, createMode, session, ip,
+AuditConstants.SUCCESS);
+}
+
+public static void logFailure(String user, String operation, String znode,
String acl,
+ String createMode, String session, String
ip) {
+log(user, operation, znode, acl, createMode, session, ip,
+AuditConstants.FAILURE);
+}
+
+private static void log(String user, String operation, String result) {
+auditLogger.logAuditEvent(createLogEvent(user, operation, result));
+}
+
+private static void log(String user, String operation, String znode,
String acl,
+String createMode, String session, String ip,
String result) {
+auditLogger.logAuditEvent(createLogEvent(user, operation, znode, acl,
createMode, session, ip, result));
+}
+
+/**
+ * A helper api for creating an AuditEvent object.
+ */
+public static AuditEvent createLogEvent(String user, String operation,
String result) {
Review comment:
Both methods could be package-private.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact
