From 82d934916b6ececb8f9e5f975723bbf487b894cb Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Fri, 14 Jul 2023 19:20:45 -0400
Subject: [PATCH] commit: Add `--sign-from-file`

Passing the private key via a direct command line argument
is just a bad idea because it's highly likely to get logged
or appear in `ps`.
Spotted in review of work for composefs signatures.
---
 man/ostree-commit.xml          | 14 +++++++-
 src/ostree/ot-builtin-commit.c | 59 ++++++++++++++++++++++------------
 tests/test-signed-pull.sh      |  5 +--
 3 files changed, 55 insertions(+), 23 deletions(-)

diff --git a/man/ostree-commit.xml b/man/ostree-commit.xml
index e9640643..3e22b172 100644
--- a/man/ostree-commit.xml
+++ b/man/ostree-commit.xml
@@ -311,10 +311,22 @@ License along with this library. If not, see <https://www.gnu.org/licenses/>.
                 </para></listitem>
 
             </varlistentry>
+
+            <varlistentry>
+                <term><option>--sign-from-file</option>="PATH"</term>
+                <listitem><para>
+                        This will read a key (corresponding to the provided <literal>--sign-type</literal> from the provided path.  The key should be base64 encoded.
+                </para></listitem>
+            </varlistentry>
+
             <varlistentry>
                 <term><option>--sign</option>="KEY-ID"</term>
                 <listitem><para>
-                        There <literal>KEY-ID</literal> is:
+                        In new code, avoid using this because passing private keys via command line arguments
+                        are prone to leakage in logs and process listings.
+                        </para>
+                        <para>
+                        The <literal>KEY-ID</literal> is:
                         <variablelist>
                             <varlistentry>
                                 <term><option>for ed25519:</option></term>
diff --git a/src/ostree/ot-builtin-commit.c b/src/ostree/ot-builtin-commit.c
index 76049671..fad6de61 100644
--- a/src/ostree/ot-builtin-commit.c
+++ b/src/ostree/ot-builtin-commit.c
@@ -68,6 +68,7 @@ static char **opt_gpg_key_ids;
 static char *opt_gpg_homedir;
 #endif
 static char **opt_key_ids;
+static char **opt_key_files;
 static char *opt_sign_name;
 static gboolean opt_generate_sizes;
 static gboolean opt_composefs_metadata;
@@ -158,6 +159,8 @@ static GOptionEntry options[] = {
     "GPG Homedir to use when looking for keyrings", "HOMEDIR" },
 #endif
   { "sign", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_key_ids, "Sign the commit with", "KEY_ID" },
+  { "sign-from-file", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_key_files,
+    "Sign the commit with key from the provided file", "PATH" },
   { "sign-type", 0, 0, G_OPTION_ARG_STRING, &opt_sign_name,
     "Signature type to use (defaults to 'ed25519')", "NAME" },
   { "generate-sizes", 0, 0, G_OPTION_ARG_NONE, &opt_generate_sizes,
@@ -918,31 +921,47 @@ ostree_builtin_commit (int argc, char **argv, OstreeCommandInvocation *invocatio
             goto out;
         }
 
-      if (opt_key_ids)
+      const char *sign_name = opt_sign_name ?: OSTREE_SIGN_NAME_ED25519;
+      if (opt_key_files || opt_key_ids)
         {
-          /* Initialize crypto system */
-          opt_sign_name = opt_sign_name ?: OSTREE_SIGN_NAME_ED25519;
-
-          sign = ostree_sign_get_by_name (opt_sign_name, error);
+          sign = ostree_sign_get_by_name (sign_name, error);
           if (sign == NULL)
             goto out;
-
-          char **iter;
-
-          for (iter = opt_key_ids; iter && *iter; iter++)
-            {
-              const char *keyid = *iter;
-              g_autoptr (GVariant) secret_key = NULL;
-
-              secret_key = g_variant_new_string (keyid);
-              if (!ostree_sign_set_sk (sign, secret_key, error))
-                goto out;
-
-              if (!ostree_sign_commit (sign, repo, commit_checksum, cancellable, error))
-                goto out;
-            }
         }
 
+      // Loop over the inline private keys on the command line; this should be
+      // avoided in new code in favor of --sign-from-file.
+      for (char **iter = opt_key_ids; iter && *iter; iter++)
+        {
+          const char *keyid = *iter;
+          g_autoptr (GVariant) secret_key = g_variant_new_string (keyid);
+          g_assert (sign);
+          if (!ostree_sign_set_sk (sign, secret_key, error))
+            goto out;
+
+          if (!ostree_sign_commit (sign, repo, commit_checksum, cancellable, error))
+            goto out;
+        }
+
+      // Load each base64 encoded private key in a file and sign with it.
+      for (char **iter = opt_key_files; iter && *iter; iter++)
+        {
+          const char *path = *iter;
+          g_autofree char *b64key
+              = glnx_file_get_contents_utf8_at (AT_FDCWD, path, NULL, NULL, error);
+          if (!b64key)
+            {
+              g_prefix_error (error, "Reading %s", path);
+              goto out;
+            }
+          g_autoptr (GVariant) secret_key = g_variant_new_string (b64key);
+          g_assert (sign);
+          if (!ostree_sign_set_sk (sign, secret_key, error))
+            goto out;
+
+          if (!ostree_sign_commit (sign, repo, commit_checksum, cancellable, error))
+            goto out;
+        }
 #ifndef OSTREE_DISABLE_GPGME
       if (opt_gpg_key_ids)
         {
diff --git a/tests/test-signed-pull.sh b/tests/test-signed-pull.sh
index 372287f6..7a42357b 100755
--- a/tests/test-signed-pull.sh
+++ b/tests/test-signed-pull.sh
@@ -157,8 +157,9 @@ gen_ed25519_keys
 PUBLIC=${ED25519PUBLIC}
 SEED=${ED25519SEED}
 SECRET=${ED25519SECRET}
-
-COMMIT_ARGS="--sign=${SECRET} --sign-type=ed25519"
+# Other tests verify --sign, we will verify --sign-from-file here
+echo ${ED25519SECRET} > key
+COMMIT_ARGS="--sign-from-file=key --sign-type=ed25519"
 
 repo_init --set=sign-verify=true
 ${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-ed25519-key "${PUBLIC}"