feat(framework): Add --root-certificates argument for flwr-* commands and flower-superexec#6986
feat(framework): Add --root-certificates argument for flwr-* commands and flower-superexec#6986
--root-certificates argument for flwr-* commands and flower-superexec#6986Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39d56b39b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if insecure or root_cert_path is None: | ||
| return None |
There was a problem hiding this comment.
Returning None when root_cert_path is omitted conflates two different states: secure TLS with system CAs and fully insecure transport. The new callers in flwr_clientapp, flwr_serverapp, and flwr_simulation pass this value as certificates, and their runtime connections compute insecure=(certificates is None), so --insecure off plus no --root-certificates now opens a plaintext channel instead of TLS (and fails against TLS-only AppIO endpoints). This is a silent security downgrade introduced by enabling these code paths without preserving the explicit insecure/secure signal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR introduces a --root-certificates CLI flag to enable TLS (with custom roots) for AppIO-style gRPC connections used by flwr-* executor commands and flower-superexec, and adds supporting exit codes/docs/tests.
Changes:
- Add
--root-certificatesto commonflwr-*app CLI args and toflower-superexec. - Introduce
flwr.supercore.tls.load_root_certificatesplus new exit codes/docs for TLS flag conflicts and invalid paths. - Wire root certificate loading through SuperExec and executor processes, with added unit tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| framework/py/flwr/supernode/cli/flwr_clientapp.py | Uses load_root_certificates and forwards cert bytes into run_clientapp. |
| framework/py/flwr/supercore/tls.py | Adds helper to validate/resolve root certificate bytes and enforce flag compatibility. |
| framework/py/flwr/supercore/tls_test.py | Adds tests for the TLS helper behavior and error paths. |
| framework/py/flwr/supercore/superexec/run_superexec.py | Adds TLS parameters to SuperExec channel creation and forwards TLS config into plugins. |
| framework/py/flwr/supercore/superexec/plugin/exec_plugin.py | Extends plugin base class to retain TLS-related settings. |
| framework/py/flwr/supercore/superexec/plugin/base_exec_plugin.py | Forwards --insecure or --root-certificates when spawning executor subprocesses. |
| framework/py/flwr/supercore/superexec/plugin/base_exec_plugin_test.py | Adds coverage for forwarding/omitting TLS flags in subprocess args. |
| framework/py/flwr/supercore/cli/flower_superexec.py | Adds --root-certificates CLI arg and forwards it into run_superexec. |
| framework/py/flwr/simulation/app.py | Loads root certs and passes them into the Simulation AppIO connection wiring. |
| framework/py/flwr/simulation/app_test.py | Adds parsing test for --root-certificates and verifies certs passed into connection. |
| framework/py/flwr/server/serverapp/app.py | Loads root certs and passes them into the ServerApp gRPC grid wiring. |
| framework/py/flwr/common/exit/exit_code.py | Adds new exit codes 603/604 and help text. |
| framework/py/flwr/common/args.py | Adds --root-certificates to shared executor CLI args. |
| framework/docs/source/ref-exit-codes/603.rst | Documents new exit code for incompatible TLS flags. |
| framework/docs/source/ref-exit-codes/604.rst | Documents new exit code for invalid path arguments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf0e4a8179
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc99645923
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| insecure=True, | ||
| root_certificates_path=None, |
There was a problem hiding this comment.
When flwr-clientapp, flwr-serverapp, or flwr-simulation run without --token, they call run_with_deprecation_warning, and this hardcodes insecure=True while discarding any root certificate path. In this commit, those CLIs now accept --root-certificates and no longer block secure mode up front, so legacy invocations can silently downgrade to plaintext (or fail against TLS-only AppIO endpoints) even when users omit --insecure or explicitly pass --root-certificates.
Useful? React with 👍 / 👎.
github-actions
bot
added
the
Maintainer
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b3fc32a1f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| insecure=insecure, | ||
| root_certificates=load_root_certificates(root_certificates_path, insecure), | ||
| insecure=True, |
There was a problem hiding this comment.
insecure kwarg in channel creation
This call passes insecure twice (insecure=insecure and insecure=True), which makes the module fail to parse with SyntaxError: keyword argument repeated: insecure. As a result, importing flwr.supercore.superexec.run_superexec breaks, so flower-superexec and the flwr-* commands that import this module cannot start at all.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6852c8a898
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if root_cert_path is None: | ||
| return None # None in gRPC means the default system root certificates | ||
|
|
||
| if not Path(root_cert_path).expanduser().is_file(): |
There was a problem hiding this comment.
expanduser failures for cert path validation
Path.expanduser() can raise RuntimeError for invalid ~user inputs (for example, --root-certificates ~missinguser/root.pem), and this call currently happens before the guarded read_bytes() block. In that case the process will crash with an uncaught traceback instead of returning the documented COMMON_PATH_INVALID (604) via flwr_exit, so invalid certificate paths are not handled consistently.
Useful? React with 👍 / 👎.
No description provided.