feat(mac): sod mandatory and mac[manual-only] #35121
Conversation
kailixu
requested review from
a team,
dapan1121,
guanshengliang and
zitsen
as code owners
There was a problem hiding this comment.
Pull request overview
This PR introduces enterprise security features around Separation of Duties (SoD) enforcement and Mandatory Access Control (MAC) security levels, and exposes the new “security policies” state via SQL/shell and information_schema system tables.
Changes:
- Add
SHOW SECURITY_POLICIESend-to-end (tokenizer/grammar/AST/auth/meta cache/mnode retrieval + shell autocomplete/help). - Add
security_levelsupport for DB/table options and user min/max security levels, propagated through RPC/message structs and vnode/mnode configs. - Add CI coverage for the new DAC/MAC/SoD behavior via a new privileges test case.
Reviewed changes
Copilot reviewed 60 out of 60 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/shell/src/shellAuto.c | Adds shell autocomplete/help entry for show security_policies. |
| test/ci/cases.task | Adds the new DAC/MAC privileges test to CI execution list. |
| test/cases/25-Privileges/test_priv_rbac.py | Updates test metadata (Jira reference). |
| test/cases/25-Privileges/test_priv_dac_mac.py | New test covering SoD + MAC security level behaviors. |
| source/util/src/terror.c | Adds MAC-related error codes/messages and SoD-related role-status error messages. |
| source/libs/parser/src/parTranslater.c | Wires new show adapter + translates DB/table/user security level options into requests. |
| source/libs/parser/src/parTokenizer.c | Adds SECURITY_LEVEL / SECURITY_POLICIES keywords. |
| source/libs/parser/src/parAuthenticator.c | Adds auth checks for SHOW SECURITY_POLICIES and SoD-phase restrictions. |
| source/libs/parser/src/parAstParser.c | Adds meta-cache collection for SHOW SECURITY_POLICIES. |
| source/libs/parser/src/parAstCreater.c | Adds AST option parsing/merging for DB/table/user security level options. |
| source/libs/parser/inc/sql.y | Adds grammar for user/db/table security level options and SHOW SECURITY_POLICIES. |
| source/libs/parser/inc/parAst.h | Extends DB/table option enums with SECURITY_LEVEL. |
| source/libs/nodes/src/nodesUtilFuncs.c | Adds node creation/destroy handling for the new show statement type and user security list cleanup. |
| source/libs/nodes/src/nodesCodeFuncs.c | Adds node name + JSON (de)serialization mapping for SHOW SECURITY_POLICIES. |
| source/libs/executor/src/sysscanoperator.c | Repurposes privInfo bitfields to carry min/max security levels. |
| source/libs/command/src/command.c | Adds ALLOW_DROP / SECURITY_LEVEL to SHOW CREATE outputs. |
| source/dnode/vnode/src/vnd/vnodeSvr.c | Applies securityLevel changes during vnode alter-config. |
| source/dnode/vnode/src/vnd/vnodeCfg.c | Persists vnode securityLevel to/from JSON with defaults. |
| source/dnode/vnode/inc/vnode.h | Adds securityLevel to SVnodeCfg. |
| source/dnode/mnode/impl/src/mndVgroup.c | Propagates DB securityLevel into create/alter vnode config requests. |
| source/dnode/mnode/impl/src/mndUser.c | Adds user min/max security levels and implements SoD enforcement restrictions/check callbacks. |
| source/dnode/mnode/impl/src/mndTrans.c | Registers new SoD-related transaction stop callbacks. |
| source/dnode/mnode/impl/src/mndStreamTrans.c | Adjusts commit-raw freeing behavior on status set failure. |
| source/dnode/mnode/impl/src/mndStb.c | Adds stable securityLevel retrieval/show support. |
| source/dnode/mnode/impl/src/mndShow.c | Maps ins_security_policies to the corresponding mgmt show type. |
| source/dnode/mnode/impl/src/mndProfile.c | Returns min/max security levels and SoD-initial state in connect response. |
| source/dnode/mnode/impl/src/mndPrivilege.c | Extends user auth response with min/max security levels. |
| source/dnode/mnode/impl/src/mndMain.c | Adds SoD enforcement-at-start support and SoD phase getters/setters. |
| source/dnode/mnode/impl/src/mndDnode.c | Adjusts commit-raw freeing behavior during dnode info update transaction. |
| source/dnode/mnode/impl/src/mndDb.c | Adds DB versioning and securityLevel to DB cfg validation/persistence/show. |
| source/dnode/mnode/impl/src/mndCluster.c | Adds cluster SoD/MAC policy state persistence and SHOW SECURITY_POLICIES retrieval; CLI enforcement flow. |
| source/dnode/mnode/impl/inc/mndUser.h | Exposes mndCheckManagementRoleStatus prototype. |
| source/dnode/mnode/impl/inc/mndTrans.h | Adds SoD stop callback function identifiers. |
| source/dnode/mnode/impl/inc/mndInt.h | Stores SoD phase in SMnode and exposes phase accessors. |
| source/dnode/mnode/impl/inc/mndDef.h | Extends cluster/user/role/db/stb structs with SoD/MAC/security level fields/bitfields. |
| source/dnode/mnode/impl/inc/mndCluster.h | Exposes cluster SoD-mode APIs and stop callbacks. |
| source/dnode/mgmt/mgmt_vnode/src/vmHandle.c | Propagates vnode securityLevel through vnode management flows and logs. |
| source/dnode/mgmt/mgmt_mnode/src/mmHandle.c | Registers handler for TDMT_MND_CONFIG_CLUSTER_RSP. |
| source/dnode/mgmt/exe/dmMain.c | Adds --SoD=mandatory CLI flag and help text. |
| source/common/src/tpriv.c | Updates privilege info table (notably removes an XNode task entry). |
| source/common/src/tglobal.c | Adds global tsSodEnforceMode. |
| source/common/src/systable.c | Adds sec_level/sec_levels columns and new ins_security_policies table schema. |
| source/common/src/msg/tmsg.c | Adds serialization/deserialization for new security-level fields and SoD flags. |
| source/client/src/clientStmt2.c | Passes SoD-initial state into async parse context. |
| source/client/src/clientMsgHandler.c | Stores SoD-initial and min/max security levels from connect response. |
| source/client/src/clientMain.c | Passes SoD-initial state into parse context. |
| source/client/src/clientImpl.c | Passes min/max security levels into planner contexts. |
| source/client/src/clientHb.c | Updates client cached enable/SoD/security-level fields from auth heartbeat. |
| source/client/src/clientEnv.c | Initializes enable and rejects requests when user is disabled client-side. |
| source/client/inc/clientInt.h | Adds packed flags for min/max security levels, enable, and SoD-initial state. |
| include/util/tpriv.h | Defines SoD phases, adds PRIV_SECURITY_POLICIES_SHOW, bumps priv table version. |
| include/util/tdef.h | Adds default/min/max security level constants + security level enum. |
| include/util/taoserror.h | Adds new MAC and SoD-related error code definitions. |
| include/libs/planner/planner.h | Updates privInfo bitfields to min/max security levels. |
| include/libs/parser/parser.h | Updates parse context bitfields for SoD-initial and min/max security levels. |
| include/libs/nodes/plannodes.h | Updates system table scan node privInfo bitfields to min/max security levels. |
| include/libs/nodes/cmdnodes.h | Adds DB/table/user option fields for security levels. |
| include/common/tmsg.h | Adds/extends message structs for security levels and SoD flags and new show type/node type. |
| include/common/tglobal.h | Declares tsSodEnforceMode. |
| include/common/systable.h | Adds ins_security_policies table name constant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TAOS_CHECK_EXIT(tEncodeI8(&encoder, pReq->allowDrop)); | ||
| TAOS_CHECK_EXIT(tEncodeI8(&encoder, pReq->secureDelete)); |
There was a problem hiding this comment.
In tSerializeSCreateDbReq, the new fields are encoded in the order isAudit, allowDrop, secureDelete, cacheLastShardBits, securityLevel, but tDeserializeSCreateDbReq decodes secureDelete before allowDrop. This will shift the decode stream (e.g., secureDelete will read the encoded allowDrop), breaking CREATE DATABASE request compatibility. Align the encode/decode order (and if backward compatibility is required, only append new fields at the end and decode them conditionally in the same order).
| TAOS_CHECK_EXIT(tEncodeI8(&encoder, pReq->allowDrop)); | |
| TAOS_CHECK_EXIT(tEncodeI8(&encoder, pReq->secureDelete)); | |
| TAOS_CHECK_EXIT(tEncodeI8(&encoder, pReq->secureDelete)); | |
| TAOS_CHECK_EXIT(tEncodeI8(&encoder, pReq->allowDrop)); |
| if (!tDecodeIsEnd(&decoder)) { | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->allowDrop)); | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->securityLevel)); | ||
| } else { | ||
| pReq->allowDrop = pReq->isAudit ? 0 : 1; | ||
| pReq->securityLevel = TSDB_DEFAULT_SECURITY_LEVEL; | ||
| } |
There was a problem hiding this comment.
tDeserializeSCreateDbReq decodes allowDrop and securityLevel only at the tail, but the serializer currently writes allowDrop before secureDelete. As written, this tail decode does not actually restore alignment/backward-compat; it will read the wrong bytes. Decode the fields in the same sequence they are encoded (or move the new encodes to the tail and keep the conditional decode here).
| if (!tDecodeIsEnd(&decoder)) { | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->securityLevel) < 0); | ||
| } else { |
There was a problem hiding this comment.
The new securityLevel decode uses TAOS_CHECK_EXIT(tDecodeI8(...) < 0), but TAOS_CHECK_EXIT expects an error code (<0) and assigns code = (CMD). With the < 0 comparison, code becomes 0/1 and decode failures will be silently ignored. Pass the decoder return value directly (like other decode calls) so errors propagate correctly.
| int32_t mndSetUserAuthRsp(SMnode *pMnode, SUserObj *pUser, SGetUserAuthRsp *pRsp) { | ||
| memcpy(pRsp->user, pUser->user, TSDB_USER_LEN); | ||
| pRsp->superAuth = 1; | ||
| pRsp->enable = pUser->enable; | ||
| pRsp->sysInfo = pUser->sysInfo; | ||
| pRsp->version = pUser->authVersion; | ||
| pRsp->passVer = pUser->passVersion; | ||
| pRsp->whiteListVer = pMnode->ipWhiteVer; | ||
| pRsp->userId = pUser->uid; | ||
| pRsp->minSecLevel = TSDB_MIN_SECURITY_LEVEL; | ||
| pRsp->maxSecLevel = TSDB_MAX_SECURITY_LEVEL; | ||
|
|
There was a problem hiding this comment.
mndSetUserAuthRsp always sets minSecLevel/maxSecLevel to the global min/max constants instead of the user's configured levels. This will cause clients to receive (and heartbeat-update) incorrect security levels for non-default users. Populate these fields from pUser->minSecLevel / pUser->maxSecLevel to keep auth info consistent with the connect response and user metadata.
| alter_db_option(A) ::= ALLOW_DROP NK_INTEGER(B). { A.type = DB_OPTION_ALLOW_DROP; A.val = B; } | ||
| alter_db_option(A) ::= SECURE_DELETE NK_INTEGER(B). { A.type = DB_OPTION_SECURE_DELETE; A.val = B; } | ||
| alter_db_option(A) ::= SECURITY_LEVEL(B). { A.type = DB_OPTION_SECURITY_LEVEL; A.val = B; } | ||
|
|
There was a problem hiding this comment.
alter_db_option(A) ::= SECURITY_LEVEL(B). captures only the SECURITY_LEVEL token, but setAlterDatabaseOption expects a numeric value token (like other options do with NK_INTEGER). This looks like a grammar bug and likely won’t parse/serialize the option correctly. Update the production to include the value token (e.g., SECURITY_LEVEL NK_INTEGER(...)) consistent with the other alter-db options.
| static int32_t collectMetaKeyFromShowSecurityPoliciesStmt(SCollectMetaKeyCxt* pCxt, SShowStmt* pStmt) { | ||
| return reserveTableMetaInCache(pCxt->pParseCxt->acctId, TSDB_INFORMATION_SCHEMA_DB, TSDB_INS_TABLE_SECURITY_POLICIES, | ||
| pCxt->pMetaCache); | ||
| } |
There was a problem hiding this comment.
collectMetaKeyFromShowSecurityPoliciesStmt is a new static function but is never referenced (the switch uses collectMetaKeyFromShowSecurityPolicies). With typical warnings-as-errors, an unused static function can fail the build. Remove the unused helper or wire it into the switch to avoid dead code.
| {PRIV_PERF_SCHEMA_READ_PRIVILEGED, PRIV_CATEGORY_SYSTEM, 0, 0, SYS_ADMIN_INFO1_ROLES, 0, "", | ||
| "READ PERFORMANCE SCHEMA PRIVILEGED"}, | ||
| {PRIV_GRANTS_SHOW, PRIV_CATEGORY_SYSTEM, 0, 0, SYS_ADMIN_INFO1_ROLES, 0, "", "SHOW GRANTS"}, | ||
| {PRIV_CLUSTER_SHOW, PRIV_CATEGORY_SYSTEM, 0, 0, SYS_ADMIN_INFO1_ROLES, 0, "", "SHOW CLUSTER"}, | ||
| {PRIV_APPS_SHOW, PRIV_CATEGORY_SYSTEM, 0, 0, SYS_ADMIN_INFO_ROLES, 0, "", "SHOW APPS"}, | ||
| // Xnode Task Management | ||
| {PRIV_XNODE_TASK_CREATE, PRIV_CATEGORY_SYSTEM, 0, 0, T_ROLE_SYSDBA, 0, "", "CREATE XNODE TASK"}, | ||
|
|
There was a problem hiding this comment.
The privilege table (privInfoTable) no longer contains an entry for PRIV_XNODE_TASK_CREATE, even though the enum still defines it. This can break privilege resolution for XNode task creation. Also, a new PRIV_SECURITY_POLICIES_SHOW enum was added but there is no corresponding entry here, so SHOW SECURITY_POLICIES privilege checks may not behave correctly. Add/update the appropriate privInfoTable entries (and keep PRIV_INFO_TABLE_VERSION aligned with the table contents).
| from new_test_framework.utils import tdLog, tdSql, tdDnodes, etool, TDSetSql | ||
| from new_test_framework.utils.sqlset import TDSetSql |
There was a problem hiding this comment.
The new test imports TDSetSql twice (from new_test_framework.utils and new_test_framework.utils.sqlset). This is redundant and can be confusing about the source of the symbol. Import TDSetSql from a single module and remove the duplicate import to keep the test module clean.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for mandatory Separation of Duties (SoD) and Mandatory Access Control (MAC) with security levels. Key changes include new CLI parameters, SQL syntax for security policies, and logic to enforce management role distribution. Several critical issues were identified during the review, including compilation errors due to incorrect field names in SUserObj, a logic bug in management role validation that fails when a user holds multiple roles, and an incorrect error-checking pattern in the message decoding logic. Additionally, feedback was provided regarding inconsistent transaction policies and missing return codes for asynchronous operations.
| TAOS_CHECK_GOTO(mndCheckManagementRoleStatus(pMnode, pUser->user, 0), &lino, _OVER); | ||
| } | ||
| } else { | ||
| if ((strncmp(pUser->name, TSDB_DEFAULT_USER, TSDB_USER_LEN) == 0) && |
There was a problem hiding this comment.
The field name does not exist in the SUserObj structure; it should be user. This will cause a compilation error.
if ((strncmp(pUser->user, TSDB_DEFAULT_USER, TSDB_USER_LEN) == 0) &&| if (taosHashGet(pUser->roles, TSDB_ROLE_SYSAUDIT_LOG, len) != NULL) { | ||
| if (taosHashGet(pUser->roles, TSDB_ROLE_SYSAUDIT_LOG, sizeof(TSDB_ROLE_SYSAUDIT_LOG)) != NULL) { | ||
| (void)taosThreadRwlockWrlock(&userCache.rw); | ||
| (void)tsnprintf(userCache.auditLogUser, TSDB_USER_LEN, "%s", pUser->name); |
There was a problem hiding this comment.
The field name does not exist in the SUserObj structure; it should be user. This will cause a compilation error.
(void)tsnprintf(userCache.auditLogUser, TSDB_USER_LEN, "%s", pUser->user);| if ((foundRoles & T_ROLE_SYSDBA) == 0 && taosHashGet(pUser->roles, TSDB_ROLE_SYSDBA, sizeof(TSDB_ROLE_SYSDBA))) { | ||
| foundRoles |= T_ROLE_SYSDBA; | ||
| } else if ((foundRoles & T_ROLE_SYSSEC) == 0 && | ||
| taosHashGet(pUser->roles, TSDB_ROLE_SYSSEC, sizeof(TSDB_ROLE_SYSSEC))) { | ||
| foundRoles |= T_ROLE_SYSSEC; | ||
| } else if ((foundRoles & T_ROLE_SYSAUDIT) == 0 && | ||
| taosHashGet(pUser->roles, TSDB_ROLE_SYSAUDIT, sizeof(TSDB_ROLE_SYSAUDIT))) { | ||
| foundRoles |= T_ROLE_SYSAUDIT; | ||
| } |
There was a problem hiding this comment.
The use of else if here is a logic bug. If a single user has multiple management roles (e.g., both SYSDBA and SYSSEC), this loop will only mark the first one found as present for that user and skip the others. Each role should be checked independently for every user to ensure the foundRoles bitmask is correctly populated.
if ((foundRoles & T_ROLE_SYSDBA) == 0 && taosHashGet(pUser->roles, TSDB_ROLE_SYSDBA, sizeof(TSDB_ROLE_SYSDBA))) {
foundRoles |= T_ROLE_SYSDBA;
}
if ((foundRoles & T_ROLE_SYSSEC) == 0 &&
taosHashGet(pUser->roles, TSDB_ROLE_SYSSEC, sizeof(TSDB_ROLE_SYSSEC))) {
foundRoles |= T_ROLE_SYSSEC;
}
if ((foundRoles & T_ROLE_SYSAUDIT) == 0 &&
taosHashGet(pUser->roles, TSDB_ROLE_SYSAUDIT, sizeof(TSDB_ROLE_SYSAUDIT))) {
foundRoles |= T_ROLE_SYSAUDIT;
}| pReq->cacheLastShardBits = -1; | ||
| } | ||
| if (!tDecodeIsEnd(&decoder)) { | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->securityLevel) < 0); |
There was a problem hiding this comment.
The condition < 0 inside TAOS_CHECK_EXIT is inconsistent with the standard pattern in this file and potentially incorrect. tDecodeI8 returns 0 on success and a non-zero error code on failure. If it returns a positive error code, error_code < 0 will evaluate to 0, and the error will not be caught. It should be called directly.
TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->securityLevel));| static int32_t mndAlterUserEx(SMnode *pMnode, SUserObj *pNew, SRpcMsg *pReq, ETrnFunc stopFunc) { | ||
| int32_t code = 0, lino = 0; | ||
| STrans *pTrans = mndTransCreate(pMnode, TRN_POLICY_ROLLBACK, TRN_CONFLICT_ROLE, pReq, "alter-user"); | ||
| STrans *pTrans = mndTransCreate(pMnode, TRN_POLICY_RETRY, TRN_CONFLICT_ROLE, pReq, "alter-user"); |
There was a problem hiding this comment.
The transaction policy for alter user was changed from TRN_POLICY_ROLLBACK to TRN_POLICY_RETRY. For user-initiated synchronous SQL commands, ROLLBACK is generally preferred to provide immediate feedback on conflicts. Please verify if this change was intentional.
| if (code < 0 && code != TSDB_CODE_ACTION_IN_PROGRESS) { | ||
| mError("%s failed at line %d since %s", __func__, lino, tstrerror(code)); | ||
| } | ||
| TAOS_RETURN(code); |
There was a problem hiding this comment.
This function should return TSDB_CODE_ACTION_IN_PROGRESS on success to indicate that the configuration change is being processed asynchronously via a transaction, consistent with other management operations.
TAOS_RETURN(TSDB_CODE_ACTION_IN_PROGRESS);
kailixu
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 60 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!tDecodeIsEnd(&decoder)) { | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->securityLevel) < 0); | ||
| } else { | ||
| pReq->securityLevel = TSDB_DEFAULT_SECURITY_LEVEL; |
There was a problem hiding this comment.
In tDeserializeSAlterVnodeConfigReq, the new decode uses TAOS_CHECK_EXIT(tDecodeI8(...) < 0). TAOS_CHECK_EXIT expects an error-code return value, but the < 0 comparison turns it into a boolean and prevents proper error handling on decode failure. Please pass the tDecodeI8(...) return code directly (and then validate securityLevel separately if needed).
| } | ||
| if (!tDecodeIsEnd(&decoder)) { | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->minSecLevel)); | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->maxSecLevel)); |
There was a problem hiding this comment.
In tDeserializeSCreateUserReq, when decoding an older message (decoder ends before the new fields), minSecLevel/maxSecLevel remain at their zero-initialized values. That changes the default security-level range for older clients to [0,0] instead of the intended defaults (TSDB_DEFAULT_USER_MIN_SECURITY_LEVEL/TSDB_DEFAULT_USER_MAX_SECURITY_LEVEL). Please set explicit defaults in the else branch when the fields are absent for backward compatibility.
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->maxSecLevel)); | |
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->maxSecLevel)); | |
| } else { | |
| pReq->minSecLevel = TSDB_DEFAULT_USER_MIN_SECURITY_LEVEL; | |
| pReq->maxSecLevel = TSDB_DEFAULT_USER_MAX_SECURITY_LEVEL; |
|
|
||
| class TestCase: | ||
|
|
||
| test_pass = "Passsword_123!" |
There was a problem hiding this comment.
test_pass value looks like a typo: "Passsword_123!" (three 's'). If unintentional, it makes the test harder to read/debug. Consider correcting it to Password_123!.
| /* Phase 1: Initial: Bootstrapped by CLI 'taosd --sod=mandatory', but SoD requirements are not yet met. | ||
| * Awaiting initial role assignment. | ||
| * Operations restricted to whitelist: CREATE/DROP/ALTER USER, GRANT/REVOKE ROLE, SHOW USERS, SHOW SECURITY_POLICIES. | ||
| */ | ||
| #define TSDB_SOD_PHASE_INITIAL 1 | ||
|
|
||
| /* Phase 2: Enforcing. Triggered by SQL command 'ALTER CLUSTER ... MANDATORY'. |
There was a problem hiding this comment.
SoD phase comments are out of sync with the actual interfaces added in this PR: the CLI flag implemented is --SoD=mandatory (case-sensitive) and the SQL path uses ALTER CLUSTER 'sod' 'mandatory' / ALTER CLUSTER 'separation_of_duties' 'mandatory' rather than the generic 'ALTER CLUSTER ... MANDATORY'. Please update these comments to match the real command syntax to avoid misleading operators/devs.
| /* Phase 1: Initial: Bootstrapped by CLI 'taosd --sod=mandatory', but SoD requirements are not yet met. | |
| * Awaiting initial role assignment. | |
| * Operations restricted to whitelist: CREATE/DROP/ALTER USER, GRANT/REVOKE ROLE, SHOW USERS, SHOW SECURITY_POLICIES. | |
| */ | |
| #define TSDB_SOD_PHASE_INITIAL 1 | |
| /* Phase 2: Enforcing. Triggered by SQL command 'ALTER CLUSTER ... MANDATORY'. | |
| /* Phase 1: Initial: Bootstrapped by CLI 'taosd --SoD=mandatory', but SoD requirements are not yet met. | |
| * Awaiting initial role assignment. | |
| * Operations restricted to whitelist: CREATE/DROP/ALTER USER, GRANT/REVOKE ROLE, SHOW USERS, SHOW SECURITY_POLICIES. | |
| */ | |
| #define TSDB_SOD_PHASE_INITIAL 1 | |
| /* Phase 2: Enforcing. Triggered by SQL command 'ALTER CLUSTER 'sod' 'mandatory'' or | |
| * 'ALTER CLUSTER 'separation_of_duties' 'mandatory''. |
| {.name = "allowed_host", .bytes = TSDB_PRIVILEDGE_HOST_LEN + VARSTR_HEADER_SIZE, .type = TSDB_DATA_TYPE_VARCHAR, .sysInfo = true}, | ||
| {.name = "allowed_datetime", .bytes = TSDB_PRIVILEDGE_HOST_LEN + VARSTR_HEADER_SIZE, .type = TSDB_DATA_TYPE_VARCHAR, .sysInfo = true}, | ||
| {.name = "roles", .bytes = TSDB_MAX_SUBROLE * TSDB_ROLE_LEN + VARSTR_HEADER_SIZE, .type = TSDB_DATA_TYPE_VARCHAR, .sysInfo = true}, | ||
| {.name = "sec_levels", .bytes = 3 + VARSTR_HEADER_SIZE, .type = TSDB_DATA_TYPE_BINARY, .sysInfo = true}, |
There was a problem hiding this comment.
ins_users.sec_levels schema size looks too small: bytes = 3 + VARSTR_HEADER_SIZE, but mndRetrieveUsers formats the value as "[%d,%d]" (e.g. [0,4], length 5). This mismatch can cause SHOW USERS / information_schema.ins_users retrieval to fail (or truncate) when colDataSetVal validates against schema bytes. Please increase the column size to at least 5 + VARSTR_HEADER_SIZE (or a slightly larger bound).
| {.name = "sec_levels", .bytes = 3 + VARSTR_HEADER_SIZE, .type = TSDB_DATA_TYPE_BINARY, .sysInfo = true}, | |
| {.name = "sec_levels", .bytes = 5 + VARSTR_HEADER_SIZE, .type = TSDB_DATA_TYPE_BINARY, .sysInfo = true}, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 65 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // MAC NRU: user.maxSecLevel must be >= table.securityLevel for SELECT | ||
| uError("MAC-DEBUG authSelect: table=%s.%s secLvl=%d userMax=%d userMin=%d flag=0x%02x", | ||
| pTable->dbName, pTable->tableName, | ||
| (int)pTableMeta->secLvl, (int)pAuthCxt->pParseCxt->maxSecLevel, | ||
| (int)pAuthCxt->pParseCxt->minSecLevel, (unsigned)pTableMeta->flag); | ||
| if (pTableMeta->secLvl > 0 && pAuthCxt->pParseCxt->maxSecLevel < pTableMeta->secLvl) { |
There was a problem hiding this comment.
The new uError("MAC-DEBUG ...") log in authSelectImpl will emit an ERROR-level message on every successful SELECT that hits table meta lookup. This is likely to flood logs and impact performance in production. Please remove it or gate it behind a debug flag / trace-level logging (e.g., uDebug) and avoid logging per-row/per-query hot paths at error severity.
| } | ||
| if (!tDecodeIsEnd(&decoder)) { | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->minSecLevel)); | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->maxSecLevel)); |
There was a problem hiding this comment.
tDeserializeSCreateUserReq conditionally decodes minSecLevel/maxSecLevel when more fields exist, but the else case does not initialize defaults. If an older client sends a shorter payload, these fields may remain uninitialized and later propagate into user objects/auth responses. Set explicit defaults (e.g., TSDB_DEFAULT_USER_MIN_SECURITY_LEVEL / TSDB_DEFAULT_USER_MAX_SECURITY_LEVEL) when the fields are absent.
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->maxSecLevel)); | |
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->maxSecLevel)); | |
| } else { | |
| pReq->minSecLevel = TSDB_DEFAULT_USER_MIN_SECURITY_LEVEL; | |
| pReq->maxSecLevel = TSDB_DEFAULT_USER_MAX_SECURITY_LEVEL; |
| pReq->ssCompact = TSDB_DEFAULT_SS_COMPACT; | ||
| if (!tDecodeIsEnd(&decoder)) { | ||
| TAOS_CHECK_EXIT(tDecodeI32(&decoder, &pReq->ssKeepLocal) < 0); | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->ssCompact) < 0); | ||
| } | ||
| if (!tDecodeIsEnd(&decoder)) { | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->allowDrop) < 0); | ||
| } else { | ||
| pReq->allowDrop = TSDB_DEFAULT_DB_ALLOW_DROP; | ||
| } | ||
| if (!tDecodeIsEnd(&decoder)) { | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->secureDelete)); | ||
| } else { | ||
| pReq->secureDelete = TSDB_DEFAULT_DB_SECURE_DELETE; | ||
| } | ||
|
|
||
| if (!tDecodeIsEnd(&decoder)) { | ||
| TAOS_CHECK_EXIT(tDecodeI32(&decoder, &pReq->cacheLastShardBits)); | ||
| } else { | ||
| pReq->cacheLastShardBits = -1; | ||
| } | ||
| if (!tDecodeIsEnd(&decoder)) { | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->securityLevel) < 0); | ||
| } else { | ||
| pReq->securityLevel = TSDB_DEFAULT_SECURITY_LEVEL; |
There was a problem hiding this comment.
In tDeserializeSAlterVnodeConfigReq, several decode calls are wrapped as TAOS_CHECK_EXIT(tDecode... < 0). Because TAOS_CHECK_EXIT only branches on code < TSDB_CODE_SUCCESS, a decode failure (negative return) becomes 1 after < 0, which is treated as success and silently ignores the decode error. Pass the decode return code directly to TAOS_CHECK_EXIT (without < 0) so failures propagate correctly (and preserve the original error code).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 71 out of 71 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| len += tsnprintf(buf2 + VARSTR_HEADER_SIZE, SHOW_CREATE_DB_RESULT_FIELD2_LEN - VARSTR_HEADER_SIZE, | ||
| "CREATE DATABASE `%s` BUFFER %d CACHESIZE %d CACHEMODEL '%s' CACHESHARDBITS %d COMP %d DURATION %s " | ||
| "WAL_FSYNC_PERIOD %d MAXROWS %d MINROWS %d STT_TRIGGER %d KEEP %s,%s,%s PAGES %d PAGESIZE %d " | ||
| "PRECISION '%s' REPLICA %d " | ||
| "WAL_LEVEL %d VGROUPS %d SINGLE_STABLE %d TABLE_PREFIX %d TABLE_SUFFIX %d TSDB_PAGESIZE %d " | ||
| "WAL_RETENTION_PERIOD %d WAL_RETENTION_SIZE %" PRId64 | ||
| " KEEP_TIME_OFFSET %d ENCRYPT_ALGORITHM '%s' SS_CHUNKPAGES %d SS_KEEPLOCAL %dm SS_COMPACT %d " | ||
| "COMPACT_INTERVAL %s COMPACT_TIME_RANGE %s,%s COMPACT_TIME_OFFSET %" PRIi8 "h IS_AUDIT %d SECURE_DELETE %d", | ||
| "COMPACT_INTERVAL %s COMPACT_TIME_RANGE %s,%s COMPACT_TIME_OFFSET %" PRIi8 "h IS_AUDIT %d SECURE_DELETE %d ALLOW_DROP %d SECURITY_LEVEL %d", | ||
| dbName, pCfg->buffer, pCfg->cacheSize, cacheModelStr(pCfg->cacheLast), pCfg->cacheShardBits, pCfg->compression, |
There was a problem hiding this comment.
SHOW CREATE DATABASE output now appends SECURITY_LEVEL %d, but in this PR CREATE DATABASE does not support a SECURITY_LEVEL option (only ALTER DATABASE ... SECURITY_LEVEL is added). This makes the generated DDL non-replayable. Either extend CREATE DATABASE grammar/translation to accept SECURITY_LEVEL, or omit it from the SHOW CREATE statement.
| int32_t nodeType = nodeType(pQuery->pRoot); | ||
| if (nodeType == QUERY_NODE_SELECT_STMT) { | ||
| SSelectStmt* pSelect = (SSelectStmt*)pQuery->pRoot; | ||
| STableNode* pTable = (STableNode*)(pSelect->pFromTable); | ||
| if (QUERY_NODE_REAL_TABLE != nodeType(pTable) || !IS_INFORMATION_SCHEMA_DB(pTable->dbName) || | ||
| (strcmp(pTable->tableName, TSDB_INS_TABLE_USERS) != 0)) { | ||
| return TSDB_CODE_MND_SOD_RESTRICTED; |
There was a problem hiding this comment.
In authenticate(), the local variable nodeType shadows the nodeType() function. After the declaration, calls like nodeType(pTable) will fail to compile because nodeType is now an int32_t variable. Rename the variable (e.g., rootType) and keep using nodeType(...) for the function calls.
| if (!tDecodeIsEnd(&decoder)) { | ||
| TAOS_CHECK_EXIT(tDecodeI8(&decoder, &pReq->securityLevel) < 0); | ||
| } else { |
There was a problem hiding this comment.
tDeserializeSAlterVnodeConfigReq() passes a boolean expression into TAOS_CHECK_EXIT (tDecodeI8(...) < 0). This causes incorrect error handling and can bypass decode failures or treat successful decodes as errors depending on the macro semantics. Call tDecodeI8(...) directly and let TAOS_CHECK_EXIT check its return code.
| appendTagFields(buf2, &len, pCfg); | ||
| len += | ||
| snprintf(buf2 + VARSTR_HEADER_SIZE + len, SHOW_CREATE_TB_RESULT_FIELD2_LEN - (VARSTR_HEADER_SIZE + len), ")"); | ||
| len += snprintf(buf2 + VARSTR_HEADER_SIZE + len, SHOW_CREATE_TB_RESULT_FIELD2_LEN - (VARSTR_HEADER_SIZE + len), | ||
| ") SECURITY_LEVEL %d", pCfg->securityLevel); | ||
| appendTableOptions(buf2, &len, pDbCfg, pCfg); |
There was a problem hiding this comment.
SHOW CREATE STABLE output now includes SECURITY_LEVEL %d, but the parser/translator comments in this PR indicate CREATE STB does not support security_level (it is only changeable via ALTER). Emitting unsupported syntax makes SHOW CREATE output non-replayable. Either add SECURITY_LEVEL support to CREATE STABLE syntax, or omit it from the generated statement (or move it into a form that is accepted).
| static bool sysTableMacVisible(const SSysTableScanInfo* pInfo, int32_t tableType, int32_t secLevel, | ||
| int32_t dbSecLevel) { | ||
| if (pInfo == NULL) { | ||
| return true; | ||
| } | ||
|
|
||
| if (tableType == TSDB_NORMAL_TABLE || tableType == TSDB_VIRTUAL_NORMAL_TABLE) { | ||
| secLevel = dbSecLevel; | ||
| } | ||
|
|
||
| if (secLevel <= 0) { | ||
| return true; | ||
| } | ||
|
|
||
| return pInfo->maxSecLevel >= secLevel; | ||
| } |
There was a problem hiding this comment.
sysTableMacVisible() filters information_schema table listings purely based on maxSecLevel, but it has no check for whether cluster MAC is actually activated. Since DB/STB security levels can be non-zero even before MAC activation, this can hide objects even when MAC is meant to be inactive. Consider bypassing this filter when MAC is inactive (e.g., propagate macActive into the scan node/privInfo or set maxSecLevel to the top level when MAC is inactive).
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.