[CELEBORN-1258] Add UserIdentifier to the master application meta #2365

turboFei · 2024-03-07T17:09:04Z

What changes were proposed in this pull request?

In this pr, we support to update the user identifier in application meta when initializing lifecycle manager.

Why are the changes needed?

to close CELEBORN-1285

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UT.

codecov · 2024-03-07T20:51:24Z

Codecov Report

Attention: Patch coverage is 30.00000% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 48.93%. Comparing base (a371f93) to head (0feedd7).
Report is 4 commits behind head on main.

Files	Patch %	Lines
...org/apache/celeborn/common/util/PbSerDeUtils.scala	0.00%	7 Missing ⚠️
...born/common/protocol/message/ControlMessages.scala	0.00%	6 Missing ⚠️
.../apache/celeborn/common/meta/ApplicationMeta.scala	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2365      +/-   ##
==========================================
+ Coverage   48.85%   48.93%   +0.08%     
==========================================
  Files         209      209              
  Lines       13102    13153      +51     
  Branches     1134     1137       +3     
==========================================
+ Hits         6400     6435      +35     
- Misses       6282     6296      +14     
- Partials      420      422       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

turboFei · 2024-03-07T23:26:16Z

cc @RexXiong

Could you help have a overall review? I can enhance it after that.

RexXiong · 2024-03-08T04:23:40Z

cc @RexXiong

Could you help have a overall review? I can enhance it after that.

OK, I will review this asap.

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala

...c/main/java/org/apache/celeborn/common/network/sasl/registration/RegistrationRpcHandler.java

common/src/main/scala/org/apache/celeborn/common/identity/UserIdentifier.scala

common/src/main/scala/org/apache/celeborn/common/meta/ApplicationInfo.scala

common/src/main/scala/org/apache/celeborn/common/util/PbSerDeUtils.scala

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala

...src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java

mridulm · 2024-03-14T07:29:52Z

A general query on this - is there any benefit to not supporting auth going forward given authn support has completed ? (except for backward compatibility reasons for older applications)
Based on a quick review, this change is not backwardly compatible - so existing applications cannot leverage it.
Given that, why not enable auth instead of supporting auth-disabled + registration ?

+CC @otterc in case I am missing something !

turboFei · 2024-03-15T04:36:15Z

is there any benefit to not supporting auth going forward given authn support has completed ?

I agree, we will also enable auth for our use case.

turboFei · 2024-03-20T00:33:16Z

This pr is used to provide the tenant information for dashboard. https://issues.apache.org/jira/browse/CELEBORN-1227

Do we plan to set celeborn.auth.enabled to true by defaults?

RexXiong · 2024-03-20T05:52:53Z

This pr is used to provide the tenant information for dashboard. https://issues.apache.org/jira/browse/CELEBORN-1227

Do we plan to set celeborn.auth.enabled to true by defaults?

No, IMO this feature has nothing to do with whether the auth enabled or not.

otterc · 2024-03-20T18:38:27Z

Have a general question. Why is user identifier needed to be shared with Celeborn? I can't find any information in the linked jira: CELEBORN-1285
Also, I think if registration is required for the feature that we are trying to support, I think it will better to add a feature flag instead of register.enabled flag.

turboFei · 2024-03-20T22:15:43Z

Have a general question. Why is user identifier needed to be shared with Celeborn? I can't find any information in the linked jira: CELEBORN-1285 Also, I think if registration is required for the feature that we are trying to support, I think it will better to add a feature flag instead of register.enabled flag.

Thanks for the suggestion, will check how to implement it.

client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala

RexXiong

Should also deserialize the userIndentifier of ApplicationMeta in MetaHandler.

otterc · 2024-03-25T21:38:29Z

client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala

  // Since method `onStart` is executed when `rpcEnv.setupEndpoint` is executed, and
  // `masterClient` is initialized after `rpcEnv` is initialized, if method `onStart` contains
  // a reference to `masterClient`, there may be cases where `masterClient` is null when
  // `masterClient` is called. Therefore, it's necessary to uniformly execute the initialization
  // method at the end of the construction of the class to perform the initialization operations.
  private def initialize(): Unit = {
    // noinspection ConvertExpressionToSAM
+    updateApplicationMeta()


This actually conflicts with safely propagating the application secret to the Celeborn Master. When auth is enabled, this will transmit application secret to Celeborn Master without any Sasl client authentication. Currently, we have added Anonymous client authentication, but the plan was to add other mechanisms.
cc. @mridulm

I think we should rename the existing ApplicationMeta to ApplicationAuthMeta and it should contain information specific to authentication. We can then use ApplicationMeta for general app info that needs to be sent to the Celeborn Master.

raised #2420 to separate application auth and general meta. cc @otterc @RexXiong

This actually conflicts with safely propagating the application secret to the Celeborn Master. When auth is enabled, this will transmit application secret to Celeborn Master without any Sasl client authentication. Currently, we have added Anonymous client authentication, but the plan was to add other mechanisms. cc. @mridulm

Based on my understanding, if authentication is enabled in Celeborn, it is not possible to access the Celeborn Master without SASL client authentication. This PR does not introduce new mechanisms; it merely adds an identifier to ApplicationMeta. Therefore, I believe this PR does not compromise security. @otterc

Yes, you are correct. The connection will only be established once the client is authenticated. However, I still believe that sending the secret in plain text multiple times to the Master may cause issues in the future. Eventually, we would want to encrypt the secret, and TLS support is for that purpose. If we are sending the secret multiple times, it would mean that not only registration has to be done with TLS but also that all messages should be sent via TLS. Therefore, I think having authentication metadata separated from general application metadata will be better.

Agree with most messages shouldn't be sent via TLS. I believe that the most fundamental issue with the existing security authentication mechanism is that it only authenticates the connection, but does not verify the legitimacy of the messages sent by the authenticated client. At the very least, we need verify that the applicationId in the sent messages matches the applicationId provided during the initial authentication, Otherwise, an authenticated client could still access or modify with the data of other applications. I am not sure if this is in line with the expectations.

Yes, this is a gap. We can validate the appId in the fetch and push messages of an application. I created https://issues.apache.org/jira/browse/CELEBORN-1360 for that and will create a PR soon.

Yes, this is a gap. We can validate the appId in the fetch and push messages of an application. I created https://issues.apache.org/jira/browse/CELEBORN-1360 for that and will create a PR soon.

If that's the case, then we won't have to send a secret; we'll only need to update the userIdentifier. I believe that extending the ApplicationMeta to contain more application-related information is acceptable.

flatten fix ut catch error for app meta update address comments deser user identifier

github-actions · 2024-04-18T08:31:02Z

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.

codecov-commenter · 2024-04-18T08:31:18Z

Codecov Report

Attention: Patch coverage is 30.00000% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 48.93%. Comparing base (fc23800) to head (0feedd7).
Report is 36 commits behind head on main.

❗ Current head 0feedd7 differs from pull request most recent head a9a4761. Consider uploading reports for the commit a9a4761 to get more accurate results

Files	Patch %	Lines
...org/apache/celeborn/common/util/PbSerDeUtils.scala	0.00%	7 Missing ⚠️
...born/common/protocol/message/ControlMessages.scala	0.00%	6 Missing ⚠️
.../apache/celeborn/common/meta/ApplicationMeta.scala	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2365      +/-   ##
==========================================
- Coverage   48.96%   48.93%   -0.03%     
==========================================
  Files         209      209              
  Lines       13146    13153       +7     
  Branches     1135     1137       +2     
==========================================
- Hits         6436     6435       -1     
- Misses       6287     6296       +9     
+ Partials      423      422       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

turboFei force-pushed the app_register_4 branch from 7cca9fd to c7fb876 Compare March 7, 2024 18:30

turboFei changed the title ~~App register 4~~ Support to register application without auth enabled Mar 7, 2024

turboFei force-pushed the app_register_4 branch from 60696de to 89472cd Compare March 7, 2024 20:45

turboFei force-pushed the app_register_4 branch 2 times, most recently from 8054753 to 2da0aed Compare March 7, 2024 22:26

turboFei changed the title ~~Support to register application without auth enabled~~ [CELEBORN-1258] Add UserIdentifier to the master application meta Mar 7, 2024

turboFei changed the title ~~[CELEBORN-1258] Add UserIdentifier to the master application meta~~ [CELEBORN-1258] Add application UserIdentifier to the master application meta Mar 7, 2024

turboFei changed the title ~~[CELEBORN-1258] Add application UserIdentifier to the master application meta~~ [CELEBORN-1258] Add UserIdentifier to the master application meta Mar 7, 2024

turboFei mentioned this pull request Mar 7, 2024

[CELEBORN-1258] Add UserIdentifier into the master application meta #2296

Closed

turboFei force-pushed the app_register_4 branch from 0bd3cab to fc8da8f Compare March 7, 2024 23:41

turboFei force-pushed the app_register_4 branch 2 times, most recently from 7cf4932 to 399e9c3 Compare March 12, 2024 22:25

RexXiong reviewed Mar 13, 2024

View reviewed changes

turboFei force-pushed the app_register_4 branch 4 times, most recently from 81d500c to 66e1a21 Compare March 21, 2024 17:50

turboFei marked this pull request as draft March 21, 2024 19:57

turboFei requested a review from RexXiong March 21, 2024 20:58

turboFei marked this pull request as ready for review March 21, 2024 20:58

RexXiong reviewed Mar 22, 2024

View reviewed changes

client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala Outdated Show resolved Hide resolved

client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala Outdated Show resolved Hide resolved

turboFei requested a review from RexXiong March 23, 2024 04:05

RexXiong approved these changes Mar 25, 2024

View reviewed changes

RexXiong self-requested a review March 25, 2024 06:00

turboFei force-pushed the app_register_4 branch from b5fd917 to 0feedd7 Compare March 25, 2024 06:19

otterc reviewed Mar 25, 2024

View reviewed changes

app meta

a9a4761

flatten fix ut catch error for app meta update address comments deser user identifier

turboFei force-pushed the app_register_4 branch from 0feedd7 to a9a4761 Compare March 25, 2024 22:54

github-actions bot added the stale label Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CELEBORN-1258] Add UserIdentifier to the master application meta #2365

[CELEBORN-1258] Add UserIdentifier to the master application meta #2365

turboFei commented Mar 7, 2024 •

edited

codecov bot commented Mar 7, 2024 •

edited

turboFei commented Mar 7, 2024

RexXiong commented Mar 8, 2024

mridulm commented Mar 14, 2024 •

edited

turboFei commented Mar 15, 2024

turboFei commented Mar 20, 2024

RexXiong commented Mar 20, 2024

otterc commented Mar 20, 2024

turboFei commented Mar 20, 2024

RexXiong left a comment •

edited

otterc Mar 25, 2024

otterc Mar 25, 2024

turboFei Mar 25, 2024

turboFei Mar 26, 2024

RexXiong Mar 27, 2024

otterc Mar 27, 2024

RexXiong Mar 28, 2024

otterc Mar 28, 2024 •

edited

RexXiong Mar 29, 2024

github-actions bot commented Apr 18, 2024

codecov-commenter commented Apr 18, 2024

[CELEBORN-1258] Add UserIdentifier to the master application meta #2365

Are you sure you want to change the base?

[CELEBORN-1258] Add UserIdentifier to the master application meta #2365

Conversation

turboFei commented Mar 7, 2024 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

codecov bot commented Mar 7, 2024 • edited

Codecov Report

turboFei commented Mar 7, 2024

RexXiong commented Mar 8, 2024

mridulm commented Mar 14, 2024 • edited

turboFei commented Mar 15, 2024

turboFei commented Mar 20, 2024

RexXiong commented Mar 20, 2024

otterc commented Mar 20, 2024

turboFei commented Mar 20, 2024

RexXiong left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otterc Mar 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 18, 2024

codecov-commenter commented Apr 18, 2024

Codecov Report

turboFei commented Mar 7, 2024 •

edited

codecov bot commented Mar 7, 2024 •

edited

mridulm commented Mar 14, 2024 •

edited

RexXiong left a comment •

edited

otterc Mar 28, 2024 •

edited