Re: [PR] Fix subscription topic authorization bypass [iotdb]
jt2594838 merged PR #18418: URL: https://github.com/apache/iotdb/pull/18418 -- 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]
Re: [PR] Fix subscription topic authorization bypass [iotdb]
Caideyipi commented on code in PR #18418:
URL: https://github.com/apache/iotdb/pull/18418#discussion_r3734760719
##
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/subscription/agent/SubscriptionTopicAgent.java:
##
@@ -369,6 +375,105 @@ public TSStatus checkTopicOwners(
return RpcUtils.SUCCESS_STATUS;
}
+ /**
+ * Check that the authenticated session can read all data covered by the
requested topics.
+ * ConsumerConfig is client-controlled and therefore must not be used as the
authorization
+ * identity.
+ */
+ public TSStatus checkTopicReadPermissions(
+ final String username,
+ final ConsumerConfig consumerConfig,
+ final Iterable topicNames) {
+if (Objects.isNull(username)) {
+ return RpcUtils.getStatus(TSStatusCode.NO_PERMISSION);
+}
+
+acquireReadLock();
+try {
+ for (final String topicName : topicNames) {
+final TopicMeta topicMeta =
+topicMetaKeeper.getTopicMeta(topicName,
isTableModel(consumerConfig));
+if (Objects.isNull(topicMeta)) {
+ continue;
+}
+
+final TSStatus status =
+topicMeta.getConfig().isTableTopic()
+? checkTableTopicReadPermission(username, topicMeta)
+: checkTreeTopicReadPermission(username, topicMeta);
+if (status.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ return status;
+}
+ }
+ return RpcUtils.SUCCESS_STATUS;
+} finally {
+ releaseReadLock();
+}
+ }
+
+ private TSStatus checkTreeTopicReadPermission(final String username, final
TopicMeta topicMeta) {
+final TopicConfig topicConfig = topicMeta.getConfig();
+final TreePattern treePattern =
+topicConfig.getAttribute().containsKey(TopicConstant.PATTERN_KEY)
+? new
PrefixTreePattern(topicConfig.getAttribute().get(TopicConstant.PATTERN_KEY))
+: new IoTDBTreePattern(
+topicConfig.getStringOrDefault(
+TopicConstant.PATH_KEY, TopicConstant.PATH_DEFAULT_VALUE));
+for (final PartialPath path : treePattern.getBaseInclusionPaths()) {
+ if (!AuthorityChecker.checkFullPathOrPatternPermission(
+ username, path, PrivilegeType.READ_DATA)) {
+return AuthorityChecker.getTSStatus(false, path,
PrivilegeType.READ_DATA);
+ }
+}
+return RpcUtils.SUCCESS_STATUS;
+ }
+
+ private TSStatus checkTableTopicReadPermission(final String username, final
TopicMeta topicMeta) {
+if (AuthorityChecker.SUPER_USER.equals(username)) {
+ return RpcUtils.SUCCESS_STATUS;
+}
+final TopicConfig topicConfig = topicMeta.getConfig();
+final String database =
+topicConfig.getStringOrDefault(
+TopicConstant.DATABASE_KEY, TopicConstant.DATABASE_DEFAULT_VALUE);
+final String table =
+topicConfig.getStringOrDefault(TopicConstant.TABLE_KEY,
TopicConstant.TABLE_DEFAULT_VALUE);
+
+// A database-level SELECT grant covers all tables in one database. For a
topic whose
+// database/table is a regular expression, only an any-scope SELECT grant
is broad enough to
+// cover every object matched by the topic.
+final boolean databasePattern = isRegexPattern(database);
+final boolean tablePattern = isRegexPattern(table);
+final boolean allowed =
+(databasePattern
+? AuthorityChecker.checkDBPermission(
+username, AuthorityChecker.ANY_SCOPE, PrivilegeType.SELECT)
+: AuthorityChecker.checkDBPermission(username, database,
PrivilegeType.SELECT)
+|| (!tablePattern
+&& AuthorityChecker.checkTablePermission(
+username, database, table, PrivilegeType.SELECT)));
+return allowed
+? RpcUtils.SUCCESS_STATUS
+: AuthorityChecker.getTSStatus(false, PrivilegeType.SELECT, database,
table);
+ }
+
+ private static boolean isRegexPattern(final String value) {
+return value.indexOf('.') >= 0
+|| value.indexOf('*') >= 0
+|| value.indexOf('+') >= 0
+|| value.indexOf('?') >= 0
+|| value.indexOf('[') >= 0
+|| value.indexOf(']') >= 0
+|| value.indexOf('(') >= 0
+|| value.indexOf(')') >= 0
+|| value.indexOf('{') >= 0
+|| value.indexOf('}') >= 0
+|| value.indexOf('|') >= 0
+|| value.indexOf('^') >= 0
+|| value.indexOf('$') >= 0
+|| value.indexOf('\\') >= 0;
+ }
Review Comment:
The database and table attributes in a table topic are regex patterns by
contract (the same values are compiled by TablePattern). Therefore a
metacharacter means the topic may cover multiple objects, and the conservative
any-scope SELECT check is intentional. A literal name containing regex
metacharacters must be escaped in the topic pattern; it is not interpreted as
an unescaped literal table name.
--
This is an automated message from the Apache Git
Re: [PR] Fix subscription topic authorization bypass [iotdb]
jt2594838 commented on code in PR #18418:
URL: https://github.com/apache/iotdb/pull/18418#discussion_r3734126683
##
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/subscription/agent/SubscriptionTopicAgent.java:
##
@@ -369,6 +375,105 @@ public TSStatus checkTopicOwners(
return RpcUtils.SUCCESS_STATUS;
}
+ /**
+ * Check that the authenticated session can read all data covered by the
requested topics.
+ * ConsumerConfig is client-controlled and therefore must not be used as the
authorization
+ * identity.
+ */
+ public TSStatus checkTopicReadPermissions(
+ final String username,
+ final ConsumerConfig consumerConfig,
+ final Iterable topicNames) {
+if (Objects.isNull(username)) {
+ return RpcUtils.getStatus(TSStatusCode.NO_PERMISSION);
+}
+
+acquireReadLock();
+try {
+ for (final String topicName : topicNames) {
+final TopicMeta topicMeta =
+topicMetaKeeper.getTopicMeta(topicName,
isTableModel(consumerConfig));
+if (Objects.isNull(topicMeta)) {
+ continue;
+}
+
+final TSStatus status =
+topicMeta.getConfig().isTableTopic()
+? checkTableTopicReadPermission(username, topicMeta)
+: checkTreeTopicReadPermission(username, topicMeta);
+if (status.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ return status;
+}
+ }
+ return RpcUtils.SUCCESS_STATUS;
+} finally {
+ releaseReadLock();
+}
+ }
+
+ private TSStatus checkTreeTopicReadPermission(final String username, final
TopicMeta topicMeta) {
+final TopicConfig topicConfig = topicMeta.getConfig();
+final TreePattern treePattern =
+topicConfig.getAttribute().containsKey(TopicConstant.PATTERN_KEY)
+? new
PrefixTreePattern(topicConfig.getAttribute().get(TopicConstant.PATTERN_KEY))
+: new IoTDBTreePattern(
+topicConfig.getStringOrDefault(
+TopicConstant.PATH_KEY, TopicConstant.PATH_DEFAULT_VALUE));
+for (final PartialPath path : treePattern.getBaseInclusionPaths()) {
+ if (!AuthorityChecker.checkFullPathOrPatternPermission(
+ username, path, PrivilegeType.READ_DATA)) {
+return AuthorityChecker.getTSStatus(false, path,
PrivilegeType.READ_DATA);
+ }
+}
+return RpcUtils.SUCCESS_STATUS;
+ }
+
+ private TSStatus checkTableTopicReadPermission(final String username, final
TopicMeta topicMeta) {
+if (AuthorityChecker.SUPER_USER.equals(username)) {
+ return RpcUtils.SUCCESS_STATUS;
+}
+final TopicConfig topicConfig = topicMeta.getConfig();
+final String database =
+topicConfig.getStringOrDefault(
+TopicConstant.DATABASE_KEY, TopicConstant.DATABASE_DEFAULT_VALUE);
+final String table =
+topicConfig.getStringOrDefault(TopicConstant.TABLE_KEY,
TopicConstant.TABLE_DEFAULT_VALUE);
+
+// A database-level SELECT grant covers all tables in one database. For a
topic whose
+// database/table is a regular expression, only an any-scope SELECT grant
is broad enough to
+// cover every object matched by the topic.
+final boolean databasePattern = isRegexPattern(database);
+final boolean tablePattern = isRegexPattern(table);
+final boolean allowed =
+(databasePattern
+? AuthorityChecker.checkDBPermission(
+username, AuthorityChecker.ANY_SCOPE, PrivilegeType.SELECT)
+: AuthorityChecker.checkDBPermission(username, database,
PrivilegeType.SELECT)
+|| (!tablePattern
+&& AuthorityChecker.checkTablePermission(
+username, database, table, PrivilegeType.SELECT)));
+return allowed
+? RpcUtils.SUCCESS_STATUS
+: AuthorityChecker.getTSStatus(false, PrivilegeType.SELECT, database,
table);
+ }
+
+ private static boolean isRegexPattern(final String value) {
+return value.indexOf('.') >= 0
+|| value.indexOf('*') >= 0
+|| value.indexOf('+') >= 0
+|| value.indexOf('?') >= 0
+|| value.indexOf('[') >= 0
+|| value.indexOf(']') >= 0
+|| value.indexOf('(') >= 0
+|| value.indexOf(')') >= 0
+|| value.indexOf('{') >= 0
+|| value.indexOf('}') >= 0
+|| value.indexOf('|') >= 0
+|| value.indexOf('^') >= 0
+|| value.indexOf('$') >= 0
+|| value.indexOf('\\') >= 0;
+ }
Review Comment:
Is it possible that the table name contains these chars?
--
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]
Re: [PR] Fix subscription topic authorization bypass [iotdb]
codecov[bot] commented on PR #18418: URL: https://github.com/apache/iotdb/pull/18418#issuecomment-5212066697 ## [Codecov](https://app.codecov.io/gh/apache/iotdb/pull/18418?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :x: Patch coverage is `0%` with `98 lines` in your changes missing coverage. Please review. :white_check_mark: Project coverage is 43.60%. Comparing base ([`7e2c453`](https://app.codecov.io/gh/apache/iotdb/commit/7e2c453fbd1880e977c4da50395881a36edc1898?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ed94e55`](https://app.codecov.io/gh/apache/iotdb/commit/ed94e550994992a949b5554a4a32d183f4435efe?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). | [Files with missing lines](https://app.codecov.io/gh/apache/iotdb/pull/18418?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [.../db/subscription/agent/SubscriptionTopicAgent.java](https://app.codecov.io/gh/apache/iotdb/pull/18418?src=pr&el=tree&filepath=iotdb-core%2Fdatanode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fdb%2Fsubscription%2Fagent%2FSubscriptionTopicAgent.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9kYXRhbm9kZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaW90ZGIvZGIvc3Vic2NyaXB0aW9uL2FnZW50L1N1YnNjcmlwdGlvblRvcGljQWdlbnQuamF2YQ==) | 0.00% | [57 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/18418?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [.../subscription/receiver/SubscriptionReceiverV1.java](https://app.codecov.io/gh/apache/iotdb/pull/18418?src=pr&el=tree&filepath=iotdb-core%2Fdatanode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fdb%2Fsubscription%2Freceiver%2FSubscriptionReceiverV1.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9kYXRhbm9kZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaW90ZGIvZGIvc3Vic2NyaXB0aW9uL3JlY2VpdmVyL1N1YnNjcmlwdGlvblJlY2VpdmVyVjEuamF2YQ==) | 0.00% | [33 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/18418?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [.../subscription/agent/SubscriptionReceiverAgent.java](https://app.codecov.io/gh/apache/iotdb/pull/18418?src=pr&el=tree&filepath=iotdb-core%2Fdatanode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fdb%2Fsubscription%2Fagent%2FSubscriptionReceiverAgent.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9kYXRhbm9kZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaW90ZGIvZGIvc3Vic2NyaXB0aW9uL2FnZW50L1N1YnNjcmlwdGlvblJlY2VpdmVyQWdlbnQuamF2YQ==) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/18418?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [.../db/protocol/thrift/impl/ClientRPCServiceImpl.java](https://app.codecov.io/gh/apache/iotdb/pull/18418?src=pr&el=tree&filepath=iotdb-core%2Fdatanode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fdb%2Fprotocol%2Fthrift%2Fimpl%2FClientRPCServiceImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9kYXRhbm9kZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaW90ZGIvZGIvcHJvdG9jb2wvdGhyaWZ0L2ltcGwvQ2xpZW50UlBDU2VydmljZUltcGwuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/18418?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #18418 +/- ## - Coverage 43.61% 43.60% -0.02% Complexity 374 374 Files 5393 5393 Lines385958 386055 +97 Branches 5026750297 +30 - Hits 168337 168323 -14 - Misses 217621 217732 +111 ``` [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/iotdb/pull/18418?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share
Re: [PR] Fix subscription topic authorization bypass [iotdb]
sonarqubecloud[bot] commented on PR #18418: URL: https://github.com/apache/iotdb/pull/18418#issuecomment-5211772764 ## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18418) **Quality Gate passed** Issues  [20 New issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=18418&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=18418&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_iotdb&pullRequest=18418&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=18418&metric=new_coverage&view=list)  [2.4% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=18418&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18418) -- 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]
