Repository: bacalhau-project/bacalhau (language: python)

Issue:
Insecurely prints NATS auth tokens when the user supplied them
E.g.
```
% bacalhau config set node.network.type nats
% bacalhau config set node.network.authsecret kerfuffle
% bacalhau serve --peer=none
To connect to this node from the client, run the following commands in your shell:
export BACALHAU_NODE_CLIENTAPI_HOST=0.0.0.0
export BACALHAU_NODE_CLIENTAPI_PORT=1234
export BACALHAU_NODE_NETWORK_TYPE=nats
export BACALHAU_NODE_NETWORK_ORCHESTRATORS=nats://kerfuffle@127.0.0.1:4222
export BACALHAU_NODE_IPFS_SWARMADDRESSES=/ip4/127.0.0.1/tcp/61949/p2p/QmR7vHpQikS5J1U3VHhp2G3r7PSTnezmHcRVG5h6jz4ego

A copy of these variables have been written to: /var/folders/xz/qrd28pyx1095q2nzvhd9wb_c0000gn/T/tmp.ocBugqCLmd/bacalhau.run
```
The node auth secret "kerfuffle" should not be printed here as it's insecure to print secrets in system logs, and we assume if the user has supplied us the token then they are capable of adding it to the compute nodes themselves.


Discussion / hints:

Fix patch:
diff --git a/cmd/cli/serve/serve.go b/cmd/cli/serve/serve.go
index 250fd81ac4..febd5e12e7 100644
--- a/cmd/cli/serve/serve.go
+++ b/cmd/cli/serve/serve.go
@@ -400,16 +400,12 @@ func buildConnectCommand(ctx context.Context, nodeConfig *node.NodeConfig, ipfsC
 
 		switch nodeConfig.NetworkConfig.Type {
 		case models.NetworkTypeNATS:
-			advertisedAddr := nodeConfig.NetworkConfig.AdvertisedAddress
-			if advertisedAddr == "" {
-				advertisedAddr = fmt.Sprintf("127.0.0.1:%d", nodeConfig.NetworkConfig.Port)
-			}
+			advertisedAddr := getPublicNATSOrchestratorURL(nodeConfig)
 
 			headerB.WriteString("To connect a compute node to this orchestrator, run the following command in your shell:\n")
-
 			cmdB.WriteString(fmt.Sprintf("%s=%s ",
 				configflags.FlagNameForKey(types.NodeNetworkOrchestrators, configflags.NetworkFlags...),
-				advertisedAddr,
+				advertisedAddr.String(),
 			))
 
 		case models.NetworkTypeLibp2p:
@@ -471,20 +467,10 @@ func buildEnvVariables(ctx context.Context, nodeConfig *node.NodeConfig, ipfsCon
 
 		switch nodeConfig.NetworkConfig.Type {
 		case models.NetworkTypeNATS:
-			orchestrator := &url.URL{
-				Scheme: "nats",
-				Host:   nodeConfig.NetworkConfig.AdvertisedAddress,
-				User:   url.User(nodeConfig.NetworkConfig.AuthSecret),
-			}
-
-			if nodeConfig.NetworkConfig.AdvertisedAddress == "" {
-				orchestrator.Host = fmt.Sprintf("127.0.0.1:%d", nodeConfig.NetworkConfig.Port)
-			}
-
 			envVarBuilder.WriteString(fmt.Sprintf(
 				"export %s=%s\n",
 				config.KeyAsEnvVar(types.NodeNetworkOrchestrators),
-				orchestrator.String(),
+				getPublicNATSOrchestratorURL(nodeConfig).String(),
 			))
 		case models.NetworkTypeLibp2p:
 			p2pAddr, err := multiaddr.NewMultiaddr("/p2p/" + nodeConfig.NetworkConfig.Libp2pHost.ID().String())
@@ -517,6 +503,26 @@ func buildEnvVariables(ctx context.Context, nodeConfig *node.NodeConfig, ipfsCon
 	return envVarBuilder.String(), nil
 }
 
+func getPublicNATSOrchestratorURL(nodeConfig *node.NodeConfig) *url.URL {
+	orchestrator := &url.URL{
+		Scheme: "nats",
+		Host:   nodeConfig.NetworkConfig.AdvertisedAddress,
+	}
+
+	// Only display the secret if the user did not set it explicitly.
+	// Else, they should already know it!
+	secret, err := config.Get[string](types.NodeNetworkAuthSecret)
+	if err == nil && secret == "" && nodeConfig.NetworkConfig.AuthSecret != "" {
+		orchestrator.User = url.User(nodeConfig.NetworkConfig.AuthSecret)
+	}
+
+	if nodeConfig.NetworkConfig.AdvertisedAddress == "" {
+		orchestrator.Host = fmt.Sprintf("127.0.0.1:%d", nodeConfig.NetworkConfig.Port)
+	}
+
+	return orchestrator
+}
+
 // pickP2pAddress will aim to select a non-localhost IPv4 TCP address, or at least a non-localhost IPv6 one, from a list
 // of addresses.
 func pickP2pAddress(addresses []multiaddr.Multiaddr) multiaddr.Multiaddr {


Test patch:
diff --git a/test/bin/bashtub b/test/bin/bashtub
index e206eb99f6..21d8a2c0b4 100755
--- a/test/bin/bashtub
+++ b/test/bin/bashtub
@@ -52,6 +52,11 @@ assert_match_matcher() {
   [[ "$2" =~ "$1" ]]
 }
 
+assert_not_match_matcher() {
+  echo "\`$2' was not expected to match \`$1'"
+  [[ ! "$2" =~ "$1" ]]
+}
+
 assert_true_matcher() {
   echo "\`$*' was expected to return true"
   subject "$@"
diff --git a/test/node_auth.sh b/test/node_auth.sh
old mode 100644
new mode 100755
index 1f1b1bb907..49e73cafdb
--- a/test/node_auth.sh
+++ b/test/node_auth.sh
@@ -16,7 +16,23 @@ testcase_node_can_connect_with_correct_token() {
     # If this returns successfully, the node started and authenticated.
 }
 
+testcase_preconfigured_token_not_printed() {
+    subject bacalhau config set node.network.type nats
+    subject bacalhau config set node.network.authsecret kerfuffle
+    assert_equal 0 $status
+    create_node requester
+
+    subject grep BACALHAU_NODE_NETWORK_ORCHESTRATORS $BACALHAU_DIR/out.log
+    assert_equal 0 $status
+    assert_not_match kerfuffle $stdout
+
+    subject grep BACALHAU_NODE_NETWORK_ORCHESTRATORS $BACALHAU_DIR/bacalhau.run
+    assert_equal 0 $status
+    assert_not_match kerfuffle $stdout
+}
+
 testcase_node_connects_with_preconfigured_token() {
+    subject bacalhau config set node.network.type nats
     subject bacalhau config set node.network.authsecret kerfuffle
     assert_match 0 $status
     create_node requester
@@ -25,6 +41,7 @@ testcase_node_connects_with_preconfigured_token() {
     export BACALHAU_NODE_NETWORK_ORCHESTRATORS=$(echo $BACALHAU_NODE_NETWORK_ORCHESTRATORS | sed "s:[^\/]*@::")
     new_repo
     subject bacalhau config set node.network.authsecret kerfuffle
+    subject bacalhau config set node.network.type nats
     create_node compute
     # If this returns successfully, the node started and authenticated.
 }