CRZbulabula commented on code in PR #17634:
URL: https://github.com/apache/iotdb/pull/17634#discussion_r3217857680
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/schema/table/Audit.java:
##########
@@ -28,6 +28,8 @@ public class Audit {
public static final String TABLE_MODEL_AUDIT_DATABASE = "__audit";
public static final String TREE_MODEL_AUDIT_DATABASE =
String.format("%s.%s", ROOT, TABLE_MODEL_AUDIT_DATABASE);
+ public static final String RESERVED_DATABASE_NAME_ERROR_MSG =
+ "The database name \"%s\" is reserved, please use another valid database
name.";
Review Comment:
**Inconsistent error messages across operations**
This new `RESERVED_DATABASE_NAME_ERROR_MSG` is only used for **CREATE
DATABASE**, while other mutation operations still use the old
`READ_ONLY_DB_ERROR_MSG` (`"The database '%s' is read-only."`). This results in
inconsistent user-facing messages:
- `CREATE DATABASE __audit` → `"__audit" is reserved`
- `ALTER DATABASE root.__audit` → `'root.__audit' is read-only` (via
`checkCreateOrAlterDatabasePermission` line 1078)
- `DELETE DATABASE root.__audit` → `'root.__audit' is read-only` (via
`visitDeleteDatabase` line 1051)
- Table-model INSERT / CREATE TABLE on `__audit` → `'__audit' is read-only`
(via `checkAuditDatabase` line 73)
Consider updating all audit-database rejection paths to use this new message
for consistency, or at least all DDL operations.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/security/TreeAccessCheckVisitor.java:
##########
@@ -985,6 +987,17 @@ public TSStatus visitShowExternalService(
@Override
public TSStatus visitSetDatabase(
DatabaseSchemaStatement statement, TreeAccessCheckContext context) {
+ if (!AuthorityChecker.INTERNAL_AUDIT_USER.equals(context.getUsername())
+ && isAuditTreeDatabase(statement.getDatabasePath())) {
+ context
+ .setDatabase(statement.getDatabasePath().getFullPath())
+ .setPrivilegeType(PrivilegeType.MANAGE_DATABASE)
+ .setAuditLogOperation(AuditLogOperation.DDL);
+ AUDIT_LOGGER.recordObjectAuthenticationAuditLog(
+ context.setResult(false), statement.getDatabasePath()::getFullPath);
+ return new TSStatus(TSStatusCode.NO_PERMISSION.getStatusCode())
+
.setMessage(getReservedDatabaseNameErrorMsg(TREE_MODEL_AUDIT_DATABASE));
Review Comment:
Nit: After this new early-return block,
`checkCreateOrAlterDatabasePermission` (called at the line below) contains its
own audit database check (at line 1070-1078) that is now **unreachable** for
the CREATE path. Not a bug, but the duplicated logic could be cleaned up — for
example, by updating `checkCreateOrAlterDatabasePermission` itself to use
`isAuditTreeDatabase` with the new error message. This would protect both
CREATE and ALTER consistently and eliminate the redundancy.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/security/AccessControlImpl.java:
##########
@@ -77,6 +78,18 @@ private void checkAuditDatabase(String databaseName) {
@Override
public void checkCanCreateDatabase(
String userName, String databaseName, IAuditEntity auditEntity) {
+ if (!AuthorityChecker.INTERNAL_AUDIT_USER.equals(userName)
+ && TABLE_MODEL_AUDIT_DATABASE.equalsIgnoreCase(databaseName)) {
+ DNAuditLogger.getInstance()
+ .recordObjectAuthenticationAuditLog(
+ auditEntity
+ .setAuditLogOperation(AuditLogOperation.DDL)
+ .setDatabase(databaseName)
+ .setPrivilegeType(PrivilegeType.CREATE)
+ .setResult(false),
+ () -> databaseName);
+ throw new
AccessDeniedException(getReservedDatabaseNameErrorMsg(TABLE_MODEL_AUDIT_DATABASE));
Review Comment:
**Table model: missing DROP/ALTER audit database protection**
The audit database protection is added here for CREATE, but
`checkCanDropDatabase` (line 86) and `checkCanAlterDatabase` (line 93) have
**no** audit database check — they only call `checkDBNameInWrite` and the auth
privilege checker. A user with sufficient privileges could still DROP or ALTER
the `__audit` database through the table model path.
This is a pre-existing gap, but since the PR's goal is to protect `__audit`,
consider adding similar protection to those methods as well.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/schema/table/Audit.java:
##########
@@ -42,4 +44,15 @@ public static boolean includeByAuditTreeDB(PartialPath
prefixPath) {
String[] nodes = prefixPath.getNodes();
return nodes.length >= 2 &&
TABLE_MODEL_AUDIT_DATABASE.equalsIgnoreCase(nodes[1]);
}
+
+ public static boolean isAuditTreeDatabase(PartialPath databasePath) {
+ String[] nodes = databasePath.getNodes();
+ return nodes.length == 2
+ && ROOT.equalsIgnoreCase(nodes[0])
+ && TABLE_MODEL_AUDIT_DATABASE.equalsIgnoreCase(nodes[1]);
Review Comment:
**Case-sensitivity inconsistency with existing checks**
`isAuditTreeDatabase` correctly uses **case-insensitive** comparison
(`equalsIgnoreCase`), which is a good improvement. However, the existing
rejection paths still use **case-sensitive** checks:
- `visitDeleteDatabase` (TreeAccessCheckVisitor line 1046):
`TREE_MODEL_AUDIT_DATABASE.equals(prefixPath)` — case-sensitive
- `checkCreateOrAlterDatabasePermission` (TreeAccessCheckVisitor line 1070):
`TREE_MODEL_AUDIT_DATABASE_PATH.equals(databaseName)` — depends on
`PartialPath.equals`, likely case-sensitive
This means `CREATE DATABASE root.__AUDIT` is now properly blocked
(case-insensitive), but `DELETE DATABASE root.__AUDIT` or `ALTER DATABASE
root.__AUDIT` may bypass protection.
Consider refactoring the other paths to also use this new case-insensitive
helper.
--
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]
