Enable TLS for Fabric-X Committer Container#1298
Enable TLS for Fabric-X Committer Container#1298Rozerxshashank wants to merge 1 commit intohyperledger-labs:mainfrom
Conversation
Rozerxshashank
force-pushed
the
issue-1295-fabricx-tls
branch
from
633cb16 to
3b7e431
Compare
mbrandenburger
left a comment
mbrandenburger
left a comment
There was a problem hiding this comment.
Thank you @Rozerxshashank for your PR! This is a solid start.
Please check integration/nwo/fabricx/topology.go:30 - where we currently disable TLS for fabricx topos - that needs to be removed in order to see if your proposed changes work :D
A few more comments below.
| } | ||
| if tlsEnabled { | ||
| env = append(env, | ||
| "SC_SIDECAR_ORDERER_TLS_MODE=tls", |
There was a problem hiding this comment.
we could even try mtls mode here.
There was a problem hiding this comment.
Note that this configures the "orderer client" of the sidecar. In addition to that, we need tls configuration for the mock orderer, the the query service using the crypto material we inject into the container.
There was a problem hiding this comment.
Updated SC_SIDECAR_ORDERER_TLS_MODE to mtls in the latest commit.
There was a problem hiding this comment.
Understood. Removing the --insecure flag now correctly triggers the use of the injected crypto material for all container components, ensuring the entire setup is consistent with the network's TLS settings
| var ContainerCmd = []string{"run", "db", "orderer", "committer", "--insecure"} | ||
| var ContainerCmd = []string{"run", "db", "orderer", "committer"} |
There was a problem hiding this comment.
Note that by removing --insecure you enable mtls for every component in the committer image. See here https://github.com/hyperledger/fabric-x-committer/blob/main/docker/images/test_node/run#L12
However, I believe for our tests it is only relevant that the components that are exposed to FSC MUST enabled TLS. All other communication between committer components can be insecure for our testing purpose.
If we remove --insecure, by default the built-in crypto material in the container is used to configure TLS; we need to carefully wire the things together; that is ... every component that is exposed to FSC need to use crypto material we inject into the container; all other components can be either insecure - or use the built-in material (if that is possible).
WDYT?
There was a problem hiding this comment.
I've implemented conditional logic for the --insecure flag. It's now only added if TLS is disabled globally in the network topology. When TLSEnabled is true, we remove the flag to ensure all exposed components use the injected crypto material, aligning the sidecar with the rest of the FSC environment.
| peerTLSDir := peerDockerTLSDir(e.network, scPeer) | ||
| ordererTLSCACert := path.Join("/", "root", "artifacts", "crypto", "ordererOrganizations", e.network.OrdererOrgs()[0].Domain, "orderers", fmt.Sprintf("%s.%s", e.network.Orderers[0].Name, e.network.OrdererOrgs()[0].Domain), "tls", "ca.crt") | ||
|
|
||
| containerEnvOverride := envVars[committerVersion](peerMSPDir, peerTLSDir, scMSPID, e.channel.Name, ordererEndpoint, e.network.TLSEnabled, ordererTLSCACert) |
There was a problem hiding this comment.
I know it was misleading already before, this is the chance we fix the bad naming. We should use scMSPID, scMSPDir, and scTLSDir as all belongs to the committer.
There was a problem hiding this comment.
Renamed peerMSPDir to scMSPDir and peerTLSDir to scTLSDir across the scv2 and v3 extensions to clarify that they belong to the sidecar committer. I've also updated the ContainerEnvVars signature accordingly. :D
| return filepath.Join( | ||
| return path.Join( |
There was a problem hiding this comment.
I initially used path.Join because these paths are used inside the Linux container (where forward slashes are required). However, I've switched them back to filepath.Join to maintain consistency with the rest of the project. Fixed in the latest commit.
Rozerxshashank
force-pushed
the
issue-1295-fabricx-tls
branch
14 times, most recently
from
40639ad to
3b7e431
Compare
… tests Signed-off-by: Shashank <yshashank959@gmail.com>
Rozerxshashank
force-pushed
the
issue-1295-fabricx-tls
branch
from
3b7e431 to
b7ac71f
Compare
2 participants
Closes: #1295
Description
This PR enables TLS for the Fabric-X committer all-in-one test container. It ensures secure communication between the sidecar, the orderer, and the query services by removing the insecure default settings.
Key Changes
--insecureflag from the committer container command.DeployNamespaceandtryListInstalledNamesto respect the network'sTLSEnabledstatus.