Fix gpg hanging waiting for input when more than one key is present in keychain (#5471)

Mill's artifact signing for publishing needs `gpg` to import the signing
keys. However, the current implementation naively assumes that the key
it imports is the only key.

If you have other keys in your keychain, a couple of things could
happen:

- `gpg` by default signs using the first key in your keychain, which
isn't necessarily the key you just imported, so your artifact can be
signed using the wrong key.
- If the key `gpg` picks has a passphrase, `gpg` will hang indefinitely
waiting for you to enter the passphrase to stdin, which will never
happen, as it's mill handling the stdin.

This PR ensures that when the key is imported into GPG, its ID is
extracted and then used in the signing commands to specify which key to
use.

Tested manually on Linux and Mac.

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

Author: arturaz

contrib/sonatypecentral/readme.adoc

@@ -30,7 +30,7 @@ $ mill -i \
 mill.contrib.sonatypecentral/publishAll \
 --username myusername \
 --password mypassword \
---gpgArgs --passphrase=$MILL_PGP_PASSPHRASE,--no-tty,--pinentry-mode,loopback,--batch,--yes,-a,-b \
+--gpgArgs --passphrase=$MILL_PGP_PASSPHRASE,--no-tty,--pinentry-mode,loopback,--batch,--yes,--armor,--detach-sign \
 --publishArtifacts __.publishArtifacts \
 --readTimeout  36000 \
 --awaitTimeout 36000 \
@@ -69,7 +69,7 @@ The `mill.contrib.sonatypecentral/publishAll` method takes the following argumen
 
 `password`: The password for calling the Sonatype Central publishing api. Defaults to the `SONATYPE_PASSWORD` environment variable if unset. If neither the parameter nor the environment variable are set, an error will be thrown. +
 
-`gpgArgs`: Arguments to pass to the gpg package for signing artifacts. Uses the `MILL_PGP_PASSPHRASE` environment variable if set. _Default: `[--passphrase=$MILL_PGP_PASSPHRASE], --no-tty, --pinentry-mode, loopback, --batch, --yes, -a, -b`._ +
+`gpgArgs`: Arguments to pass to the gpg package for signing artifacts. Uses the `MILL_PGP_PASSPHRASE` environment variable if set. _Default: `[--passphrase=$MILL_PGP_PASSPHRASE], --no-tty, --pinentry-mode, loopback, --batch, --yes, --armor, --detach-sign`._ +
 
 `publishArtifacts`: The command for generating all publishable artifacts (ex. `__.publishArtifacts`). Required. +
 

libs/javalib/src/mill/javalib/PublishModule.scala

@@ -510,14 +510,13 @@ trait PublishModule extends JavaModule { outer =>
       stagingRelease: Boolean = true
   ): Task.Command[Unit] = Task.Command {
     val PublishModule.PublishData(artifactInfo, artifacts) = publishArtifacts()
-    PublishModule.pgpImportSecretIfProvided(Task.env)
+    val gpgArgs0 = PublishModule.pgpImportSecretIfProvidedAndMakeGpgArgs(Task.env, gpgArgs)
     new SonatypePublisher(
       sonatypeUri,
       sonatypeSnapshotUri,
       checkSonatypeCreds(sonatypeCreds)(),
       signed,
-      if (gpgArgs.isEmpty) PublishModule.defaultGpgArgsForPassphrase(Task.env.get("PGP_PASSPHRASE"))
-      else gpgArgs,
+      gpgArgs0,
       readTimeout,
       connectTimeout,
       Task.log,
@@ -545,26 +544,128 @@ trait PublishModule extends JavaModule { outer =>
 object PublishModule extends ExternalModule with TaskModule {
   def defaultTask(): String = "publishAll"
   val defaultGpgArgs: Seq[String] = defaultGpgArgsForPassphrase(None)
-  def pgpImportSecretIfProvided(env: Map[String, String]): Unit = {
-    for (secret <- env.get("MILL_PGP_SECRET_BASE64")) {
-      os.call(
-        ("gpg", "--import", "--no-tty", "--batch", "--yes"),
-        stdin = java.util.Base64.getDecoder.decode(secret)
-      )
+
+  /**
+   * Imports a Base64 encoded GPG secret, if one is provided in the environment.
+   *
+   * @return Some(Right(the key ID of the imported secret)), Some(Left(error message)) if the import failed, None if
+   *         the environment variable is not set.
+   */
+  def pgpImportSecretIfProvided(env: Map[String, String]): Option[Either[String, String]] = {
+    for (secret <- env.get(EnvVarPgpSecretBase64)) yield {
+      pgpImportSecret(secret).left.map { errorLines =>
+        s"""Could not import PGP secret from environment variable '$EnvVarPgpSecretBase64'. gpg output:
+           |
+           |${errorLines.mkString("\n")}""".stripMargin
+      }
     }
   }
 
-  def defaultGpgArgsForPassphrase(passphrase: Option[String]): Seq[String] = {
-    passphrase.map("--passphrase=" + _).toSeq ++
-      Seq(
-        "--no-tty",
-        "--pinentry-mode",
-        "loopback",
-        "--batch",
-        "--yes",
-        "-a",
-        "-b"
+  /** Imports a Base64 encoded GPG secret, if one is provided in the environment. Throws if the import fails. */
+  def pgpImportSecretIfProvidedOrThrow(env: Map[String, String]): Option[String] =
+    pgpImportSecretIfProvided(env).map(_.fold(
+      err => throw new IllegalArgumentException(err),
+      identity
+    ))
+
+  /**
+   * Imports a Base64 encoded GPG secret.
+   *
+   * @return Right(the key ID of the imported secret), or Left(gnupg output) if the import failed.
+   */
+  def pgpImportSecret(secretBase64: String): Either[Vector[String], String] = {
+    val cmd = Seq(
+      "gpg",
+      "--import",
+      "--no-tty",
+      "--batch",
+      "--yes",
+      // Use the machine parseable output format and send it to stdout.
+      "--with-colons",
+      "--status-fd",
+      "1"
+    )
+    val res = os.call(cmd, stdin = java.util.Base64.getDecoder.decode(secretBase64))
+    val outLines = res.out.lines()
+    val importRegex = """^\[GNUPG:\] IMPORT_OK \d+ (\w+)""".r
+    outLines.collectFirst { case importRegex(key) => key }.toRight(outLines)
+  }
+
+  def defaultGpgArgsForPassphrase(passphrase: Option[GpgKey]): Seq[String] = {
+    passphrase.iterator.flatMap(_.gpgArgs).toSeq ++ Seq(
+      "--no-tty",
+      "--pinentry-mode",
+      "loopback",
+      "--batch",
+      "--yes",
+      "--armor",
+      "--detach-sign"
+    )
+  }
+
+  def pgpImportSecretIfProvidedAndMakeGpgArgs(
+      env: Map[String, String],
+      providedGpgArgs: Seq[String]
+  ): Seq[String] = {
+    val maybeKeyId = pgpImportSecretIfProvidedOrThrow(env)
+    makeGpgArgs(env, maybeKeyId, providedGpgArgs)
+  }
+
+  def makeGpgArgs(
+      env: Map[String, String],
+      maybeKeyId: Option[String],
+      providedGpgArgs: Seq[String]
+  ): Seq[String] = {
+    if (providedGpgArgs.nonEmpty) providedGpgArgs
+    else {
+      val maybePassphrase = GpgKey.createFromEnvVarsOrThrow(
+        maybeKeyId = maybeKeyId,
+        maybePassphrase = env.get(EnvVarPgpPassphrase)
       )
+      defaultGpgArgsForPassphrase(maybePassphrase)
+    }
+  }
+
+  val EnvVarPgpPassphrase = "MILL_PGP_PASSPHRASE"
+  val EnvVarPgpSecretBase64 = "MILL_PGP_SECRET_BASE64"
+
+  case class GpgKey private (keyId: String, passphrase: Option[String]) {
+    def gpgArgs: Seq[String] =
+      Seq("--local-user", keyId) ++ passphrase.iterator.flatMap(p => Seq("--passphrase", p))
+  }
+
+  object GpgKey {
+
+    /** Creates an instance if the passphrase is not empty. */
+    def apply(keyId: String, passphrase: Option[String]): GpgKey =
+      new GpgKey(keyId = keyId, passphrase = passphrase.filter(_.nonEmpty))
+
+    /** Creates an instance if the passphrase is not empty. */
+    def apply(keyId: String, passphrase: String): GpgKey =
+      new GpgKey(keyId = keyId, passphrase = if (passphrase.isEmpty) None else Some(passphrase))
+
+    /**
+     * @param maybeKeyId      will be [[None]] if the PGP key was not provided in the environment.
+     * @param maybePassphrase will be [[None]] if the PGP passphrase was not provided in the environment.
+     */
+    def createFromEnvVars(
+        maybeKeyId: Option[String],
+        maybePassphrase: Option[String]
+    ): Option[Either[String, GpgKey]] =
+      (maybeKeyId, maybePassphrase) match {
+        case (None, None) => None
+        case (Some(keyId), maybePassphrase) => Some(Right(apply(keyId = keyId, maybePassphrase)))
+        // If passphrase is provided, key is required.
+        case (None, Some(_)) =>
+          Some(Left("A passphrase was provided, but key was not successfully imported."))
+      }
+
+    def createFromEnvVarsOrThrow(
+        maybeKeyId: Option[String],
+        maybePassphrase: Option[String]
+    ): Option[GpgKey] =
+      createFromEnvVars(maybeKeyId, maybePassphrase)
+        .map(_.fold(err => throw new IllegalArgumentException(err), identity))
   }
 
   case class PublishData(meta: Artifact, payload: Seq[(PathRef, String)]) {
@@ -592,7 +693,7 @@ object PublishModule extends ExternalModule with TaskModule {
    *                      <i>Note: consider using environment variables over this argument due
    *                      to security reasons.</i>
    * @param signed
-   * @param gpgArgs       GPG arguments. Defaults to `--passphrase=$MILL_PGP_PASSPHRASE,--no-tty,--pienty-mode,loopback,--batch,--yes,-a,-b`.
+   * @param gpgArgs       GPG arguments. Defaults to [[defaultGpgArgsForPassphrase]].
    *                      Specifying this will override/remove the defaults.
    *                      Add the default args to your args to keep them.
    * @param release Whether to release the artifacts after staging them
@@ -624,15 +725,14 @@ object PublishModule extends ExternalModule with TaskModule {
       case PublishModule.PublishData(a, s) => (s.map { case (p, f) => (p.path, f) }, a)
     }
 
-    pgpImportSecretIfProvided(Task.env)
+    val gpgArgs0 = pgpImportSecretIfProvidedAndMakeGpgArgs(Task.env, gpgArgs.split(','))
 
     new SonatypePublisher(
       sonatypeUri,
       sonatypeSnapshotUri,
       checkSonatypeCreds(sonatypeCreds)(),
       signed,
-      if (gpgArgs.isEmpty) defaultGpgArgsForPassphrase(Task.env.get("MILL_PGP_PASSPHRASE"))
-      else gpgArgs.split(','),
+      gpgArgs0,
       readTimeout,
       connectTimeout,
       Task.log,

libs/javalib/src/mill/javalib/SonatypeCentralPublishModule.scala

@@ -23,8 +23,12 @@ import mill.scalalib.publish.SonatypeHelpers.{
 import mill.api.BuildCtx
 
 trait SonatypeCentralPublishModule extends PublishModule {
-  def sonatypeCentralGpgArgs: T[String] = Task {
-    PublishModule.defaultGpgArgsForPassphrase(Task.env.get("MILL_PGP_PASSPHRASE")).mkString(",")
+
+  /**
+   * @return (keyId => gpgArgs), where maybeKeyId is the PGP key that was imported and should be used for signing.
+   */
+  def sonatypeCentralGpgArgs: Task[String => Seq[String]] = Task.Anon { (keyId: String) =>
+    PublishModule.makeGpgArgs(Task.env, maybeKeyId = Some(keyId), providedGpgArgs = Seq.empty)
   }
 
   def sonatypeCentralConnectTimeout: T[Int] = Task { defaultConnectTimeout }
@@ -42,12 +46,19 @@ trait SonatypeCentralPublishModule extends PublishModule {
     Task.Command {
       val publishData = publishArtifacts()
       val fileMapping = publishData.withConcretePath._1
+
+      val maybeKeyId = PublishModule.pgpImportSecretIfProvidedOrThrow(Task.env)
+      val keyId = maybeKeyId.getOrElse(throw new IllegalArgumentException(
+        s"Publishing to Sonatype Central requires a PGP key. Please set the '${PublishModule.EnvVarPgpSecretBase64}' " +
+          s"and '${PublishModule.EnvVarPgpPassphrase}' (if needed) environment variables."
+      ))
+
+      val gpgArgs = sonatypeCentralGpgArgs()(keyId)
       val artifact = publishData.meta
       val finalCredentials = getSonatypeCredentials(username, password)()
-      PublishModule.pgpImportSecretIfProvided(Task.env)
       val publisher = new SonatypeCentralPublisher(
         credentials = finalCredentials,
-        gpgArgs = sonatypeCentralGpgArgs().split(",").toIndexedSeq,
+        gpgArgs = gpgArgs,
         connectTimeout = sonatypeCentralConnectTimeout(),
         readTimeout = sonatypeCentralReadTimeout(),
         log = Task.log,
@@ -98,13 +109,11 @@ object SonatypeCentralPublishModule extends ExternalModule with TaskModule {
 
     val finalBundleName = if (bundleName.isEmpty) None else Some(bundleName)
     val finalCredentials = getSonatypeCredentials(username, password)()
-    PublishModule.pgpImportSecretIfProvided(Task.env)
+    val gpgArgs0 =
+      PublishModule.pgpImportSecretIfProvidedAndMakeGpgArgs(Task.env, gpgArgs.split(','))
     val publisher = new SonatypeCentralPublisher(
       credentials = finalCredentials,
-      gpgArgs = gpgArgs match {
-        case "" => PublishModule.defaultGpgArgsForPassphrase(Task.env.get("MILL_PGP_PASSPHRASE"))
-        case gpgArgs => gpgArgs.split(",").toIndexedSeq
-      },
+      gpgArgs = gpgArgs0,
       connectTimeout = connectTimeout,
       readTimeout = readTimeout,
       log = Task.log,

website/docs/modules/ROOT/partials/Publishing_Footer.adoc

@@ -261,7 +261,7 @@ The `./mill mill.javalib.SonatypeCentralPublishModule/publishAll` takes the foll
 
 `password`: The password for calling the Sonatype Central publishing api. Defaults to the `SONATYPE_PASSWORD` environment variable if unset. If neither the parameter nor the environment variable are set, an error will be thrown. +
 
-`gpgArgs`: Arguments to pass to the gpg package for signing artifacts. Uses the `MILL_PGP_PASSPHRASE` environment variable if set. _Default: `[--passphrase=$MILL_PGP_PASSPHRASE], --no-tty, --pinentry-mode, loopback, --batch, --yes, -a, -b`._ +
+`gpgArgs`: Arguments to pass to the gpg package for signing artifacts. Uses the `MILL_PGP_PASSPHRASE` environment variable if set. _Default: `[--passphrase=$MILL_PGP_PASSPHRASE], --no-tty, --pinentry-mode, loopback, --batch, --yes, --armor, --detach-sign`._ +
 
 `publishArtifacts`: The command for generating all publishable artifacts (ex. `__.publishArtifacts`). Required. +
 
@@ -282,7 +282,7 @@ $ mill -i \
 mill.javalib.SonatypeCentralPublishModule/publishAll \
 --username myusername \
 --password mypassword \
---gpgArgs --passphrase=$MILL_PGP_PASSPHRASE,--no-tty,--pinentry-mode,loopback,--batch,--yes,-a,-b \
+--gpgArgs --passphrase=$MILL_PGP_PASSPHRASE,--no-tty,--pinentry-mode,loopback,--batch,--yes,--armor,--detach-sign \
 --publishArtifacts __.publishArtifacts \
 --readTimeout  36000 \
 --awaitTimeout 36000 \