Expose node public port correctly (#2035)

achamayou · web-flow · commit 7f44044edf47 · 2020-12-18T15:28:24.000Z
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Changed
 
 - Snapshots are generated by default on the current primary node, every `10,000` committed transaction (#2029).
+- Node information exposed in the API now correctly reports the public port when it differs from the local one. (#2001)
 - All `/gov` endpoints accept signature authentication again. Read-only `/gov` endpoints had been incorrectly changed in [0.16.1] to accept session certification authentication only (#2033).
 
 ## [0.16.1]
diff --git a/src/host/main.cpp b/src/host/main.cpp
@@ -624,6 +624,10 @@ int main(int argc, char** argv)
         fmt::format("{}\n{}", rpc_address.hostname, rpc_address.port),
         rpc_address_file);
     }
+    if (public_rpc_address.port == "0")
+    {
+      public_rpc_address.port = rpc_address.port;
+    }
 
     // Initialise the enclave and create a CCF node in it
     const size_t certificate_size = 4096;
@@ -655,7 +659,8 @@ int main(int argc, char** argv)
                                     public_rpc_address.hostname,
                                     node_address.hostname,
                                     node_address.port,
-                                    rpc_address.port};
+                                    rpc_address.port,
+                                    public_rpc_address.port};
     ccf_config.domain = domain;
     ccf_config.snapshot_tx_interval = snapshot_tx_interval;
 
diff --git a/src/node/node_info_network.h b/src/node/node_info_network.h
@@ -17,10 +17,11 @@ namespace ccf
     std::string nodehost;
     std::string nodeport;
     std::string rpcport;
+    std::string pubport;
 
-    MSGPACK_DEFINE(rpchost, pubhost, nodehost, nodeport, rpcport);
+    MSGPACK_DEFINE(rpchost, pubhost, nodehost, nodeport, rpcport, pubport);
   };
   DECLARE_JSON_TYPE(NodeInfoNetwork);
   DECLARE_JSON_REQUIRED_FIELDS(
-    NodeInfoNetwork, rpchost, pubhost, nodehost, nodeport, rpcport);
+    NodeInfoNetwork, rpchost, pubhost, nodehost, nodeport, rpcport, pubport);
 }
diff --git a/src/node/rpc/common_endpoint_registry.h b/src/node/rpc/common_endpoint_registry.h
@@ -173,7 +173,7 @@ namespace ccf
             GetPrimaryInfo::Out out;
             out.primary_id = primary_id;
             out.primary_host = info->pubhost;
-            out.primary_port = info->rpcport;
+            out.primary_port = info->pubport;
             out.current_view = current_view;
             return make_success(out);
           }
@@ -202,7 +202,7 @@ namespace ccf
         nodes_view->foreach([&out](const NodeId& nid, const NodeInfo& ni) {
           if (ni.status == ccf::NodeStatus::TRUSTED)
           {
-            out.nodes.push_back({nid, ni.pubhost, ni.rpcport});
+            out.nodes.push_back({nid, ni.pubhost, ni.pubport});
           }
           return true;
         });
@@ -242,7 +242,7 @@ namespace ccf
         auto nodes_view =
           args.tx.template get_read_only_view<Nodes>(Tables::NODES);
         nodes_view->foreach([&in, &out](const NodeId& nid, const NodeInfo& ni) {
-          if (ni.rpchost == in.host && ni.rpcport == in.port)
+          if (ni.pubhost == in.host && ni.pubport == in.port)
           {
             if (ni.status != ccf::NodeStatus::RETIRED || in.retired)
             {
diff --git a/src/node/rpc/frontend.h b/src/node/rpc/frontend.h
@@ -135,7 +135,7 @@ namespace ccf
           {
             ctx->set_response_header(
               http::headers::LOCATION,
-              fmt::format("{}:{}", info->pubhost, info->rpcport));
+              fmt::format("{}:{}", info->pubhost, info->pubport));
           }
         }
 
diff --git a/src/node/rpc/node_frontend.h b/src/node/rpc/node_frontend.h
@@ -367,7 +367,7 @@ namespace ccf
               args.rpc_ctx->set_response_header(
                 "Location",
                 fmt::format(
-                  "https://{}:{}/node/primary", info->pubhost, info->rpcport));
+                  "https://{}:{}/node/primary", info->pubhost, info->pubport));
             }
           }
         }
diff --git a/tests/code_update.py b/tests/code_update.py
@@ -40,7 +40,7 @@ def test_verify_quotes(network, args):
         assert (
             infra.proc.ccall(
                 "verify_quote.sh",
-                f"https://{node.pubhost}:{node.rpc_port}",
+                f"https://{node.pubhost}:{node.pubport}",
                 "--cacert",
                 f"{cafile}",
                 log_output=True,
diff --git a/tests/e2e_logging.py b/tests/e2e_logging.py
@@ -58,7 +58,7 @@ def test_illegal(network, args, verify=True):
     )
     sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
     conn = context.wrap_socket(sock, server_side=False, server_hostname=primary.host)
-    conn.connect((primary.host, primary.rpc_port))
+    conn.connect((primary.host, primary.pubport))
     conn.sendall(b"NOTAVERB ")
     rv = conn.recv(1024)
     assert rv == b"", rv
@@ -593,7 +593,7 @@ def test_primary(network, args):
         assert r.status_code == http.HTTPStatus.PERMANENT_REDIRECT.value
         assert (
             r.headers["location"]
-            == f"https://{primary.pubhost}:{primary.rpc_port}/node/primary"
+            == f"https://{primary.pubhost}:{primary.pubport}/node/primary"
         )
     return network
 
diff --git a/tests/e2e_scenarios.py b/tests/e2e_scenarios.py
@@ -76,9 +76,9 @@ def run(args):
 
     if args.network_only:
         LOG.info("Keeping network alive with the following nodes:")
-        LOG.info("  Primary = {}:{}".format(primary.pubhost, primary.rpc_port))
+        LOG.info("  Primary = {}:{}".format(primary.pubhost, primary.pubport))
         for i, f in enumerate(backups):
-            LOG.info("  Backup[{}] = {}:{}".format(i, f.pubhost, f.rpc_port))
+            LOG.info("  Backup[{}] = {}:{}".format(i, f.pubhost, f.pubport))
 
         input("Press Enter to shutdown...")
 
diff --git a/tests/governance.py b/tests/governance.py
@@ -83,7 +83,7 @@ def test_quote(network, args, verify=True):
                 assert "Evidence verification succeeded (0)." in out
 
             node = network.nodes[quote["node_id"]]
-            node_cert = ssl.get_server_certificate((node.pubhost, node.rpc_port))
+            node_cert = ssl.get_server_certificate((node.pubhost, node.pubport))
             public_key = x509.load_pem_x509_certificate(
                 node_cert.encode(), default_backend()
             ).public_key()
@@ -198,7 +198,7 @@ def test_node_ids(network, args):
     nodes = network.find_nodes()
     for node in nodes:
         with node.client() as c:
-            r = c.get(f'/node/node/ids?host="{node.pubhost}"&port="{node.rpc_port}"')
+            r = c.get(f'/node/node/ids?host="{node.pubhost}"&port="{node.pubport}"')
             assert r.status_code == 200
             assert r.body.json()["nodes"] == [
                 {"node_id": node.node_id, "status": "TRUSTED"}
diff --git a/tests/infra/member.py b/tests/infra/member.py
@@ -179,7 +179,7 @@ def get_and_submit_recovery_share(self, remote_node):
 
         res = infra.proc.ccall(
             self.share_script,
-            f"https://{remote_node.host}:{remote_node.rpc_port}",
+            f"https://{remote_node.pubhost}:{remote_node.pubport}",
             "--member-enc-privk",
             os.path.join(self.common_dir, f"member{self.member_id}_enc_privk.pem"),
             "--cert",
diff --git a/tests/infra/network.py b/tests/infra/network.py
@@ -149,8 +149,8 @@ def _adjust_local_node_ids(self, primary):
         with primary.client() as nc:
             r = nc.get("/node/primary_info")
             first_node_id = r.body.json()["primary_id"]
-            assert (r.body.json()["primary_host"] == primary.host) and (
-                int(r.body.json()["primary_port"]) == primary.rpc_port
+            assert (r.body.json()["primary_host"] == primary.pubhost) and (
+                int(r.body.json()["primary_port"]) == primary.pubport
             ), "Primary is not the node that just started"
             for n in self.nodes:
                 n.node_id = n.node_id + first_node_id
diff --git a/tests/infra/node.py b/tests/infra/node.py
@@ -64,15 +64,20 @@ def __init__(
         else:
             assert False, f"{host} does not start with 'local://' or 'ssh://'"
 
-        hosts, *port = host[host.find("/") + 2 :].split(":")
-        self.host, *self.pubhost = hosts.split(",")
-        self.rpc_port = int(port[0]) if port else None
-        self.node_port = None
+        host_, *pubhost_ = host.split(",")
 
+        self.host, *port = host_[host_.find("/") + 2 :].split(":")
+        self.rpc_port = int(port[0]) if port else None
         if self.host == "localhost":
             self.host = infra.net.expand_localhost()
 
-        self.pubhost = self.pubhost[0] if self.pubhost else self.host
+        if pubhost_:
+            self.pubhost, *pubport = pubhost_[0].split(":")
+            self.pubport = int(pubport[0]) if pubport else self.rpc_port
+        else:
+            self.pubhost = self.host
+            self.pubport = self.rpc_port
+        self.node_port = None
 
     def __hash__(self):
         return self.node_id
@@ -233,6 +238,8 @@ def _read_ports(self):
                     rpc_port == self.rpc_port
                 ), f"Unexpected change in RPC port from {self.rpc_port} to {rpc_port}"
             self.rpc_port = rpc_port
+            if self.pubport is None:
+                self.pubport = self.rpc_port
 
     def stop(self):
         if self.remote and self.network_state is not NodeNetworkState.stopped:
@@ -317,7 +324,7 @@ def client(self, identity=None, **kwargs):
             }
         )
         akwargs.update(kwargs)
-        return ccf.clients.client(self.pubhost, self.rpc_port, **akwargs)
+        return ccf.clients.client(self.pubhost, self.pubport, **akwargs)
 
     def suspend(self):
         self.remote.suspend()
diff --git a/tests/start_network.py b/tests/start_network.py
@@ -72,14 +72,14 @@ def pad_node_id(nid):
         LOG.info("Started CCF network with the following nodes:")
         LOG.info(
             "  Node [{}] = https://{}:{}".format(
-                pad_node_id(primary.node_id), primary.pubhost, primary.rpc_port
+                pad_node_id(primary.node_id), primary.pubhost, primary.pubport
             )
         )
 
         for b in backups:
             LOG.info(
                 "  Node [{}] = https://{}:{}".format(
-                    pad_node_id(b.node_id), b.pubhost, b.rpc_port
+                    pad_node_id(b.node_id), b.pubhost, b.pubport
                 )
             )
 
@@ -112,7 +112,7 @@ def add(parser):
         parser.add_argument(
             "-n",
             "--node",
-            help=f"List of (local://|ssh://)hostnames[,pub_hostnames:ports]. Default is {DEFAULT_NODES}",
+            help=f"List of (local://|ssh://)hostname:port[,pub_hostnames:pub_port]. Default is {DEFAULT_NODES}",
             action="append",
         )
         parser.add_argument(
diff --git a/tests/tlstest.py b/tests/tlstest.py
@@ -12,7 +12,7 @@
 @reqs.at_least_n_nodes(1)
 def test(network, args):
     node = network.nodes[0]
-    endpoint = f"https://{node.host}:{node.rpc_port}"
+    endpoint = f"https://{node.pubhost}:{node.pubport}"
     r = subprocess.run(
         ["testssl/testssl.sh", "--outfile", "tls_report", endpoint], check=False
     )
diff --git a/tests/vegeta_stress.py b/tests/vegeta_stress.py
@@ -33,7 +33,7 @@ def run(args, additional_attack_args):
         network.start_and_join(args)
 
         primary, _ = network.find_primary()
-        primary_hostname = f"{primary.pubhost}:{primary.rpc_port}"
+        primary_hostname = f"{primary.pubhost}:{primary.pubport}"
 
         vegeta_targets = "vegeta_targets"
         with open(vegeta_targets, "w") as f:
diff --git a/tests/zap.py b/tests/zap.py
@@ -12,7 +12,7 @@
 @reqs.at_least_n_nodes(1)
 def test(network, args):
     node = network.nodes[0]
-    openapi_endpoint = f"https://{node.host}:{node.rpc_port}/node/api"
+    openapi_endpoint = f"https://{node.pubhost}:{node.pubport}/node/api"
 
     args = [
         "docker",

Original file line number	Diff line number	Diff line change
`@@ -173,7 +173,7 @@ namespace ccf`
`173`	`173`	`GetPrimaryInfo::Out out;`
`174`	`174`	`out.primary_id = primary_id;`
`175`	`175`	`out.primary_host = info->pubhost;`
`176`		`- out.primary_port = info->rpcport;`
	`176`	`+ out.primary_port = info->pubport;`
`177`	`177`	`out.current_view = current_view;`
`178`	`178`	`return make_success(out);`
`179`	`179`	`}`
`@@ -202,7 +202,7 @@ namespace ccf`
`202`	`202`	`nodes_view->foreach([&out](const NodeId& nid, const NodeInfo& ni) {`
`203`	`203`	`if (ni.status == ccf::NodeStatus::TRUSTED)`
`204`	`204`	`{`
`205`		`- out.nodes.push_back({nid, ni.pubhost, ni.rpcport});`
	`205`	`+ out.nodes.push_back({nid, ni.pubhost, ni.pubport});`
`206`	`206`	`}`
`207`	`207`	`return true;`
`208`	`208`	`});`
`@@ -242,7 +242,7 @@ namespace ccf`
`242`	`242`	`auto nodes_view =`
`243`	`243`	`args.tx.template get_read_only_view<Nodes>(Tables::NODES);`
`244`	`244`	`nodes_view->foreach([&in, &out](const NodeId& nid, const NodeInfo& ni) {`
`245`		`- if (ni.rpchost == in.host && ni.rpcport == in.port)`
	`245`	`+ if (ni.pubhost == in.host && ni.pubport == in.port)`
`246`	`246`	`{`
`247`	`247`	`if (ni.status != ccf::NodeStatus::RETIRED \|\| in.retired)`
`248`	`248`	`{`
Original file line number	Diff line number	Diff line change
`@@ -135,7 +135,7 @@ namespace ccf`
`135`	`135`	`{`
`136`	`136`	`ctx->set_response_header(`
`137`	`137`	`http::headers::LOCATION,`
`138`		`- fmt::format("{}:{}", info->pubhost, info->rpcport));`
	`138`	`+ fmt::format("{}:{}", info->pubhost, info->pubport));`
`139`	`139`	`}`
`140`	`140`	`}`
`141`	`141`
Original file line number	Diff line number	Diff line change
`@@ -367,7 +367,7 @@ namespace ccf`
`367`	`367`	`args.rpc_ctx->set_response_header(`
`368`	`368`	`"Location",`
`369`	`369`	`fmt::format(`
`370`		`- "https://{}:{}/node/primary", info->pubhost, info->rpcport));`
	`370`	`+ "https://{}:{}/node/primary", info->pubhost, info->pubport));`
`371`	`371`	`}`
`372`	`372`	`}`
`373`	`373`	`}`