Copilot commented on code in PR #16501:
URL: https://github.com/apache/iotdb/pull/16501#discussion_r2384531858
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/security/TreeAccessCheckVisitor.java:
##########
@@ -1055,153 +1361,202 @@ public TSStatus visitShowChildPaths(
ShowChildPathsStatement showChildPathsStatement, TreeAccessCheckContext
context) {
if (AuthorityChecker.SUPER_USER.equals(context.getUsername())) {
showChildPathsStatement.setCanSeeAuditDB(true);
+ recordObjectAuthenticationAuditLog(
+ context.setResult(true),
+ () ->
+ showChildPathsStatement.getPaths().stream()
+ .distinct()
+ .collect(Collectors.toList())
+ .toString());
return SUCCEED;
}
- setCanSeeAuditDB(showChildPathsStatement, context.getUsername());
+ setCanSeeAuditDB(showChildPathsStatement, context);
return visitAuthorityInformation(showChildPathsStatement, context);
}
@Override
public TSStatus visitAlterTimeSeries(
AlterTimeSeriesStatement statement, TreeAccessCheckContext context) {
+ context.setAuditLogOperation(AuditLogOperation.DDL);
// audit db is read-only
if (includeByAuditTreeDB(statement.getPath())
&&
!context.getUsername().equals(AuthorityChecker.INTERNAL_AUDIT_USER)) {
+ recordObjectAuthenticationAuditLog(
+ context.setResult(false),
+ () ->
statement.getPaths().stream().distinct().collect(Collectors.toList()).toString());
return new TSStatus(TSStatusCode.NO_PERMISSION.getStatusCode())
.setMessage(String.format(READ_ONLY_DB_ERROR_MSG,
TREE_MODEL_AUDIT_DATABASE));
}
- return checkTimeSeriesPermission(
- context.getUsername(), statement.getPaths(),
PrivilegeType.WRITE_SCHEMA);
+ return checkTimeSeriesPermission(context, statement::getPaths,
PrivilegeType.WRITE_SCHEMA);
}
@Override
public TSStatus visitDeleteTimeSeries(
DeleteTimeSeriesStatement statement, TreeAccessCheckContext context) {
+ context.setAuditLogOperation(AuditLogOperation.DDL);
// audit db is read-only
for (PartialPath path : statement.getPathPatternList()) {
if (includeByAuditTreeDB(path)
&&
!context.getUsername().equals(AuthorityChecker.INTERNAL_AUDIT_USER)) {
+ recordObjectAuthenticationAuditLog(
+ context.setResult(false),
+ () ->
statement.getPaths().stream().distinct().collect(Collectors.toList()).toString());
return new TSStatus(TSStatusCode.NO_PERMISSION.getStatusCode())
.setMessage(String.format(READ_ONLY_DB_ERROR_MSG,
TREE_MODEL_AUDIT_DATABASE));
}
}
- return checkTimeSeriesPermission(
- context.getUsername(), statement.getPaths(),
PrivilegeType.WRITE_SCHEMA);
+ return checkTimeSeriesPermission(context, statement::getPaths,
PrivilegeType.WRITE_SCHEMA);
}
// ================================== maintain related
=============================
@Override
public TSStatus visitExtendRegion(
ExtendRegionStatement statement, TreeAccessCheckContext context) {
- return checkGlobalAuth(context.getUsername(), PrivilegeType.MAINTAIN);
+ return checkGlobalAuth(
+ context.setAuditLogOperation(AuditLogOperation.DDL),
+ PrivilegeType.MAINTAIN,
+ () -> statement.getRegionIds().toString());
}
@Override
public TSStatus visitGetRegionId(GetRegionIdStatement statement,
TreeAccessCheckContext context) {
- return checkGlobalAuth(context.getUsername(), PrivilegeType.MAINTAIN);
+ return checkGlobalAuth(
+
context.setAuditLogOperation(AuditLogOperation.QUERY).setDatabase(statement.getDatabase()),
+ PrivilegeType.MAINTAIN,
+ statement::getDatabase);
}
@Override
public TSStatus visitGetSeriesSlotList(
GetSeriesSlotListStatement statement, TreeAccessCheckContext context) {
- return checkGlobalAuth(context.getUsername(), PrivilegeType.MAINTAIN);
+ return checkGlobalAuth(
+
context.setAuditLogOperation(AuditLogOperation.QUERY).setDatabase(statement.getDatabase()),
+ PrivilegeType.MAINTAIN,
+ statement::getDatabase);
}
@Override
public TSStatus visitGetTimeSlotList(
GetTimeSlotListStatement statement, TreeAccessCheckContext context) {
- return checkGlobalAuth(context.getUsername(), PrivilegeType.MAINTAIN);
+ return checkGlobalAuth(
+
context.setAuditLogOperation(AuditLogOperation.QUERY).setDatabase(statement.getDatabase()),
+ PrivilegeType.MAINTAIN,
+ statement::getDatabase);
}
@Override
public TSStatus visitCountTimeSlotList(
CountTimeSlotListStatement statement, TreeAccessCheckContext context) {
- return checkGlobalAuth(context.getUsername(), PrivilegeType.MAINTAIN);
+ return checkGlobalAuth(
+
context.setAuditLogOperation(AuditLogOperation.QUERY).setDatabase(statement.getDatabase()),
+ PrivilegeType.MAINTAIN,
+ statement::getDatabase);
}
@Override
public TSStatus visitKillQuery(KillQueryStatement statement,
TreeAccessCheckContext context) {
- if (checkHasGlobalAuth(context.getUsername(), PrivilegeType.MAINTAIN)) {
+ if (checkHasGlobalAuth(
+ context.setAuditLogOperation(AuditLogOperation.CONTROL),
+ PrivilegeType.MAINTAIN,
+ () -> "")) {
statement.setAllowedUsername(context.getUsername());
}
return SUCCEED;
}
@Override
public TSStatus visitFlush(FlushStatement flushStatement,
TreeAccessCheckContext context) {
- return checkGlobalAuth(context.getUsername(), PrivilegeType.SYSTEM);
+ return checkGlobalAuth(
+ context.setAuditLogOperation(AuditLogOperation.CONTROL),
+ PrivilegeType.SYSTEM,
+ () ->
+
flushStatement.getPaths().stream().distinct().collect(Collectors.toList()).toString());
}
@Override
public TSStatus visitSetConfiguration(
SetConfigurationStatement setConfigurationStatement,
TreeAccessCheckContext context) {
+ List<PrivilegeType> relatedPrivileges;
try {
- return AuthorityChecker.getTSStatus(
- AuthorityChecker.checkUserMissingSystemPermissions(
- context.getUsername(),
setConfigurationStatement.getNeededPrivileges()));
+ relatedPrivileges = new
ArrayList<>(setConfigurationStatement.getNeededPrivileges());
+ TSStatus result =
+ AuthorityChecker.getTSStatus(
+ AuthorityChecker.checkUserMissingSystemPermissions(
+ context.getUsername(), relatedPrivileges));
+ recordObjectAuthenticationAuditLog(
+ context
+ .setResult(result.getCode() ==
TSStatusCode.SUCCESS_STATUS.getStatusCode())
+ .setAuditLogOperation(AuditLogOperation.CONTROL)
+ .setPrivilegeTypes(relatedPrivileges),
+ () -> "");
+ return result;
} catch (IOException e) {
+ recordObjectAuthenticationAuditLog(
+
context.setResult(false).setAuditLogOperation(AuditLogOperation.CONTROL), () ->
"");
return AuthorityChecker.getTSStatus(false, "Failed to check config item
permission");
}
}
@Override
public TSStatus visitSetSystemStatus(
SetSystemStatusStatement setSystemStatusStatement,
TreeAccessCheckContext context) {
- return checkGlobalAuth(context.getUsername(), PrivilegeType.SYSTEM);
+ return checkGlobalAuth(
+ context.setAuditLogOperation(AuditLogOperation.CONTROL),
PrivilegeType.SYSTEM, () -> "");
}
@Override
public TSStatus visitStartRepairData(
StartRepairDataStatement startRepairDataStatement,
TreeAccessCheckContext context) {
- return checkGlobalAuth(context.getUsername(), PrivilegeType.SYSTEM);
+ return checkGlobalAuth(
+ context.setAuditLogOperation(AuditLogOperation.CONTROL),
PrivilegeType.SYSTEM, () -> "");
}
@Override
public TSStatus visitStopRepairData(
StopRepairDataStatement stopRepairDataStatement, TreeAccessCheckContext
context) {
- return checkGlobalAuth(context.getUsername(), PrivilegeType.SYSTEM);
+ return checkGlobalAuth(context, PrivilegeType.MAINTAIN, () -> "");
Review Comment:
Empty string suppliers for audit objects provide no meaningful audit
information. Consider passing more descriptive audit object information or a
null supplier if no specific object is involved.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/audit/DNAuditLogger.java:
##########
@@ -130,31 +129,52 @@ public void setCoordinator(Coordinator coordinator) {
@NotNull
private static InsertRowStatement generateInsertStatement(
- AuditLogFields auditLogFields, String log, PartialPath log_device) {
+ IAuditEntity auditLogFields, String log, PartialPath log_device) {
String username = auditLogFields.getUsername();
String address = auditLogFields.getCliHostname();
- AuditEventType type = auditLogFields.getAuditType();
- AuditLogOperation operation = auditLogFields.getOperationType();
- PrivilegeType privilegeType = auditLogFields.getPrivilegeType();
- PrivilegeLevel privilegeLevel = judgePrivilegeLevel(privilegeType);
+ AuditEventType type = auditLogFields.getAuditEventType();
+ AuditLogOperation operation = auditLogFields.getAuditLogOperation();
+ PrivilegeLevel privilegeLevel = null;
+ if (auditLogFields.getPrivilegeTypes() != null) {
+ for (PrivilegeType privilegeType : auditLogFields.getPrivilegeTypes()) {
+ privilegeLevel = judgePrivilegeLevel(privilegeType);
+ if (privilegeLevel.equals(PrivilegeLevel.GLOBAL)) {
+ break;
+ }
+ }
+ }
String dataNodeId = String.valueOf(config.getDataNodeId());
InsertRowStatement insertStatement = new InsertRowStatement();
insertStatement.setDevicePath(log_device);
insertStatement.setTime(CommonDateTimeUtils.currentTime());
insertStatement.setMeasurements(
new String[] {
- USERNAME,
- CLI_HOSTNAME,
- AUDIT_EVENT_TYPE,
- OPERATION_TYPE,
- PRIVILEGE_TYPE,
- PRIVILEGE_LEVEL,
- RESULT,
- DATABASE,
- SQL_STRING,
- LOG
+ AUDIT_LOG_USERNAME,
+ AUDIT_LOG_CLI_HOSTNAME,
+ AUDIT_LOG_AUDIT_EVENT_TYPE,
+ AUDIT_LOG_OPERATION_TYPE,
+ AUDIT_LOG_PRIVILEGE_TYPE,
+ AUDIT_LOG_PRIVILEGE_LEVEL,
+ AUDIT_LOG_RESULT,
+ AUDIT_LOG_DATABASE,
+ AUDIT_LOG_SQL_STRING,
+ AUDIT_LOG_LOG
});
insertStatement.setAligned(false);
+ String sqlString = auditLogFields.getSqlString();
+ if (sqlString != null) {
+ if (sqlString.toUpperCase().startsWith("CREATE USER")) {
+ sqlString = String.join(" ", Arrays.asList(sqlString.split("
")).subList(0, 3)) + " ...";
+ }
+ Pattern pattern = Pattern.compile("(?i)(values)\\([^)]*\\)");
Review Comment:
Pattern compilation inside the log method could impact performance when
called frequently. Consider making this a static final Pattern to compile it
only once.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/audit/AuditLogFields.java:
##########
@@ -66,32 +91,73 @@ public String getCliHostname() {
return cliHostname;
}
- public AuditEventType getAuditType() {
+ @Override
+ public AuditEventType getAuditEventType() {
return auditType;
}
- public AuditLogOperation getOperationType() {
+ @Override
+ public AuditLogOperation getAuditLogOperation() {
return operationType;
}
- public PrivilegeType getPrivilegeType() {
- return privilegeType;
+ @Override
+ public List<PrivilegeType> getPrivilegeTypes() {
+ return privilegeTypes;
}
- public boolean isResult() {
- return result;
- }
-
- public AuditLogFields setResult(boolean result) {
- this.result = result;
- return this;
+ @Override
+ public String getPrivilegeTypeString() {
+ return privilegeTypes.toString();
}
+ @Override
public String getDatabase() {
return database;
}
+ @Override
public String getSqlString() {
return sqlString;
}
+
+ @Override
+ public boolean getResult() {
+ return result;
+ }
+
+ @Override
+ public IAuditEntity setAuditEventType(AuditEventType auditEventType) {
+ throw new UnsupportedOperationException();
Review Comment:
The setter methods in AuditLogFields throw UnsupportedOperationException,
which could confuse API consumers. Consider making AuditLogFields immutable by
design and documenting this behavior, or provide a builder pattern for
modification.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/audit/DNAuditLogger.java:
##########
@@ -73,25 +74,23 @@
import java.time.ZoneId;
import java.util.Arrays;
import java.util.List;
+import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Supplier;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
import static
org.apache.iotdb.db.pipe.receiver.protocol.legacy.loader.ILoader.SCHEMA_FETCHER;
public class DNAuditLogger extends AbstractAuditLogger {
+ public static final String PREFIX_PASSWORD_HISTORY =
"root.__audit.password_history";
private static final Logger logger =
LoggerFactory.getLogger(DNAuditLogger.class);
+ // TODO: @zhujt20 Optimize the following stupid retry
+ private static final int INSERT_RETRY_COUNT = 5;
+ private static final int INSERT_RETRY_INTERVAL_MS = 2000;
+
private static final IoTDBConfig config =
IoTDBDescriptor.getInstance().getConfig();
Review Comment:
Hard-coded retry values should be configurable. Consider moving these to
configuration properties to allow runtime adjustment of audit log insertion
behavior.
```suggestion
// Retry values are now configurable via IoTDBConfig
private static final IoTDBConfig config =
IoTDBDescriptor.getInstance().getConfig();
private static int getInsertRetryCount() {
return config.getAuditLogInsertRetryCount();
}
private static int getInsertRetryIntervalMs() {
return config.getAuditLogInsertRetryIntervalMs();
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
