From 463e049cdd72c3087aefd6d2183e818cfb0e7f88 Mon Sep 17 00:00:00 2001
From: Christian Decker <decker.christian@gmail.com>
Date: Thu, 18 Jul 2024 15:34:41 +0200
Subject: [PATCH] signer: Report policy errors back to the node

When the signer rejects a request we had no indication as to what went
wrong until now. With this change we (a) send back the error as if it
were a normal response and then (b) print the error in the logs,
allowing us to collate the operations causing the rejection with the
rejection itself.
---
 .../.resources/proto/glclient/greenlight.proto       |  6 ++++++
 libs/gl-client/src/signer/mod.rs                     | 12 ++++++++++++
 libs/gl-plugin/src/hsm.rs                            |  5 ++++-
 libs/gl-plugin/src/node/mod.rs                       |  7 +++++++
 libs/proto/glclient/greenlight.proto                 |  6 ++++++
 5 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/libs/gl-client/.resources/proto/glclient/greenlight.proto b/libs/gl-client/.resources/proto/glclient/greenlight.proto
index 534d4a68c..1d468f7a8 100644
--- a/libs/gl-client/.resources/proto/glclient/greenlight.proto
+++ b/libs/gl-client/.resources/proto/glclient/greenlight.proto
@@ -69,6 +69,12 @@ message HsmResponse {
 	// A list of updated key-value-version tuples that is to be
 	// merged into the state tracked by the plugin.
 	repeated SignerStateEntry signer_state = 5;
+
+	// If the signer reported an error, and did therefore not include
+	// `raw`, this is the stringified error, so we can print it in the
+	// logs. This should help us collate policy errors with the changes
+	// proposed by CLN
+	string error = 6;
 }
 
 message HsmRequest {
diff --git a/libs/gl-client/src/signer/mod.rs b/libs/gl-client/src/signer/mod.rs
index ab3810024..84d97bf9f 100644
--- a/libs/gl-client/src/signer/mod.rs
+++ b/libs/gl-client/src/signer/mod.rs
@@ -397,6 +397,7 @@ impl Signer {
                     return Ok(());
                 }
             };
+            let request_id = req.request_id;
             let hex_req = hex::encode(&req.raw);
             let signer_state = req.signer_state.clone();
             trace!("Received request {}", hex_req);
@@ -410,6 +411,16 @@ impl Signer {
                         .map_err(|e| Error::NodeDisconnect(e))?;
                 }
                 Err(e) => {
+                    let response = HsmResponse {
+                        raw: vec![],
+                        request_id,
+                        error: format!("{:?}", e),
+                        signer_state: vec![],
+                    };
+                    client
+                        .respond_hsm_request(response)
+                        .await
+                        .map_err(|e| Error::NodeDisconnect(e))?;
                     warn!(
                         "Ignoring error {} for request {} with state {:?}",
                         e, hex_req, signer_state,
@@ -561,6 +572,7 @@ impl Signer {
             raw: response.0.as_vec(),
             request_id: req.request_id,
             signer_state,
+            error: "".to_owned(),
         })
     }
 
diff --git a/libs/gl-plugin/src/hsm.rs b/libs/gl-plugin/src/hsm.rs
index 2a90878a3..5d1a838f1 100644
--- a/libs/gl-plugin/src/hsm.rs
+++ b/libs/gl-plugin/src/hsm.rs
@@ -63,6 +63,7 @@ impl Hsm for StagingHsmServer {
                 request_id: req.request_id,
                 raw: response,
                 signer_state: Vec::new(),
+                error: "".into(),
             }));
         } else if req.get_type() == 11 {
             debug!("Returning stashed init msg: {:?}", self.node_info.initmsg);
@@ -70,6 +71,7 @@ impl Hsm for StagingHsmServer {
                 request_id: req.request_id,
                 raw: self.node_info.initmsg.clone(),
                 signer_state: Vec::new(), // the signerproxy doesn't care about state
+                error: "".into(),
             }));
         } else if req.get_type() == 33 {
             debug!("Returning stashed dev-memleak response");
@@ -77,13 +79,14 @@ impl Hsm for StagingHsmServer {
                 request_id: req.request_id,
                 raw: vec![0, 133, 0],
                 signer_state: Vec::new(), // the signerproxy doesn't care about state
+                error: "".into(),
             }));
         }
 
         let mut chan = match self.stage.send(req).await {
             Err(e) => {
                 return Err(Status::unknown(format!(
-                    "Error while queing request from node: {:?}",
+                    "Error while queuing request from node: {:?}",
                     e
                 )))
             }
diff --git a/libs/gl-plugin/src/node/mod.rs b/libs/gl-plugin/src/node/mod.rs
index 675b8ebc7..d6201c743 100644
--- a/libs/gl-plugin/src/node/mod.rs
+++ b/libs/gl-plugin/src/node/mod.rs
@@ -394,6 +394,13 @@ impl Node for PluginNodeServer {
         request: Request<pb::HsmResponse>,
     ) -> Result<Response<pb::Empty>, Status> {
         let req = request.into_inner();
+
+	if req.error != "" {
+	    log::error!("Signer reports an error: {}", req.error);
+	    log::warn!("The above error was returned instead of a response.");
+	    return Ok(Response::new(pb::Empty::default()));
+	}
+
         // Create a state from the key-value-version tuples. Need to
         // convert here, since `pb` is duplicated in the two different
         // crates.
diff --git a/libs/proto/glclient/greenlight.proto b/libs/proto/glclient/greenlight.proto
index 534d4a68c..1d468f7a8 100644
--- a/libs/proto/glclient/greenlight.proto
+++ b/libs/proto/glclient/greenlight.proto
@@ -69,6 +69,12 @@ message HsmResponse {
 	// A list of updated key-value-version tuples that is to be
 	// merged into the state tracked by the plugin.
 	repeated SignerStateEntry signer_state = 5;
+
+	// If the signer reported an error, and did therefore not include
+	// `raw`, this is the stringified error, so we can print it in the
+	// logs. This should help us collate policy errors with the changes
+	// proposed by CLN
+	string error = 6;
 }
 
 message HsmRequest {