[committer] Secure Committer Communication with TLS and mTLS#85
Conversation
8910626 to
681e5f5
Compare
- Added TLS and mTLS configuration for both server and client - Implemented secure communication unit tests - Fixed linter issues and test configurations - Updated Docker ENV and YAML configurations - Refactored ServerConfig references to ClientConfig Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds end-to-end TLS and mTLS support for all committer components, enabling encrypted and authenticated gRPC communication.
- Introduce
ConfigTLSwithServerOption/ClientOptioninutils/connection/tls.go - Update server and client utilities to consume
ConfigTLSinstead of insecure defaults - Add a reusable
RunSecureConnectionTesthelper and cover all services/tests with secure-connection scenarios
Reviewed Changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| utils/connection/tls.go | Implement ConfigTLS type and methods for TLS/mTLS support |
| utils/connection/server_util.go | Refactor GrpcServer to return TLS-enabled grpc.Server |
| utils/connection/client_util.go | Update dial configs to use ClientOption from ConfigTLS |
| utils/test/secure_connection.go | New test helper to exercise various TLS modes end-to-end |
| service/**/.go / **/_test.go | Add secure-connection tests for verifier, VC, sidecar, etc. |
| integration/runner/runtime.go | Wire up TLS cert generation and credential injection in runner |
| loadgen/client_test.go | Update loadgen tests to reference TLS-enabled client configs |
| cmd/config/templates/*.yaml | Extend all config templates to include server-creds/client-creds |
| cmd/config/app_config_test.go | Update config-parse tests for new client TLS settings |
| docker/test/container_test.go | Update container tests to use ClientConfig |
| docker/images/test_node/Dockerfile | Rename env vars for client endpoint lists |
Comments suppressed due to low confidence (5)
utils/connection/server_util.go:46
- The comment for GrpcServer is outdated since the method now returns (*grpc.Server, error). Please update it to describe the new signature and error return.
// GrpcServer instantiate a [grpc.Server].
utils/connection/tls.go:38
- [nitpick] The constant name
TLSEmptymay be unclear. Consider renaming toTLSDefaultorTLSUnspecifiedto better convey its purpose.
TLSEmpty TLSMode = ""
utils/connection/tls.go:155
- There’s no unit test covering the case where
buildCertPoolreceives an empty slice of CA paths and returns an error. Consider adding a test to verify that error path.
if len(paths) == 0 {
utils/connection/tls.go:79
- [nitpick] The name
ClientOptionWithConfigis ambiguous given that you already haveClientOption. Consider renaming it to something likeClientCredentialsWithConfigto clarify that it returns both rawtls.Configand transport credentials.
func (c *ConfigTLS) ClientOptionWithConfig() (*tls.Config, credentials.TransportCredentials, error) {
integration/runner/runtime.go:647
- createSystemConfigWithServerCerts returns &serviceCfg, a pointer to a locally scoped variable. This pointer will be invalid after the function returns. Consider allocating on the heap or returning a copy of c.SystemConfig directly.
serviceCfg := c.SystemConfig
* I focused test coverage on the core functionality rather than simple input validation checks. The empty slice case is a straightforward early return that doesn't affect the main certificate building logic. Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
…tests * Moved tlsgen to an outer package to prevent over-coupling Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
|
Conducted an online review with @liran-funaro. Updates will follow accordingly. |
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
* Semantic changes. Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
* removed TLSMode type. Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
…g type. * Move tls_manager to the test package. Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
…S configurations. Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
…set by the hostname of the service we want to connect to by default. * Removed unnecessary code. Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
* Explicit comments and returned errors for tls creds builders. Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
* Added support for loadgen clients tls and for distributed loadgen tls connectivity. Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
liran-funaro
left a comment
There was a problem hiding this comment.
This is my last review after multiple iterations of online reviews.
This looks good overall, but I still have a few minor comments.
Especially in utils/test/secure_connection.go.
@cendhu Please add your review and merge the PR when it is ready.
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
liran-funaro
left a comment
There was a problem hiding this comment.
LGTM. @cendhu If you have time, I'd prefer you have a quick review as well.
If not, then you can merge it.
cendhu
left a comment
There was a problem hiding this comment.
LGTM.
In Fabric, the tlscacert can be provided in the config block as well as through the local config of the executable. We need to understand the reasoning and employ the same here in the subsequent PRs if needed.
| case OneSideTLSMode, MutualTLSMode: | ||
| tlsCfg := &tls.Config{ | ||
| MinVersion: DefaultTLSMinVersion, | ||
| ClientAuth: tls.NoClientCert, |
There was a problem hiding this comment.
MutualTLS requires both the server and the client to authenticate each other using certificates.
I noticed line 119 to enable this. We can remove this and add it as else part in line 120.
|
|
||
| type ( | ||
| // ClientConfig contains the endpoints, CAs, and retry profile. | ||
| // MultiClientConfig contains the endpoints, CAs, and retry profile. |
| // ClientConfig contains the endpoints, CAs, and retry profile. | ||
| // MultiClientConfig contains the endpoints, CAs, and retry profile. | ||
| // This config allows the support of number of different endpoints to multiple service instances. | ||
| MultiClientConfig struct { |
There was a problem hiding this comment.
Earlier, we have used ClientConfig with Endpoints []*Endpoint. Anyway, I prefer the proposed changes as they are explicit about the content of the struct.
Type of change
Description
To support secure communication between components, we added a TLS layer to ensure encrypted message transmission.
This PR includes the following changes:
Additional details
Related issues
Partly solves epic: #19, solves #108
Next related PRs: