You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2022/11/17 09:57:06 UTC
[GitHub] [iotdb] caozj1011 opened a new pull request, #8031: [rel/0.13] upgrade audit log
caozj1011 opened a new pull request, #8031:
URL: https://github.com/apache/iotdb/pull/8031
1. added whether to enable audit logs during open session.
2. replace the original AUDIT_LOGGER.
3. when the Audit log level is set to LOGGER, the write operation is changed from DEBUG to INFO.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] JackieTien97 commented on a diff in pull request #8031: [To rel/0.13] upgrade audit log
Posted by GitBox <gi...@apache.org>.
JackieTien97 commented on code in PR #8031:
URL: https://github.com/apache/iotdb/pull/8031#discussion_r1036589856
##########
jdbc/src/main/java/org/apache/iotdb/jdbc/IoTDBConnection.java:
##########
@@ -464,7 +464,7 @@ private void openSession() throws SQLException {
openReq.setPassword(params.getPassword());
openReq.setZoneId(getTimeZone());
openReq.putToConfiguration("version", params.getVersion().toString());
-
+ openReq.putToConfiguration("enableAudit", String.valueOf(params.isEnableAudit()));
Review Comment:
use the defined constant value `AUTH_ENABLE_AUDIT` and `VERSION` instead of this raw string.
##########
jdbc/src/main/java/org/apache/iotdb/jdbc/Config.java:
##########
@@ -56,4 +56,8 @@ private Config() {}
/** key of thrift max frame size */
public static final String THRIFT_FRAME_MAX_SIZE = "thrift_max_frame_size";
+
+ static final String AUTH_ENABLE_AUDIT = "enableAudit";
Review Comment:
```suggestion
public static final String AUTH_ENABLE_AUDIT = "enableAudit";
```
##########
server/src/main/java/org/apache/iotdb/db/service/basic/ServiceProvider.java:
##########
@@ -206,21 +206,26 @@ public BasicOpenSessionResp login(
openSessionResp.setCode(TSStatusCode.SUCCESS_STATUS.getStatusCode());
openSessionResp.setMessage("Login successfully");
- SESSION_MANAGER.supplySession(session, username, zoneId, clientVersion);
- LOGGER.info(
- "{}: Login status: {}. User : {}, opens Session-{}",
- IoTDBConstant.GLOBAL_DB_NAME,
- openSessionResp.getMessage(),
- username,
- session);
+ SESSION_MANAGER.supplySession(session, username, zoneId, clientVersion, enableAudit);
+ if (!AuditLogUtils.LOG_LEVEL_NONE.equals(CONFIG.getAuditLogStorage())) {
Review Comment:
It seems that it will be constant value after iotdb starting, no need to do this judgement each time.
##########
server/src/main/java/org/apache/iotdb/db/service/thrift/impl/TSServiceImpl.java:
##########
@@ -345,6 +351,14 @@ private IoTDBConstant.ClientVersion parseClientVersion(TSOpenSessionReq req) {
return IoTDBConstant.ClientVersion.V_0_12;
}
+ private boolean parseEnableAudit(TSOpenSessionReq req) {
+ Map<String, String> configuration = req.configuration;
+ if (configuration != null && configuration.containsKey("enableAudit")) {
+ return Boolean.parseBoolean(configuration.get("enableAudit"));
+ }
+ return true;
Review Comment:
default true? shouldn't be false?
##########
server/src/main/java/org/apache/iotdb/db/service/thrift/impl/TSServiceImpl.java:
##########
@@ -345,6 +351,14 @@ private IoTDBConstant.ClientVersion parseClientVersion(TSOpenSessionReq req) {
return IoTDBConstant.ClientVersion.V_0_12;
}
+ private boolean parseEnableAudit(TSOpenSessionReq req) {
+ Map<String, String> configuration = req.configuration;
+ if (configuration != null && configuration.containsKey("enableAudit")) {
+ return Boolean.parseBoolean(configuration.get("enableAudit"));
Review Comment:
same as above
##########
server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfig.java:
##########
@@ -875,11 +875,15 @@ public class IoTDBConfig {
private int archivingThreadNum = 2;
// determines whether audit logs are written to log files or IoTDB
- private String auditLogStorage = AuditLogUtils.LOG_LEVEL_NONE;
+ private String auditLogStorage = AuditLogUtils.LOG_LEVEL_LOGGER;
// determines whether audit logs record IoTDB write operation
private boolean enableAuditLogWrite = false;
+ private int remoteConfigPort = 8667;
+
+ private boolean enableExternalService = false;
Review Comment:
It seems that these two fields are totally not used.
##########
metrics/interface/src/main/java/org/apache/iotdb/metrics/config/MetricConfigDescriptor.java:
##########
@@ -122,6 +123,13 @@ private String getPropsUrl() {
}
}
}
+ if (url == null) {
+ URL uri = MetricConfig.class.getResource("/" + MetricConstant.CONFIG_NAME);
+ logger.info("MetricConfig.class.getResource,{}", uri);
+ if (uri != null) {
+ return uri.getPath();
+ }
+ }
Review Comment:
Add some comments about this if-branch, actually I can't understand its motivation.
##########
server/src/main/java/org/apache/iotdb/db/service/thrift/impl/TSServiceImpl.java:
##########
@@ -222,8 +224,10 @@ public TSExecuteStatementResp call() throws Exception {
plan.setLoginUserName(username);
QUERY_FREQUENCY_RECORDER.incrementAndGet();
- AUDIT_LOGGER.debug("Session {} execute Query: {}", session, statement);
-
+ if (!AuditLogUtils.LOG_LEVEL_NONE.equals(conf.getAuditLogStorage())) {
Review Comment:
same as above, you check it all throught your pr and I won't point it out in the remaing reviews.
##########
server/src/main/java/org/apache/iotdb/db/service/basic/ServiceProvider.java:
##########
@@ -206,21 +206,26 @@ public BasicOpenSessionResp login(
openSessionResp.setCode(TSStatusCode.SUCCESS_STATUS.getStatusCode());
openSessionResp.setMessage("Login successfully");
- SESSION_MANAGER.supplySession(session, username, zoneId, clientVersion);
- LOGGER.info(
- "{}: Login status: {}. User : {}, opens Session-{}",
- IoTDBConstant.GLOBAL_DB_NAME,
- openSessionResp.getMessage(),
- username,
- session);
+ SESSION_MANAGER.supplySession(session, username, zoneId, clientVersion, enableAudit);
+ if (!AuditLogUtils.LOG_LEVEL_NONE.equals(CONFIG.getAuditLogStorage())) {
+ AuditLogUtils.writeAuditLog(
+ String.format(
+ "%s: Login status: %s. User : %s, opens Session-%s",
+ IoTDBConstant.GLOBAL_DB_NAME, openSessionResp.getMessage(), username, session));
+ }
+
} else {
openSessionResp.setMessage(loginMessage != null ? loginMessage : "Authentication failed.");
openSessionResp.setCode(TSStatusCode.WRONG_LOGIN_PASSWORD_ERROR.getStatusCode());
- AUDIT_LOGGER.info("User {} opens Session failed with an incorrect password", username);
+ session.setUsername(username);
+ if (!AuditLogUtils.LOG_LEVEL_NONE.equals(CONFIG.getAuditLogStorage())) {
+ AuditLogUtils.writeAuditLog(
+ String.format("User %s opens Session failed with an incorrect password", username),
+ true);
Review Comment:
It means that we need to record all the failed login info?
##########
session/src/main/java/org/apache/iotdb/session/SessionConnection.java:
##########
@@ -141,7 +141,7 @@ private void init(EndPoint endPoint) throws IoTDBConnectionException {
openReq.setPassword(session.password);
openReq.setZoneId(zoneId.toString());
openReq.putToConfiguration("version", session.version.toString());
-
+ openReq.putToConfiguration("enableAudit", String.valueOf(session.enableAudit));
Review Comment:
same as above
##########
server/src/main/java/org/apache/iotdb/db/service/IoTDB.java:
##########
@@ -194,6 +195,13 @@ private void setUp() throws StartupException, QueryProcessException {
registerManager.register(ContinuousQueryService.getInstance());
registerManager.register(MetricService.getInstance());
+ if (IoTDBDescriptor.getInstance().getConfig().isEnableExternalService()) {
+ ServiceLoader<IService> iServices = ServiceLoader.load(IService.class);
+ for (IService iService : iServices) {
+ registerManager.register(iService);
+ }
+ }
+
Review Comment:
Why here re-register all the services again?
##########
server/src/main/java/org/apache/iotdb/db/service/thrift/impl/TSServiceImpl.java:
##########
@@ -345,6 +351,14 @@ private IoTDBConstant.ClientVersion parseClientVersion(TSOpenSessionReq req) {
return IoTDBConstant.ClientVersion.V_0_12;
}
+ private boolean parseEnableAudit(TSOpenSessionReq req) {
+ Map<String, String> configuration = req.configuration;
+ if (configuration != null && configuration.containsKey("enableAudit")) {
Review Comment:
use constant value instead of this raw string.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] caozj1011 commented on a diff in pull request #8031: [To rel/0.13] upgrade audit log
Posted by GitBox <gi...@apache.org>.
caozj1011 commented on code in PR #8031:
URL: https://github.com/apache/iotdb/pull/8031#discussion_r1036689336
##########
server/src/main/java/org/apache/iotdb/db/service/basic/ServiceProvider.java:
##########
@@ -206,21 +206,26 @@ public BasicOpenSessionResp login(
openSessionResp.setCode(TSStatusCode.SUCCESS_STATUS.getStatusCode());
openSessionResp.setMessage("Login successfully");
- SESSION_MANAGER.supplySession(session, username, zoneId, clientVersion);
- LOGGER.info(
- "{}: Login status: {}. User : {}, opens Session-{}",
- IoTDBConstant.GLOBAL_DB_NAME,
- openSessionResp.getMessage(),
- username,
- session);
+ SESSION_MANAGER.supplySession(session, username, zoneId, clientVersion, enableAudit);
+ if (!AuditLogUtils.LOG_LEVEL_NONE.equals(CONFIG.getAuditLogStorage())) {
+ AuditLogUtils.writeAuditLog(
+ String.format(
+ "%s: Login status: %s. User : %s, opens Session-%s",
+ IoTDBConstant.GLOBAL_DB_NAME, openSessionResp.getMessage(), username, session));
+ }
+
} else {
openSessionResp.setMessage(loginMessage != null ? loginMessage : "Authentication failed.");
openSessionResp.setCode(TSStatusCode.WRONG_LOGIN_PASSWORD_ERROR.getStatusCode());
- AUDIT_LOGGER.info("User {} opens Session failed with an incorrect password", username);
+ session.setUsername(username);
+ if (!AuditLogUtils.LOG_LEVEL_NONE.equals(CONFIG.getAuditLogStorage())) {
+ AuditLogUtils.writeAuditLog(
+ String.format("User %s opens Session failed with an incorrect password", username),
+ true);
Review Comment:
yes, the user wants to know which illegitimate users are connected to the database
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] JackieTien97 merged pull request #8031: [To rel/0.13] upgrade audit log
Posted by GitBox <gi...@apache.org>.
JackieTien97 merged PR #8031:
URL: https://github.com/apache/iotdb/pull/8031
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org