You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "otterc (via GitHub)" <gi...@apache.org> on 2024/04/03 06:20:21 UTC
[PR] [CELEBORN-1365] Ensure that a client cannot update the metadata belonging to a different application [celeborn]
otterc opened a new pull request, #2441:
URL: https://github.com/apache/celeborn/pull/2441
### What changes were proposed in this pull request?
This ensures that an authenticated client does not update the metadata belonging to another application.
### Why are the changes needed?
The changes are needed for authentication support.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1365] Ensure that a client cannot update the metadata belonging to a different application [celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2441:
URL: https://github.com/apache/celeborn/pull/2441#discussion_r1551888055
##########
common/src/main/scala/org/apache/celeborn/common/rpc/RpcEndpoint.scala:
##########
@@ -134,6 +136,21 @@ trait RpcEndpoint {
rpcEnv.stop(_self)
}
}
+
+ def checkAuth(context: RpcCallContext, appId: String): Unit = {
+ context match {
+ case remoteContext: RemoteNettyRpcCallContext =>
+ checkAuth(remoteContext.transportClient, appId)
+ case _ =>
+ // Do nothing if the context is not RemoteNettyRpcCallContext
+ }
+ }
+
+ private def checkAuth(client: TransportClient, appId: String): Unit = {
+ if (client.getClientId != null && !(client.getClientId == appId))
Review Comment:
Done
##########
common/src/main/scala/org/apache/celeborn/common/rpc/RpcEndpoint.scala:
##########
@@ -134,6 +136,21 @@ trait RpcEndpoint {
rpcEnv.stop(_self)
}
}
+
+ def checkAuth(context: RpcCallContext, appId: String): Unit = {
+ context match {
+ case remoteContext: RemoteNettyRpcCallContext =>
+ checkAuth(remoteContext.transportClient, appId)
+ case _ =>
+ // Do nothing if the context is not RemoteNettyRpcCallContext
+ }
+ }
+
+ private def checkAuth(client: TransportClient, appId: String): Unit = {
+ if (client.getClientId != null && !(client.getClientId == appId))
+ throw new SecurityException(
Review Comment:
Done.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1365] Ensure that a client cannot update the metadata belonging to a different application [celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on PR #2441:
URL: https://github.com/apache/celeborn/pull/2441#issuecomment-2033732660
@RexXiong @waitinfuture @mridulm @SteNicholas @pan3793 Please take a look
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1365] Ensure that a client cannot update the metadata belonging to a different application [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong closed pull request #2441: [CELEBORN-1365] Ensure that a client cannot update the metadata belonging to a different application
URL: https://github.com/apache/celeborn/pull/2441
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1365] Ensure that a client cannot update the metadata belonging to a different application [celeborn]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2441:
URL: https://github.com/apache/celeborn/pull/2441#issuecomment-2033659804
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/2441?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `11 lines` in your changes are missing coverage. Please review.
> Project coverage is 39.21%. Comparing base [(`6153ba4`)](https://app.codecov.io/gh/apache/celeborn/commit/6153ba4c627de4bdeedcb4f1e4ae45f18ad2965b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`ae8d8c7`)](https://app.codecov.io/gh/apache/celeborn/pull/2441?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files](https://app.codecov.io/gh/apache/celeborn/pull/2441?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...a/org/apache/celeborn/common/rpc/RpcEndpoint.scala](https://app.codecov.io/gh/apache/celeborn/pull/2441?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2Frpc%2FRpcEndpoint.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3JwYy9ScGNFbmRwb2ludC5zY2FsYQ==) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../apache/celeborn/common/rpc/netty/Dispatcher.scala](https://app.codecov.io/gh/apache/celeborn/pull/2441?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2Frpc%2Fnetty%2FDispatcher.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3JwYy9uZXR0eS9EaXNwYXRjaGVyLnNjYWxh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...eleborn/common/rpc/netty/NettyRpcCallContext.scala](https://app.codecov.io/gh/apache/celeborn/pull/2441?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2Frpc%2Fnetty%2FNettyRpcCallContext.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3JwYy9uZXR0eS9OZXR0eVJwY0NhbGxDb250ZXh0LnNjYWxh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...apache/celeborn/common/rpc/netty/NettyRpcEnv.scala](https://app.codecov.io/gh/apache/celeborn/pull/2441?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2Frpc%2Fnetty%2FNettyRpcEnv.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3JwYy9uZXR0eS9OZXR0eVJwY0Vudi5zY2FsYQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2441 +/- ##
==========================================
- Coverage 39.24% 39.21% -0.03%
==========================================
Files 217 217
Lines 13432 13441 +9
Branches 1173 1174 +1
==========================================
- Hits 5270 5269 -1
- Misses 7863 7874 +11
+ Partials 299 298 -1
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/celeborn/pull/2441?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 it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1365] Ensure that a client cannot update the metadata belonging to a different application [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2441:
URL: https://github.com/apache/celeborn/pull/2441#discussion_r1550946338
##########
common/src/main/scala/org/apache/celeborn/common/rpc/RpcEndpoint.scala:
##########
@@ -134,6 +136,21 @@ trait RpcEndpoint {
rpcEnv.stop(_self)
}
}
+
+ def checkAuth(context: RpcCallContext, appId: String): Unit = {
+ context match {
+ case remoteContext: RemoteNettyRpcCallContext =>
+ checkAuth(remoteContext.transportClient, appId)
+ case _ =>
+ // Do nothing if the context is not RemoteNettyRpcCallContext
+ }
+ }
+
+ private def checkAuth(client: TransportClient, appId: String): Unit = {
+ if (client.getClientId != null && !(client.getClientId == appId))
Review Comment:
super nit:
```suggestion
if (client.getClientId != null && client.getClientId != appId)
```
##########
common/src/main/scala/org/apache/celeborn/common/rpc/RpcEndpoint.scala:
##########
@@ -134,6 +136,21 @@ trait RpcEndpoint {
rpcEnv.stop(_self)
}
}
+
+ def checkAuth(context: RpcCallContext, appId: String): Unit = {
+ context match {
+ case remoteContext: RemoteNettyRpcCallContext =>
+ checkAuth(remoteContext.transportClient, appId)
+ case _ =>
+ // Do nothing if the context is not RemoteNettyRpcCallContext
+ }
+ }
+
+ private def checkAuth(client: TransportClient, appId: String): Unit = {
+ if (client.getClientId != null && !(client.getClientId == appId))
+ throw new SecurityException(
Review Comment:
`SecurityException` is typically for use by security manager in java (see `java.lang.SecurityManager`).
Might be better to create our own `Exception`, or perhaps use `IllegalStateException`
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org