online: review nits for the proxy fix and keyserver URL verdict

Correct the rationale: the env proxy was resolved to the key service's pinned address, so every lookup failed.
Gate set_last_decode_reachability behind a test-hooks feature, mark the verdict types non_exhaustive, test a .invalid host, add CHANGELOG.
This commit is contained in:
Matthew Jackson
2026-09-27 12:40:09 -07:00
parent 10b0348db8
commit 1606ff3ca3
4 changed files with 34 additions and 7 deletions
+11
View File
@@ -1,5 +1,16 @@
# Changelog
## [Unreleased]
### Fixed
- The online key service ignores `HTTP_PROXY`/`HTTPS_PROXY`/`ALL_PROXY`: with one set, every key lookup failed to connect (the pinned resolver also answered the proxy's address).
### Added
- `check_keyserver_url` with `KeyserverUrlRejection`/`KeyserverUrlFault`: a typed Permanent vs Temporary verdict, so callers need not match `validate_keyserver_url`'s text.
- `test-hooks` feature exposing `set_last_decode_reachability`, to test decode-verdict handling without a network.
## [1.7.7] — 2026-09-26
### Maintenance
+4
View File
@@ -30,6 +30,10 @@ zip = { version = "8", default-features = false, features = ["deflate"] }
# key material).
tracing = "0.1"
[features]
# `set_last_decode_reachability`, for consumers' tests only (enable in dev-dependencies).
test-hooks = []
[profile.release]
lto = "thin"
codegen-units = 1
+3 -2
View File
@@ -16,10 +16,11 @@ mod paths;
pub use keydb::{KeydbSource, UpdateResult};
pub use keydb_format::{DiscEntry, KeyDb};
#[cfg(feature = "test-hooks")]
pub use online::set_last_decode_reachability;
pub use online::{
DecodeReachability, KeyserverUrlFault, KeyserverUrlRejection, MIN_SAMPLE_UNITS, OnlineSource,
check_keyserver_url, set_last_decode_reachability, take_last_decode_reachability,
validate_keyserver_url,
check_keyserver_url, take_last_decode_reachability, validate_keyserver_url,
};
pub use paths::{default_keydb_path, existing_keydb_path, keydb_search_paths};
+16 -5
View File
@@ -248,6 +248,7 @@ pub fn validate_keyserver_url(url: &str) -> Result<(), String> {
/// Whether a rejected key-service URL can start working without a config change.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[non_exhaustive]
pub enum KeyserverUrlFault {
/// Bad scheme, host or port, or a non-public address: retrying changes nothing.
Permanent,
@@ -257,6 +258,7 @@ pub enum KeyserverUrlFault {
/// Why [`check_keyserver_url`] rejected a URL; `message` is [`validate_keyserver_url`]'s text.
#[derive(Debug, Clone, PartialEq, Eq)]
#[non_exhaustive]
pub struct KeyserverUrlRejection {
pub fault: KeyserverUrlFault,
pub message: String,
@@ -333,7 +335,8 @@ fn hardened_agent(pinned: Vec<SocketAddr>) -> ureq::Agent {
// a body deadline a stalled reply hangs the POST forever. The JSON key
// answer arrives in one read, so a TOTAL cap is the right shape here.
.timeout_recv_body(Some(Duration::from_secs(TIMEOUT_SECS)))
// Never the env proxy: a proxy re-resolves the host, bypassing the guarded pin.
// Never the env proxy: PinnedResolver would also answer the proxy's lookup with the key
// service's address, so with HTTP(S)_PROXY/ALL_PROXY set every key lookup failed to connect.
.proxy(None)
.build();
// `with_parts`, never `new_with_config` — see [`PinnedResolver`].
@@ -543,8 +546,9 @@ pub fn take_last_decode_reachability() -> Option<DecodeReachability> {
LAST_DECODE_REACHABILITY.with(std::cell::Cell::take)
}
/// Set this thread's decode-reachability slot, as a real decode POST would. A test hook: lets a
/// caller exercise its [`take_last_decode_reachability`] handling without a network or DNS.
/// Set this thread's decode-reachability slot, as a real decode POST would. Test hook (feature
/// `test-hooks`): lets a caller test its [`take_last_decode_reachability`] handling offline.
#[cfg(any(test, feature = "test-hooks"))]
pub fn set_last_decode_reachability(outcome: Option<DecodeReachability>) {
LAST_DECODE_REACHABILITY.with(|c| c.set(outcome));
}
@@ -1048,8 +1052,8 @@ mod tests {
);
}
// ureq 3 defaults to `Proxy::try_from_env()`; a proxy re-resolves the host itself, so the
// pinned, SSRF-guarded addresses would be bypassed. Checked in a child so no test mutates env.
// ureq 3 defaults to `Proxy::try_from_env()`; PinnedResolver would send the proxy connection to
// the key service's address, failing every lookup. Checked in a child so no test mutates env.
#[test]
fn the_key_service_agent_never_uses_an_environment_proxy() {
const CHILD: &str = "FMKV_KS_PROXY_CHILD";
@@ -1112,6 +1116,13 @@ mod tests {
assert_eq!(c.fault, KeyserverUrlFault::Permanent);
}
// RFC 6761: `.invalid` never resolves, so the lookup fails (or times out) either way.
#[test]
fn an_unresolvable_host_is_a_temporary_rejection() {
let r = check_keyserver_url("https://nonexistent.invalid/").unwrap_err();
assert_eq!(r.fault, KeyserverUrlFault::Temporary, "{r}");
}
// Callers test their verdict mapping with the hook instead of a real DNS lookup.
#[test]
fn the_decode_reachability_hook_plants_what_take_reads() {