Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple CC Authorizer support CCManager #2396

Merged
merged 45 commits into from
Mar 19, 2024
Merged

Multiple CC Authorizer support CCManager #2396

merged 45 commits into from
Mar 19, 2024

Conversation

yhwen
Copy link
Collaborator

@yhwen yhwen commented Mar 8, 2024

Fixes # .

Description

CCManager support multiple CC Authorizer for token generation and verification.

  • multiple CC Authorizer based on the CCAuthorizer interface.
  • individual CC Authorizer token expiration configuration.
  • Configurable CC token verification frequency.
  • critical_level configuration to control Stop_Job or Shutdown_System if CC verification failure.

Sample component config in the "local/resource.json":

{
	"id": "cc_manager",
	"path": "nvflare.app_opt.confidential_computing.cc_manager.CCManager",
	"args": {
		"cc_issuers_conf": [
			{
  				  "issuer_id": "tdx_authorizer",
			  "token_expiration": 250
  				}
		],
		"cc_verifier_ids": ["tdx_authorizer"],
		"verify_frequency": 600,
		"critical_level": 2
	}
},
{
	"id": "tdx_authorizer",
	"path": "nvflare.app_opt.confidential_computing.tdx_authorizer.TDXAuthorizer",
	"args": {
		"tdx_cli_command": "/home/azureuser/TDX/client/tdx-cli/trustauthority-cli",
		"config_dir": "/home/azureuser/NVFlare/nvflare/poc/workspace/server/local"
	}
}

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Quick tests passed locally by running ./runtest.sh.
  • In-line docstrings updated.
  • Documentation updated.

yhwen added 30 commits February 21, 2024 12:35
Copy link
Collaborator

@chesterxgchen chesterxgchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add few comments.

Notice there is no unit tests for this PR, we should add some critical ones.

Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comments

nvflare/app_common/job_schedulers/job_scheduler.py Outdated Show resolved Hide resolved
nvflare/app_opt/confidential_computing/cc_authorizer.py Outdated Show resolved Hide resolved
nvflare/app_opt/confidential_computing/cc_authorizer.py Outdated Show resolved Hide resolved
nvflare/app_opt/confidential_computing/gpu_authorizer.py Outdated Show resolved Hide resolved
nvflare/app_opt/confidential_computing/tdx_authorizer.py Outdated Show resolved Hide resolved
nvflare/app_opt/confidential_computing/tdx_authorizer.py Outdated Show resolved Hide resolved
nvflare/private/fed/app/client/client_train.py Outdated Show resolved Hide resolved
nvflare/private/fed/app/deployer/server_deployer.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@YuanTingHsieh YuanTingHsieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good unit test showcasing how to use Mock in different ways.
Have some questions

nvflare/app_opt/confidential_computing/cc_authorizer.py Outdated Show resolved Hide resolved
nvflare/app_opt/confidential_computing/gpu_authorizer.py Outdated Show resolved Hide resolved
nvflare/private/fed/app/client/client_train.py Outdated Show resolved Hide resolved
nvflare/private/fed/app/deployer/server_deployer.py Outdated Show resolved Hide resolved
nvflare/private/fed/server/admin.py Show resolved Hide resolved
nvflare/app_opt/confidential_computing/tdx_authorizer.py Outdated Show resolved Hide resolved
nvflare/app_opt/confidential_computing/tdx_authorizer.py Outdated Show resolved Hide resolved
YuanTingHsieh
YuanTingHsieh previously approved these changes Mar 15, 2024
Copy link
Collaborator

@YuanTingHsieh YuanTingHsieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some issues I raised about cc_manager.py still are not addressed. Please look into them.

@yhwen
Copy link
Collaborator Author

yhwen commented Mar 18, 2024

Some issues I raised about cc_manager.py still are not addressed. Please look into them.

renamed the events to distinguish the events for server and client side.

Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The event name "PROCEEDED_XXX" isn't what you meant. Should change to PROCESSED_XXX.

nvflare/apis/event_type.py Outdated Show resolved Hide resolved
nvflare/apis/event_type.py Outdated Show resolved Hide resolved
nvflare/private/fed/server/fed_server.py Outdated Show resolved Hide resolved
nvflare/private/fed/server/fed_server.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yhwen yhwen merged commit 0e443f8 into NVIDIA:main Mar 19, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants