diff --git a/AGENTS.md b/AGENTS.md index 800e46b..3e022d0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -240,6 +240,26 @@ mutable at runtime from the phone. somebody asked. Stop and Uninstall do go through the script, and do strand the phone; the app confirms them rather than hiding them. +- **A stripped copy is only serveable if it is signed by the key the + build was.** `strip.rs` re-signs with `~/.android/debug.keystore`, and + nothing about a debug keystore says which builds it made -- it is per + machine, and one recreated after an APK was built signs as an entirely + unrelated certificate (measured: a fresh AGP-parameter keystore against + the existing one, no relation, as a new RSA keypair should be). Where + they differ Android refuses the package and the phone says "App not + installed" with no cause, which reads as the download rather than the + signing and is among the most expensive sentences here to be handed. So + the two are compared -- `apksigner verify --print-certs` on the source + and on what was just signed, as sets of digests so a v2 source and a v3 + output still match -- and a mismatch is refused with both certificates + and the keystore path named. Measured on the files rather than inferred + from the keystore, so it stays true if the signing step changes. The + slim copy is deleted on refusal: with no stamp beside it nothing would + serve it, but `serveable_now` reports the size of whatever slim file is + on disk, so leaving it would have the card describing a download + nothing can install. Empty digests mean "not compared" rather than "no + signer", and say so in the log rather than refusing -- an APK apksigner + cannot read is one this comparison has no opinion about. - **A 500 answers with its message.** `ApiError::Internal` used to be a bare status with an empty body, on the grounds that an internal cause is not safe to hand back -- but every route here is behind the bearer @@ -839,8 +859,14 @@ session reads. What follows is what that means here. `--avd NAME` or `AVD_NAME=`, and when it cannot find that one it lists what *is* attached instead of guessing which of the three situations it is in. -- The server needs `aapt2` (SDK build-tools) to add an app, and - `llvm-strip` (NDK) only for apps that need stripping. **It has to find +- The server needs `aapt2` (SDK build-tools) to add an app, and, for an + app that needs stripping, `llvm-strip` (NDK), `zipalign` and + `apksigner` (build-tools) **and `~/.android/debug.keystore`** -- the + last is the least guessable of the four, since it belongs to no SDK, + is created as a side effect of any Gradle Android build, and is + consumed by the pipeline's final step. A machine that builds APKs and + has no keystore is odd rather than new: something removed it after the + build, and the APK it produced is signed by a key that is now gone. **It has to find both without an environment**, because the way this server usually starts is from a service manager, and one hands its process a scrubbed environment: measured in the Gentoo guest, an OpenRC user service gets diff --git a/server/src/strip.rs b/server/src/strip.rs index aa173ba..35d5e28 100644 --- a/server/src/strip.rs +++ b/server/src/strip.rs @@ -1,13 +1,16 @@ //! Produces a sibling `.slim.apk` for an APK with native `.so` //! debug symbols stripped out of every `lib/**/*.so` (everything else //! copied byte-for-byte, via `ZipWriter::raw_copy_file` so untouched -//! entries are never decompressed/recompressed), re-signed with the same -//! debug key so it still installs cleanly. +//! entries are never decompressed/recompressed), re-signed with this +//! machine's debug key so it still installs cleanly -- which holds only +//! while that is the key the build was signed with, and is therefore +//! checked rather than assumed. //! //! Cached against the source APK's (mtime, size) in a `.slim-stamp` //! sidecar, so it's only regenerated when the underlying build actually //! changes. +use std::collections::BTreeSet; use std::io::{Read, Write}; use std::path::{Path, PathBuf}; use std::process::Command; @@ -118,6 +121,41 @@ fn strip_debug_symbols(src_path: &Path) -> Result { .arg(&aligned_path), )?; + // Re-signing keeps the copy installable only if this machine's debug + // key *is* the key the build was signed with, and nothing about a + // debug keystore says which builds it made -- it is per machine, and + // one recreated after the APK was built signs as an entirely + // unrelated certificate. Where they differ the phone's answer is + // "App not installed" with no cause shown, because Android refuses a + // package whose signature does not match the one it already has; the + // failure is silent, arrives on the device, and looks like the + // download rather than the signing. + // + // Measured on both files rather than inferred from the keystore, so + // this says what was actually produced -- and it stays true if the + // signing step ever changes which key or scheme it uses. + let built_by = signer_certificates(src_path, build_tools); + let signed_by = signer_certificates(&slim_path, build_tools); + if !built_by.is_empty() && !signed_by.is_empty() && built_by != signed_by { + // Removed rather than left for a later request to find: with no + // stamp beside it nothing would serve it, but `serveable_now` + // reports the size of whatever slim copy is on disk, so leaving + // it would have the card describing a download nothing can + // install. + let _ = std::fs::remove_file(&slim_path); + let _ = std::fs::remove_file(with_suffix(&slim_path, ".idsig")); + anyhow::bail!( + "{} is signed by {}, but the debug keystore at {} signs as {} -- a stripped copy \ + signed with it is a different app to Android, and the phone would refuse it with \ + \"App not installed\". Rebuild this app on this machine, or restore the debug \ + keystore it was built with.", + src_path.display(), + built_by.iter().cloned().collect::>().join(", "), + sdk::debug_keystore_path()?.display(), + signed_by.iter().cloned().collect::>().join(", "), + ); + } + std::fs::write(&stamp_path, &stamp)?; tracing::info!( "stripped -> {} ({} bytes)", @@ -127,6 +165,43 @@ fn strip_debug_symbols(src_path: &Path) -> Result { Ok(slim_path) } +/// Which certificates `apk` is signed with, as the SHA-256 digests +/// apksigner prints -- one line per signature scheme, so an APK signed +/// once under v1 and v2 answers with a set of one. +/// +/// Empty when apksigner will not verify the file or says nothing about +/// its certificates. Deliberately not an error: an APK this cannot read +/// is one this comparison has no opinion about, and refusing over it +/// would break a case that works today. The caller treats empty as "not +/// compared" rather than as "no signer", and the log line is what says +/// which of the two happened. +fn signer_certificates(apk: &Path, build_tools: &Path) -> BTreeSet { + let output = Command::new(build_tools.join("apksigner")) + .arg("verify") + .arg("--print-certs") + .arg(apk) + .output(); + let output = match output { + Ok(output) if output.status.success() => output, + other => { + tracing::warn!( + "could not read the signer of {}, so its signature was not compared: {}", + apk.display(), + match other { + Ok(output) => String::from_utf8_lossy(&output.stderr).trim().to_string(), + Err(err) => err.to_string(), + }, + ); + return BTreeSet::new(); + } + }; + String::from_utf8_lossy(&output.stdout) + .lines() + .filter_map(|line| line.split_once("certificate SHA-256 digest:")) + .map(|(_, digest)| digest.trim().to_string()) + .collect() +} + /// Rewrites `src_path` into `dst_path`, running every `lib/**/*.so` entry /// through `llvm-strip --strip-debug` and copying everything else as-is. fn strip_native_libs(