feat: Add GRPC TLS support - #6374
Conversation
There was a problem hiding this comment.
Pull request overview
Adds optional TLS configuration to the gRPC server and updates tests/docs to support TLS/mTLS scenarios.
Changes:
- Parse
ssl_cert/ssl_key/ssl_chainfrom[port_grpc]and build gRPC server credentials accordingly. - Add envconfig helpers to generate TLS-enabled gRPC configs for tests.
- Add a new unit test suite plus example config documentation for gRPC TLS settings.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/xrpld/app/main/GRPCServer.h |
Stores TLS config paths and declares a credentials factory helper. |
src/xrpld/app/main/GRPCServer.cpp |
Reads TLS settings from config and switches gRPC listener credentials to TLS when configured. |
src/test/jtx/impl/envconfig.cpp |
Adds helper functions to populate TLS fields in test configs. |
src/test/jtx/envconfig.h |
Declares the new TLS-related envconfig helpers. |
src/test/app/GRPCServerTLS_test.cpp |
Introduces tests intended to cover TLS and “mTLS” gRPC startup paths. |
cfg/xrpld-example.cfg |
Documents the new [port_grpc] TLS settings and provides example values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/xrpld/app/main/GRPCServer.cpp:10
#include <boost/filesystem.hpp>appears to be unused in this file (noboost::filesystemsymbols are referenced). Consider removing it to reduce compile time and avoid unnecessary dependencies (FileUtilities.h already brings in the needed Boost types).
namespace xrpl {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6374 +/- ##
=========================================
+ Coverage 81.6% 81.8% +0.2%
=========================================
Files 1010 1010
Lines 75992 76075 +83
Branches 7601 7529 -72
=========================================
+ Hits 62013 62216 +203
+ Misses 13979 13859 -120
🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| namespace { | ||
|
|
||
| constexpr std::string_view kCA_CERT_CONTENT = |
There was a problem hiding this comment.
I would appreciate it if you added commands in comments how you generated these certs, so it can be done again, if ever needed
There was a problem hiding this comment.
It's in fact done by asking AI to regenerate them, like most things these days 🤖
But also it should not be needed again - the last thing i did is regenerated it with as far possible expiry as possible so it ended up being a full 100 years from now.
| if (sslCertPath_.has_value() && sslKeyPath_.has_value()) | ||
| { |
There was a problem hiding this comment.
I would probably do a fast return in case it's without certificate.
This would decrease the identation (now almost the whole function is under if)
| JLOG(journal_.error()) | ||
| << "Failed to start gRPC server at " << serverAddress_ << " (TLS mode: " << tlsMode | ||
| << "); Possible causes: address already in use, invalid address format, or permission " | ||
| "denied"; // LCOV_EXCL_LINE |
There was a problem hiding this comment.
Do we need all these LCOV_EXCL_LINEs?
There was a problem hiding this comment.
It's common.. JLOG lines produce coverage issues apparently. But tbf i'm not too sure this particular one does anything because it's not on the same line technically. I'd leave it though as that's the general pattern used
mathbunnyru
left a comment
There was a problem hiding this comment.
A few nits, nothing major, approving, but would be glad if these nits were fixed
| grpc::SslServerCredentialsOptions sslOpts; | ||
| sslOpts.pem_key_cert_pairs.push_back(keyCertPair); | ||
|
|
||
| // Configure client certificate verification (mTLS) if CA is provided | ||
| if (sslClientCAPath_.has_value()) | ||
| { | ||
| if (clientCAContents.empty()) | ||
| { | ||
| JLOG(journal_.error()) | ||
| << "Empty/truncated gRPC SSL client CA file: " << *sslClientCAPath_ | ||
| << " - failed to configure mutual TLS"; // LCOV_EXCL_LINE | ||
| return nullptr; | ||
| } | ||
|
|
||
| sslOpts.pem_root_certs = clientCAContents; | ||
| sslOpts.client_certificate_request = | ||
| GRPC_SSL_REQUEST_AND_REQUIRE_CLIENT_CERTIFICATE_AND_VERIFY; | ||
| JLOG(journal_.info()) << "gRPC mutual TLS enabled - client certificates will be " | ||
| "required and verified"; | ||
| } |
There was a problem hiding this comment.
This can be moved above. If the sslOpts are defined earlier, then around line 616 you already confirmed that sslClientCAPath_.has_value(), so you can perform the clientCAContents.empty() check there right away, and fill the sslOpts with the values.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
High Level Overview of Change
This PR enhances the gRPC server with optional TLS and mTLS support.
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)