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

Fix auth failure on one-way TLS #1167

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

MasterSkepticista
Copy link
Collaborator

@MasterSkepticista MasterSkepticista commented Nov 21, 2024

Edit: Internal changes for ofl-s pending, do not merge.

Changelog:

  • Fixes issue where disabling client auth with TLS-enabled, broke taskrunner experiments (Federation run fails when client auth is disabled #1123).
  • Renames tls -> use_tls in the plan.
  • Renames disable_client_auth -> require_client_auth in the plan. NOTE: This change has been made to reduce cognitive overhead of knowing how to enable mTLS in communication between the client-server.
    • Before this PR: (disable_client_auth=False, tls=True) meant enabling mTLS.
    • After this PR: (require_client_auth=True, use_tls=True) will enable mTLS.
    • Default behaviour FL plans is to use mTLS.
  • Migrates logger.warn -> logger.warning to hide deprecation warnings

Tested with 4 combinations of two updated flags.

@MasterSkepticista MasterSkepticista marked this pull request as ready for review November 21, 2024 07:58
Copy link
Collaborator

@theakshaypant theakshaypant left a comment

Choose a reason for hiding this comment

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

Is updating the usage of these keys in tests not in scope for this PR? Tests Ref.

@MasterSkepticista
Copy link
Collaborator Author

Is updating the usage of these keys in tests not in scope for this PR? Tests Ref.

Yes. @payalcha suggested applying those changes in a separate PR (after this is merged).

Copy link
Collaborator

@teoparvanov teoparvanov left a comment

Choose a reason for hiding this comment

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

Great, thanks @MasterSkepticista !

PS: as discussed offline, don't forget to run the OpenFL-Security CI against this branch

openfl/transport/grpc/aggregator_client.py Outdated Show resolved Hide resolved
openfl/transport/grpc/aggregator_client.py Outdated Show resolved Hide resolved
openfl/transport/grpc/aggregator_client.py Show resolved Hide resolved
openfl/transport/grpc/aggregator_server.py Outdated Show resolved Hide resolved
openfl/transport/grpc/aggregator_server.py Show resolved Hide resolved
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