Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/24825 )
Change subject: [security] Add proxy user authorization ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/24825/1/src/kudu/security/proxy_user_authorizer-test.cc File src/kudu/security/proxy_user_authorizer-test.cc: http://gerrit.cloudera.org:8080/#/c/24825/1/src/kudu/security/proxy_user_authorizer-test.cc@33 PS1, Line 33: ProxyUserAuthorizerTest I think it would be worth to add some more test scenarios, e.g.: - wildcards ("*") for users/hosts/groups - whitespace in config lists (e.g. user_acl="proxy=alice, bob") - mixed values parsing (proxy=alice,bob,other=*) http://gerrit.cloudera.org:8080/#/c/24825/1/src/kudu/security/proxy_user_authorizer.cc File src/kudu/security/proxy_user_authorizer.cc: http://gerrit.cloudera.org:8080/#/c/24825/1/src/kudu/security/proxy_user_authorizer.cc@39 PS1, Line 39: stable since these are new security flags, they are first usually flagged as experimental, and when the feature is complete and matured the flags are updated to stable http://gerrit.cloudera.org:8080/#/c/24825/1/src/kudu/security/proxy_user_authorizer.cc@87 PS1, Line 87: strings::Split(flag, ",", strings::SkipWhitespace() Here, and the other ParseX methods, the strings::SkipWhitespace() only drops all-whitespace tokens, but it does not trim retained tokens, so a config like "--proxy_user_acl=proxy=alice, bob" stores the effective user as " bob", so it will never match "bob" at auth time. Please StripWhiteSpace/Trim each entry (and the real_user) in SplitRule before inserting it. http://gerrit.cloudera.org:8080/#/c/24825/1/src/kudu/security/proxy_user_authorizer.cc@138 PS1, Line 138: cidr += "/32"; Here a bare address without / gets += "/32". Network::ParseCIDRString fully supports IPv6, and bits > 128 is the only ceiling, so a bare IPv6 literal like fe80::1 becomes fe80::1/32, which is a valid but massively over-broad /32 network that matches far more than the intended single host. For an authorization host allow-list, silently widening the match is a security-relevant bug. Either pick the default prefix based on the parsed family (/128 for v6, /32 for v4), or require an explicit prefix for IPv6. http://gerrit.cloudera.org:8080/#/c/24825/1/src/kudu/security/proxy_user_authorizer.cc@222 PS1, Line 222: proxy user host rules only support IPv4 addresses this is misleading: Sockaddr::is_ip() returns true for both AF_INET and AF_INET6, and Network::WithinNetwork handles IPv6. So IPv6 peers do flow through host matching, the code isn't actually IPv4-only. -- To view, visit http://gerrit.cloudera.org:8080/24825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id4f2efa2d853c00328cde49c076f6fb5d919471f Gerrit-Change-Number: 24825 Gerrit-PatchSet: 1 Gerrit-Owner: mintao <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 14 Sep 2026 13:22:36 +0000 Gerrit-HasComments: Yes
