Set GPG_TTY when invoking gpg-connect-agent

We have seen user reports having issues with PIN entry not working
properly in headless environments or when the moon enters a certain
phase. While somewhat hard to reproduce or isolate, setting the GPG_TTY
environment variable more often than not seems to resolve them.
According to gpg-agent(1), this variable should be initialized with the
output of tty(1). That is strictly speaking a step that users need to
take, but given that it is more or less an implementation detail that we
may end up using GPG agent for PIN entry, they may be excused for not
having everything set up and should not be presented with a nondescript
error. Of course, such a variable should not be required to begin with,
but that ship sailed...
Be that as it may, the most sensible (read: user-friendly) step to take
from our side is to set the variable if it is not already present. To
that end, we essentially query the current TTY as tty(1) would and set
GPG_TTY before interacting with gpg-agent.

CHANGELOG.md

@@ -2,6 +2,8 @@ Unreleased
 ----------
 - Introduced `otp-cache` core extension
 - Included `git` tag/revision in `-V`/`--version` output
+- Automatically set `GPG_TTY` environment variable when running
+ `gpg-connect-agent` unless it is already set
 - Fixed endless loop when a Nitrokey FIDO2 device is present
 - Updated minimum supported Rust version to `1.47.0`
 - Bumped `anyhow` dependency to `1.0.57`

src/pinentry.rs

@@ -4,6 +4,7 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 
 use std::borrow;
+use std::env;
 use std::ffi;
 use std::fmt;
 use std::process;
@@ -215,13 +216,78 @@ where
  }
 }
 
+/// Retrieve a path to the TTY used for stdin, if any.
+///
+/// This function works on a best effort basis and skips any advanced
+/// error reporting, knowing that callers do not care.
+#[cfg(target_os = "linux")]
+pub(crate) fn retrieve_tty() -> Result<ffi::OsString, ()> {
+ use std::io;
+ use std::os::unix::ffi::OsStringExt as _;
+ use std::os::unix::io::AsRawFd as _;
+
+ let fd = io::stdin().as_raw_fd();
+ let fd_path = format!("/proc/self/fd/{}\0", fd);
+ let fd_path = ffi::CStr::from_bytes_with_nul(fd_path.as_bytes()).unwrap();
+
+ let mut buffer = Vec::<u8>::with_capacity(56);
+ // SAFETY: We provide valid pointers, `fd_path` is NUL terminated, and
+ // the provided capacity reflects the actual length of the
+ // buffer.
+ let rc = unsafe {
+ libc::readlink(
+ fd_path.as_ptr(),
+ buffer.as_mut_ptr() as *mut libc::c_char,
+ buffer.capacity(),
+ )
+ };
+ if rc <= 0 {
+ return Err(());
+ }
+
+ let rc = rc as usize;
+ // If `readlink` filled the entire buffer we could have experienced
+ // silent truncation. So we just bail out out of precaution.
+ if rc == buffer.capacity() {
+ return Err(());
+ }
+
+ // SAFETY: At this point we know that `readlink` has written `rc`
+ // bytes to `buffer`.
+ unsafe { buffer.set_len(rc) };
+
+ Ok(ffi::OsString::from_vec(buffer))
+}
+
+#[cfg(not(target_os = "linux"))]
+pub(crate) fn retrieve_tty() -> Result<OsString, ()> {
+ Err(())
+}
+
+/// Ensure that the `GPG_TTY` environment variable is present in the
+/// environment, setting it as appropriate if that is not currently the
+/// case.
+fn ensure_gpg_tty(command: &mut process::Command) -> &mut process::Command {
+ const GPG_TTY: &str = "GPG_TTY";
+
+ if let Some(tty) = env::var_os(GPG_TTY) {
+ // We don't strictly speaking need to set the variable here, because
+ // it would be inherited anyway. But we want to make that explicit.
+ command.env(GPG_TTY, tty)
+ } else if let Ok(tty) = retrieve_tty() {
+ command.env(GPG_TTY, tty)
+ } else {
+ command
+ }
+}
+
 /// Connect to `gpg-agent`, run the provided command, and return the
 /// output it emitted.
 fn gpg_agent<C>(command: C) -> anyhow::Result<process::Output>
 where
  C: AsRef<ffi::OsStr>,
 {
- process::Command::new("gpg-connect-agent")
+ ensure_gpg_tty(&mut process::Command::new("gpg-connect-agent"))
  .arg(command)
  .arg("/bye")
  .output()
@@ -341,6 +407,8 @@ where
 mod tests {
  use super::*;
 
+ use std::fs;
+
  #[test]
  fn parse_pinentry_pin_empty() {
  let response = "OK\n";
@@ -392,4 +460,28 @@ mod tests {
  let error = parse_pinentry_response(response).unwrap_err();
  assert_eq!(error.to_string(), expected)
  }
+
+ /// Check that we can retrieve the path to the TTY used for stdin.
+ #[cfg(target_os = "linux")]
+ #[test]
+ fn tty_retrieval() {
+ use std::os::unix::io::AsRawFd as _;
+
+ // We may be run with stdin not referring to a TTY in CI.
+ if unsafe { libc::isatty(std::io::stdin().as_raw_fd()) } == 0 {
+ return;
+ }
+
+ let tty = retrieve_tty().unwrap();
+ // To check sanity of the reported path at least somewhat, we just
+ // try opening the file, which should be possible. Note that we open
+ // in write mode, because for one reason or another we would not
+ // actually fail opening a *directory* in read-only mode.
+ let _file = fs::OpenOptions::new()
+ .create(false)
+ .write(true)
+ .read(true)
+ .open(tty)
+ .unwrap();
+ }
 }

src/tests/run.rs

@@ -400,5 +400,13 @@ fn extension_arguments(model: nitrokey::Model) -> anyhow::Result<()> {
 
  // NITROCLI_USB_PATH should not be set, so the program errors out.
  let _ = test(model, "NITROCLI_USB_PATH", &[], |out| out == "\n").unwrap_err();
+
+ let tty = crate::pinentry::retrieve_tty().unwrap();
+ test(model, "GPG_TTY", &[], |out| {
+ // It's conceivable that this check fails if the user has set
+ // GPG_TTY to a different TTY than the current one. We declare that
+ // as not supported for testing purposes.
+ out.trim() == tty
+ })?;
  Ok(())
 }