Repository navigation
Fix the identity used by the asynchronous LOAD of the pipe receiver - #19029
luoluoyuyu wants to merge 12 commits into
Conversation
The load attributes of a TsFile received by the pipe receiver were built from the default user instead of from the sender of the transfer. The receiver now passes the authenticated sender to the active loader, and a seal request whose user can not be determined is rejected. The type mismatch converter inherits the identity of the load request. TsFiles placed directly in the active listening directory are still loaded with the default user. Tests: IoTDBPipeReceiverAsyncLoadAuthIT and IoTDBDataNodeReceiverTest.
Several places substituted the default user when a load carried no identity. LoadUtil, ActiveLoadTsFileLoader and the type mismatch converter now require the user of the load and fail otherwise, while a TsFile placed directly in the active listening directory still falls back to the default user. The worker session of the active loader is created after the per-file identity is known, and the thrift pipe receiver returns NOT_LOGIN instead of dereferencing a missing session. Tests: LoadUtilTest and IoTDBPipeReceiverAsyncLoadAuthIT.
The V2 handshake took the user id from the handshake parameters, and the receiver fell back to default credentials when the handshake carried none. The receiver now resolves the identity from the handshake user name with the local user manager or the permission manager, creates the audit entity only after a successful login, and clears it when the handshake fails or the connection exits. Two null pointer exceptions on the LOAD paths are fixed as well. Tests: IoTDBFileReceiverTest and IoTDBDataNodeReceiverAuditEntityTest.
A connection that was already authenticated kept the identity of its first transfer while the audit entity followed the new handshake. A handshake that does not match the user of the connection is now rejected with NO_PERMISSION, and the identity of a transfer is resolved from the verified user name, with the address of the connection preferred over the hostname of the handshake. Tests: IoTDBFileReceiverTest.
- Use "{}" placeholders in the new LOG_ templates and keep the en and zh
constants in parity.
- Fail closed when the session of a pipe connection is not logged in: an
unauthenticated session still carries the id of the super user.
- Resolve the user id of a load from its user name instead of trusting the
default id of the session, in the tree model schema verifier and in the
TsFile data type converter.
- Allow a local TsFile that is dropped into a sub directory of the active
listening directory to be loaded with the default user, while a file that
carries attributes but no user is still rejected.
The pipe receiver and the LOAD pipeline now fail when the user that executes a load can not be resolved. Keep the legacy default-user fallback by default and gate the strict behavior behind two internal switches instead of changing the existing load code: - pipe_receiver_require_user_identity (default false): an asynchronous LOAD of the pipe receiver without an authenticated sender is executed as the default user instead of being rejected. - load_require_user_identity (default false): an asynchronous LOAD without a user is queued with the default user instead of letting the strict LoadUtil check reject it. The switches are not added to iotdb-system.properties.template and therefore stay internal. ActiveLoadTsFileLoader, LoadTsFileDataTypeConverter, LoadUtil and TreeSchemaAutoCreatorAndVerifier keep the behavior of this branch. Tests: LoadUtilTest, ActiveLoadTsFileLoaderTest, IoTDBDataNodeReceiverTest and LoadTsFileAnalyzerTest.
Caideyipi
left a comment
Caideyipi
left a comment
There was a problem hiding this comment.
Reviewed the full diff at d6d29c5239e0c08f7bb03b61c556c5207f558c2e, including the latest identity switches. Two actionable compatibility/test issues are noted inline.
Validation: git diff --check passed. I checked the CI logs for the preceding revision 4547a819701: both tests in IoTDBPipeReceiverAsyncLoadAuthIT passed, while the pipe single-cluster job failed in the two legacy-receiver tests described below. Those affected paths are unchanged by the latest commit. I did not run tests locally; the separate remote test attempt did not reach Maven because fetching the source failed with a GitHub TLS error. The latest configuration changes therefore remain unverified by this review.
| if (Objects.nonNull(userName) && !userName.isEmpty()) { | ||
| return userName; | ||
| } | ||
| return attributes.isEmpty() && isInsideListeningDir(tsFile, pendingDir) |
There was a problem hiding this comment.
[P2] Preserve local active-load directories that carry LOAD options
A locally dropped file can also have recognized attribute directories, e.g. <listeningDir>/verify-false/1-0-0-0.tsfile or <listeningDir>/database-name-mydb/1-0-0-0.tsfile. parseAttributes returns a nonempty map without user for these paths, so this condition now returns null and loadTsFile moves the file into the failure directory. The base revision loaded these files with the default user. The presence of LOAD options does not establish that a file came from a remote sender, and neither new identity switch affects this branch. Please distinguish local inputs from receiver handoffs when requiring an explicit user, and cover a locally dropped file with attributes but no user in a regression test.
| // The identity of the sender is only set after a successful handshake or taken from a logged in | ||
| // session. There is deliberately no default user, because a missing identity must never degrade | ||
| // to a privileged one. | ||
| protected String username; |
There was a problem hiding this comment.
[P2] Update the legacy receiver tests for the removed default identity
A successful V1 handshake does not populate username, and recordPipeReceiverHandshake uses this field directly. Removing its default changes the reported user from root to Unknown, but IoTDBShowReceiversIT.hasOldSenderReceiverRuntime still requires SessionConfig.DEFAULT_USER. This already causes testOldSenderRuntimeShowsUnknownPipeIdsAndDisappearsAfterClientClose and testReceiverRuntimeClearedAfterDataNodeRestartAndCanReconnect to time out in the pipe single-cluster CI job for 4547a819701; this path is unchanged in the latest commit. Please update the legacy-handshake runtime/test expectations consistently with the new identity semantics and rerun those two cases, preserving the removal of privileged default credentials.
Reduce the diff of the configurable load identity requirement: - Move the LOAD switch back into LoadUtil so LoadTsFileAnalyzer keeps the behavior of this branch. - Read the two switches only when the properties are loaded; they are not hot reloadable. - Shorten the comments and javadocs of the new switches. ActiveLoadTsFileLoader, LoadTsFileDataTypeConverter, TreeSchemaAutoCreatorAndVerifier and the asynchronous LOAD acknowledgement semantics are unchanged. Tests: LoadUtilTest.
The load path already carries a locally resolved user and LoadUtil always queues a user when the strict switch is enabled, so the extra resolution layers are redundant: - TreeSchemaAutoCreatorAndVerifier uses the user of the load session again instead of resolving the id per measurement. - ActiveLoadTsFileLoader uses the user attribute with the default user as fallback, instead of walking the listening directory and failing files. - Remove the i18n constants that are no longer used. Tests: LoadUtilTest, ActiveLoadTsFileLoaderTest, IoTDBDataNodeReceiverTest, LoadTsFileAnalyzerTest.
LoadUtil only needs to use the user name of the session when it is present and fall back to the default user otherwise, which is what the original code does. Drop the strict switch and restore the original method. Remove the now unused i18n constant. Tests: LoadUtilTest, ActiveLoadTsFileLoaderTest, IoTDBDataNodeReceiverTest, LoadTsFileAnalyzerTest.
- pipe_receiver_require_user_identity: the asynchronous LOAD of the pipe receiver requires the authenticated sender to be the logged in user of the connection. - load_require_user_identity: a LOAD without a user requires a logged in session that carries a user name instead of falling back to the default user. Both switches keep the legacy fallback when they are disabled. Tests: LoadUtilTest, ActiveLoadTsFileLoaderTest, IoTDBDataNodeReceiverTest, LoadTsFileAnalyzerTest.
- A V1 handshake carries no credentials, so the receiver runtime reports an unknown user instead of the default user. Update the legacy receiver expectation accordingly, keeping the removal of privileged defaults. - Add a regression test for a locally dropped TsFile under an attribute directory without a user; such a local input must still be loaded with the default user.
Caideyipi
left a comment
Caideyipi
left a comment
There was a problem hiding this comment.
Re-reviewed the full current diff and relevant context at 8e1286d. The earlier local active-load compatibility and legacy receiver-runtime findings are addressed, with regression coverage. The asynchronous LOAD now carries the authenticated sender, and the conversion session resolves the user identity locally. No further actionable issues found.
Validation: git diff --check passed. The latest Linux datanode and other unit-test jobs, pipe single-cluster job, and zh-locale compile are green. Some other CI jobs are still running or queued, so this approval does not imply that the full CI suite has finished. No tests were run locally in this review.
|
Here is a design concern about the security boundary of this change. The security invariant should be that every remote Pipe LOAD is executed with an identity authenticated and resolved by the receiver, and that the same identity is carried through the asynchronous handoff, active loader, and type-mismatch conversion. The current implementation still makes this conditional through the two default-disabled switches ( I recommend separating the two sources at the API boundary:
The identity should be represented as one immutable receiver-side context passed through the whole async pipeline, rather than being reconstructed independently by The asynchronous acknowledgement semantics also need to be documented and tested: a successful seal response currently means that the file was accepted into the background queue, not that authorization and loading succeeded. The task status, retry behavior, and the effect of permissions revoked after enqueue should be explicit. I would add coverage for table-model loads, type-mismatch conversion, missing/deleted users, permission revocation after enqueue, and both identity switches in combination with local file drops. |
Enable pipe_receiver_require_user_identity by default: the receiver only queues an asynchronous LOAD when the sender is authenticated and the connection is logged in as that user. Set the switch to false to restore the legacy default user fallback. The load_require_user_identity switch keeps its default (false). Tests: LoadUtilTest, ActiveLoadTsFileLoaderTest, IoTDBDataNodeReceiverTest, LoadTsFileAnalyzerTest.
Description
This PR has:
for an unfamiliar reader.
for code coverage.
Key changed/added classes (or packages if there are too many classes) in this PR