feat(tls): Secure master-admin with TLS layer LS-245#748
Conversation
rolysr
changed the title
|
@gemini-code-assist review |
Summary of ChangesHello @rolysr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the system by integrating a TLS layer for communication between the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant security enhancement by adding a TLS layer to encrypt traffic between the master and the saunafs-admin binary. The changes are consistently applied across all admin commands. While the overall approach is sound, I've found some critical functional issues in the new TLS communication logic within ServerConnection, specifically that the function for receiving data over TLS is currently broken and will not work as intended. There is also an opportunity to reduce code duplication across the admin command files, though this is considered a low priority.
There was a problem hiding this comment.
Pull request overview
This PR adds an optional TLS layer for saunafs-admin ↔ metadata server communication, and updates the master-side protocol handling to allow shadow metadata servers to accept TLS upgrade requests as well.
Changes:
- Add
--tlsconfigfilesupport acrosssaunafs-admincommands and pass it through to connections. - Extend
ServerConnection/RegisteredAdminConnectionto initiate STARTTLS and wrap subsequent traffic in TLS. - Add a short system test validating
saunafs-adminoperations over TLS (including shadow connectivity).
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_suites/ShortSystemTests/test_tls_master_admin_communication.sh | New system test covering saunafs-admin TLS interactions with master and shadow. |
| src/master/matoclserv.cc | Allow handling STARTTLS on non-master personalities (enables shadow TLS upgrade). |
| src/common/server_connection.h | Add optional TLS config path/state to ServerConnection and extend API signatures. |
| src/common/server_connection.cc | Implement STARTTLS request + TLS handshake, and route send/receive via OpenSSL when enabled. |
| src/admin/saunafs_admin_command.h | Define new --tlsconfigfile option constants. |
| src/admin/saunafs_admin_command.cc | Provide --tlsconfigfile option string/description. |
| src/admin/registered_admin_connection.h | Extend RegisteredAdminConnection::create() to accept TLS config path. |
| src/admin/registered_admin_connection.cc | Pass TLS config path through to the underlying connection. |
| src/admin/info_command.cc | Add TLS option and use TLS-enabled ServerConnection. |
| src/admin/dump_config_command.cc | Add TLS option and use TLS-enabled RegisteredAdminConnection. |
| src/admin/save_metadata_command.cc | Add TLS option and use TLS-enabled RegisteredAdminConnection. |
| src/admin/reload_config_command.h | Declare TLS option support. |
| src/admin/reload_config_command.cc | Add TLS option and use TLS-enabled RegisteredAdminConnection. |
| src/admin/stop_task_command.h | Declare TLS option support. |
| src/admin/stop_task_command.cc | Add TLS option and use TLS-enabled RegisteredAdminConnection. |
| src/admin/stop_master_without_saving_metadata.h | Declare TLS option support. |
| src/admin/stop_master_without_saving_metadata.cc | Add TLS option and use TLS-enabled RegisteredAdminConnection. |
| src/admin/promote_shadow_command.h | Declare TLS option support. |
| src/admin/promote_shadow_command.cc | Add TLS option and use TLS-enabled RegisteredAdminConnection. |
| src/admin/ready_chunkservers_count_command.h | Declare TLS option support. |
| src/admin/ready_chunkservers_count_command.cc | Add TLS option and plumb TLS into chunkserver list retrieval. |
| src/admin/list_chunkservers_command.h | Extend chunkserver-list helper to accept TLS config. |
| src/admin/list_chunkservers_command.cc | Add TLS option and pass TLS config into helper. |
| src/admin/mount_info_list_command.h | Declare TLS option support. |
| src/admin/mount_info_list_command.cc | Add TLS option and use TLS-enabled ServerConnection. |
| src/admin/metadataserver_status_command.cc | Add TLS option and use TLS-enabled ServerConnection. |
| src/admin/manage_locks_command.cc | Add TLS option and use TLS-enabled RegisteredAdminConnection. |
| src/admin/magic_recalculate_metadata_checksum_command.cc | Add TLS option and pass TLS config alongside timeout. |
| src/admin/list_tasks_command.h | Declare TLS option support. |
| src/admin/list_tasks_command.cc | Add TLS option and use TLS-enabled ServerConnection. |
| src/admin/list_sessions_command.cc | Add TLS option and use TLS-enabled ServerConnection. |
| src/admin/list_mounts_command.cc | Add TLS option to supported options list. |
| src/admin/list_metadataservers_command.cc | Add TLS option and use TLS-enabled ServerConnection for initial query. |
| src/admin/list_inotifiers_command.cc | Add TLS option and use TLS-enabled ServerConnection. |
| src/admin/list_goals_command.cc | Add TLS option and use TLS-enabled ServerConnection. |
| src/admin/list_disks_command.cc | Add TLS option and pass TLS config into chunkserver list retrieval. |
| src/admin/list_disk_groups_command.h | Declare TLS option support. |
| src/admin/list_disk_groups_command.cc | Add TLS option and pass TLS config into chunkserver list retrieval. |
| src/admin/list_defective_files_command.cc | Add TLS option and use TLS-enabled ServerConnection. |
| src/admin/io_limits_status_command.cc | Add TLS option and use TLS-enabled ServerConnection. |
| src/admin/delete_sessions_command.h | Declare TLS option support. |
| src/admin/delete_sessions_command.cc | Add TLS option and use TLS-enabled ServerConnection. |
| src/admin/chunk_health_command.cc | Add TLS option and use TLS-enabled ServerConnection. |
tests/test_suites/ShortSystemTests/test_tls_master_admin_communication.sh
Show resolved
Hide resolved
There was a problem hiding this comment.
Code Review
This pull request introduces a TLS layer to secure communication between saunafs-admin and the master/shadow servers, which is a great security enhancement. The implementation correctly adds the --tlsconfigfile option across all relevant admin commands and modifies the connection logic to handle TLS sessions.
I've found a critical issue in the TLS data receiving logic that could lead to data corruption or crashes. Please see my detailed comment for suggestions on how to address this, incorporating established rules regarding blocking I/O and graceful shutdowns.
rolysr
force-pushed
the
add-tls-master-saunafs-admin
branch
from
6096638 to
6eda0c1
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant security enhancement by adding a TLS layer to encrypt traffic between saunafs-admin and the master/shadow servers. The implementation involves adding a --tlsconfigfile option to nearly all admin commands and modifying the ServerConnection class to handle TLS handshakes and encrypted I/O. The changes are extensive and well-tested with a new integration test.
However, I have identified a few important issues that should be addressed:
- High Severity: The client-side TLS implementation in
server_connection.ccuses a large, fixed-size buffer for receiving messages, which could be a denial-of-service vector. - High Severity: The server-side
STARTTLShandling inmatoclserv.ccdoes not check if a TLS session is already active, which could lead to resource leaks or undefined behavior if a client sends the request multiple times. - Omission: The
list-mountscommand has the--tlsconfigfileoption added to its supported options, but the implementation inrun()was not updated to use it, meaning it will not establish a TLS connection. This appears to be an oversight.
I have provided detailed comments on the high-severity issues. Addressing these will improve the robustness, efficiency, and security of the new TLS functionality.
rolysr
force-pushed
the
add-tls-master-saunafs-admin
branch
2 times, most recently
from
0fd54a8 to
e3f2503
Compare
rolysr
requested review from
antuan96314,
dmga44,
lgsilva3087,
ralcolea and
uristdwarf
rolysr
force-pushed
the
add-tls-master-saunafs-admin
branch
4 times, most recently
from
3bd0f91 to
ea8c608
Compare
lgsilva3087
left a comment
lgsilva3087
left a comment
There was a problem hiding this comment.
Great work, please see my minor comments.
rolysr
force-pushed
the
add-tls-master-saunafs-admin
branch
from
ea8c608 to
8522b23
Compare
rolysr
force-pushed
the
add-tls-master-saunafs-admin
branch
from
8522b23 to
7c64523
Compare
This change introduces a TLS layer to encrypt traffic between the master and the saunafs-admin binary, protecting data in transit and mitigating on-path attacks. For that goal, an optional TLS configuration file path option has been added on all commands for saunafs-admin. Also, this change allows the shadow metadata servers to start a TLS connection request, which is needed for some saunafs-admin binary to securely communicate to it. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
rolysr
force-pushed
the
add-tls-master-saunafs-admin
branch
from
7c64523 to
20aa492
Compare
This change introduces a TLS layer to encrypt traffic between the master and the saunafs-admin binary, protecting data in transit and mitigating on-path attacks. For that goal, an optional TLS configuration file path option has been added on all commands for saunafs-admin.
Also, this change allows the shadow metadata servers to start a TLS connection request, which is needed for some saunafs-admin binary to securely communicate to it.
To the reviewers:
src/master/matoclserv.ccwhich can be divided by two, one is the addition of a protection layer to avoid any possible protocol leak if a client tries to call an*_STARTTLSrequest if there is an already instantiatedtlsSession; the goal of this idea is to force clients to first close their connection reference and then starting a new tls session request from scratch. The second change is that now not only masters can handle *_STARTTLS requests, but shadows can also do it; that change was needed to allowing other components communicating directly to shadow servers through TLS, to have a secure way to protect that communication, like for instance thesaunafs-adminbinary.saunafs-admin. The idea was to make it completely optional and ensuring that all server connection derived instances take it into account if present.