Add HTTP/HTTPS proxy support for Java SDK (KSM-531)

[stas-schaller]

Summary

Java SDK: adds HTTP/HTTPS proxy support for outbound requests to the Keeper vault API, for environments where direct internet access isn't available.

Changes

New Features

• New proxyUrl option on SecretsManagerOptions, covering secret queries and file uploads (KSM-531)
• Resolution precedence: explicit proxyUrl → JVM system properties (https.proxyHost/https.proxyPort) → HTTPS_PROXY/HTTP_PROXY environment variables; NO_PROXY/http.nonProxyHosts exclusions honored (KSM-531)
• Authenticated proxies supported via the http://user:password@host:port URL form (KSM-531)
• A 407 from an authenticated proxy now throws a SecretsManagerException naming the exact JVM-flag remediation, instead of surfacing a bare status code or an opaque IOException (KSM-531)
• downloadFile(options, file) and downloadThumbnail(options, file) overloads added for proxy-aware file downloads; the single-argument forms use ambient proxy settings but do not pick up allowUnverifiedCertificate from options (KSM-531)

Testing

cd sdk/java/core && ./gradlew test

Also live-verified proxy behavior (both an open proxy and a Basic-auth-gated one) against real local forward proxies (mitmdump), in addition to the mocked ProxyTest unit suite.

Security Impact

Adds an authenticated-proxy credential path (java.net.Authenticator) and threads proxyUrl through all outbound HTTP(S) connections. Credentials are held in memory only, never logged or persisted, and the vault API's own TLS/transmission-key encryption is unaffected by this change since the proxy only touches the network hop, not payload confidentiality. Authenticated-proxy support additionally requires the host JVM to clear jdk.http.auth.tunneling.disabledSchemes (a CVE-2016-5597 mitigation) before making any other HTTPS connection; this constraint is documented prominently in the README and enforced as a clear, actionable runtime exception rather than a silent failure.

Breaking Changes

• Kotlin callers compiled against 17.3.0 must recompile against 17.4.0. Adding proxyUrl to the SecretsManagerOptions primary constructor changes the constructor and copy() signatures that Kotlin default-argument calls bind to, so any Kotlin call site using default or named arguments throws NoSuchMethodError against a 17.4.0 jar until recompiled. Java callers are unaffected: all previously published constructor arities are retained via @JvmOverloads.

Related Issues

• Jira: KSM-531

[mgallego-keeper]

Thanks Stas, this lands a genuinely useful feature and parts of it show real care: @JvmOverloads on SecretsManagerOptions, the retained 4-arg postFunction overload, the honest README section on the CVE-2016-5597 disabledSchemes constraint, and the deterministic ProxyEnvironment seam for tests.

I went deep on the resolution/auth layer and found enough that I'd like a rework pass before merge, so requesting changes. I verified these locally rather than eyeballing: kotlinc/javap plus a reproduced NoSuchMethodError for the binary-compat item, jshell for the URI parsing claims, and a live local proxy for the exclusion/fallback behavior. 15 inline comments, grouped by theme:

1. Process-global side effects and credential hygiene. ProxyAuthenticator.register() re-asserts the JVM-wide default Authenticator on every proxied connection with no restore path, and can be triggered purely by ambient HTTPS_PROXY credentials with no SDK-level opt-in; proxyUrl (which may carry user:password) participates in the generated toString() of both SecretsManagerOptions and KeeperFile, so logging options or records prints the proxy password.

2. Resolution semantics. Ambient NO_PROXY silently overrides an explicit programmatic proxyUrl (our .NET and Python SDKs treat the explicit option as authoritative); unparseable input fails open to a direct connection; the exclusion matcher misses JDK-style patterns while a matched exclusion still isn't actually direct (a regression vs the base branch for http.nonProxyHosts users); http.proxyHost is applied to HTTPS traffic; https:// proxies default to port 80; an empty HTTPS_PROXY masks HTTP_PROXY; partial userinfo silently drops credentials; an out-of-range port throws a raw IllegalArgumentException.

3. 407 handling. The remediation exception is unreachable on the POST paths (the tunnel failure throws from connection.outputStream before checkedResponseCode runs), and the "tunnel"/"407" string match misclassifies proxy 502/503/504 as auth failures while dropping the original exception (SecretsManagerException has no cause constructor).

4. Compatibility and integration. The KeeperFile constructor change breaks Java source and binary compatibility; postQuery drops the proxy for custom queryFunctions, where cachingPostFunction then masks the failure by silently serving stale cache.

5. Efficiency and tests. Two blocking DNS resolutions per proxied request, and ProxyTest's .local hostnames trigger live mDNS lookups (about 20 seconds of environment-dependent wall clock on macOS); no test covers malformed proxy input.

Structurally, four changes resolve most of the list: make explicit proxyUrl authoritative and fail closed on unparseable explicit config; don't fight the host app for the default Authenticator (chain/restore where the platform allows, and never install it from ambient env credentials without opt-in); keep proxyUrl out of the data classes' public constructors and redact it in any string form; and run the 407 classification where the POST paths actually throw, preserving the cause.

Two lower-confidence observations I didn't file inline: the 407 remediation text may misdirect users of NTLM/Negotiate proxies, and two clients sharing one proxy host with different credentials will clobber each other in the singleton credentials map. Worth keeping in mind if the authenticator design gets reworked.

Happy to discuss any of these, and I'll re-review quickly once updated.

[mgallego-keeper]

Round 2, reviewed at 65c6c4d. Good progress, and the suite passes at head (71 tests, with ProxyTest down from ~20s of live mDNS to 0.003s). Fixed outright: the toString() leak, the KeeperFile compatibility break (javap-verified), explicit proxyUrl beating NO_PROXY, and fail-closed parsing for explicit config. Materially fixed: the eager-DNS/double-resolve cost. Partially fixed: the Authenticator takeover (ambient no longer triggers it; chaining/unregister still open), the 407 remediation (works for postFunction, verified against a live 407 proxy; uploads still bare), and the tunnel-failure classification (narrowed to 407; original exception still dropped). I replied on every round-1 thread with its status and resolved the completed ones.

Still requesting changes, on a much narrower set:

1. The new fail-closed exception interpolates the full proxyUrl, password included, into its message; passwords with reserved characters are exactly the inputs that fail parsing (ProxySupport.kt line 46).
2. Downloads no longer inherit options.proxyUrl at all, while the README still says the option covers all SDK network calls (SecretsManager.kt line 1181, README line 32).
3. uploadFile() still lacks the 407 conversion the other paths now have (SecretsManager.kt line 1236).
4. An out-of-range proxy port still escapes as a raw IllegalArgumentException, which contradicts the new fail-closed behavior for explicit config (existing thread on ProxySupport.kt).
5. New: ambient env credentials are now parsed but never registered, so an authenticated HTTPS_PROXY is guaranteed to 407 with a message that misdirects to proxyUrl and the JVM flag (ProxySupport.kt line 113). If ignoring ambient credentials is the intended design (defensible), a README bullet plus a message branch covers it.

Everything else I'm happy to defer to an explicit follow-up (ticket or a checklist in the PR description): the exclusion matcher rewrite and the Proxy.NO_PROXY fallback, the http.proxyHost tier, QueryFunction proxy carriage (with at least a cache-fallback log line), scheme-based default ports, empty HTTPS_PROXY masking, partial-userinfo handling, Authenticator chaining/unregister, and a cause constructor on SecretsManagerException. Nothing in that list is marked deferred anywhere yet, so let's either fix or record them, and I'll treat the recorded ones as out of scope for this PR.

[mgallego-keeper]

The new fail-closed exception leaks the credential it's failing on. The message interpolates the full proxyUrl, and passwords containing URI-reserved characters (@, spaces) are precisely the inputs that make URI parsing fail: http://user:p@ssword@proxy:8080 parses with host == null, lands here, and the password goes into the exception message, which is exactly what ends up in logs/APM. The unparsableExplicitProxyUrlFailsClosed test only uses a credential-free URL, so it can't catch this. Suggest redacting userinfo before interpolating, or not echoing the value at all (name the host portion only).

[mgallego-keeper]

Still open at 818981e, and I'd rather flag it clearly than let it look done: redactProxyUrl() cannot redact anything that reaches it. It only rewrites when URI parses the value into a server-based authority (getUserInfo() != null), but this call site is reached precisely because parseProxy failed, which happens either when URI throws or when the authority isn't server-based, and in the latter case getUserInfo() is null. Both fall through to ?: proxyUrl and return the input verbatim.

I compiled the current redactProxyUrl and parseProxy verbatim out of this file and ran the explicit branch:

input=http://user:p@ssword@proxy:8080
  redacted -> http://user:p@ssword@proxy:8080   (unchanged)
  thrown: proxyUrl 'http://user:p@ssword@proxy:8080' could not be parsed as a valid proxy URL
input=http://user:pass word@proxy:8080
  redacted -> http://user:pass word@proxy:8080   (unchanged)
  thrown: proxyUrl 'http://user:pass word@proxy:8080' could not be parsed as a valid proxy URL

The first parses with a registry authority (host and userInfo both null); the second throws URISyntaxException and hits the getOrNull() ?: proxyUrl fallback. http://user:secret@proxy.corp:8080 does redact correctly, but that URL parses fine and so never reaches line 46. Reserved characters in the password are the whole failure population here, so the helper is a no-op where it matters.

Redaction has to work without a successful parse. Either operate on the string (take everything up to the first ://, drop through the last @ before the host, e.g. proxyUrl.replace(Regex("(?<=://)[^@/]*@"), "***@")), or don't echo the value at all and name only what you can safely extract. A test with a credentialed URL is what would have caught this, see the comment on ProxyTest.

[mgallego-keeper]

Half fixed at 23999c6, and the half that is fixed is the half the new test covers. Textual redaction is the right approach and it works when the value carries a scheme:

input    : http://user:p@ssword@proxy.corp:8080
redacted : http://user:***@proxy.corp:8080
THROWN   : proxyUrl 'http://user:***@proxy.corp:8080' could not be parsed as a valid proxy URL

Reopening because a scheme-less proxyUrl still leaks verbatim. redactProxyUrl returns early at ProxySupport.kt:262 when there is no ://, but parseProxy deliberately accepts scheme-less input by prepending http:// (line 72), and that form resolves successfully, so it is a supported way to pass the option. Run against the jar built from this commit:

input    : proxy.corp:8080                    -> resolves fine, so scheme-less is a supported form
input    : user:p@ssword@proxy.corp:8080
  redacted : user:p@ssword@proxy.corp:8080    (unchanged)
  THROWN   : proxyUrl 'user:p@ssword@proxy.corp:8080' could not be parsed as a valid proxy URL
input    : user:pass word@proxy.corp:8080
  redacted : user:pass word@proxy.corp:8080   (unchanged)
  THROWN   : proxyUrl 'user:pass word@proxy.corp:8080' could not be parsed as a valid proxy URL

Reserved-character passwords are still the whole failure population, so this is the same shape as before, just narrowed to inputs without a scheme. Redacting the normalized value instead of the raw one fixes both cases in one line, since parseProxy already computes normalized.

Two notes while you are in here:

1. redactProxyUrlStripsCredentialsTextually asserts redactProxyUrl("notaurl") == "notaurl", which locks in the passthrough. It needs a credentialed scheme-less case alongside it, which is what would have caught this.
2. The helper mangles an @ in the path: http://proxy.corp:8080/tenant@acme comes out as http://proxy.corp:***@acme, inventing a credential and hiding the host in a message meant to help someone debug. Bounding the @ search to the authority (stop at the first /) handles it.

The PR description's Security Impact section says credentials are "never logged", which is still not quite true for this path.

[stas-schaller]

Covered in v2: the fail-closed exception at resolveProxy line 46 interpolates redactProxyUrl(explicitProxyUrl), not the raw value. redactProxyUrl uses lastIndexOf('@') (textual, not URI-parsing) to locate the userinfo/host boundary, so a password containing @ or spaces — like http://user:p@ssword@proxy:8080 — is correctly redacted to http://user:***@proxy:8080. The unparsableExplicitProxyUrlRedactsCredentials test in ProxyTest uses exactly that input and verifies the password is absent from the exception message.

[mgallego-keeper]

Reopening: this is fixed for the with-scheme form only, and redactProxyUrl has not changed since round 4.

A scheme-less proxyUrl still lands here with the password intact:

proxyUrl = "user:p@ssword@proxy.corp:8080"
  -> proxyUrl 'user:p@ssword@proxy.corp:8080' could not be parsed as a valid proxy URL

The reason this is a real configuration rather than a nonsense input: parseProxy supports scheme-less values by prepending http://, and I confirmed against the built jar that svc:S3cret!@proxy.corp:8080 resolves fine with isExplicit=true. So a scheme-less proxy is a supported path, and a password containing @ or a space is exactly the input that fails parsing and reaches this message.

redactProxyUrl returns early when there is no ://. The verified fix and the missing test (ProxyTest.kt:111 currently has the credential-free case only) are on the round-5 inline comments.

(Apologies for the garbled reply just above this one, that was a shell quoting accident on my side.)

[mgallego-keeper]

Round 3, reviewed at 818981e. Good round: four of the five round-2 blockers are genuinely fixed and I verified each rather than reading them. uploadFile() now has the same 407 wrap postFunction got, so all three network paths convert a tunnel 407 into the remediation exception; an out-of-range port in an explicit proxyUrl fails closed; partial userinfo in an explicit proxyUrl fails loudly; https:// proxies default to 443; the four *_PROXY env reads are blank-guarded; a matched exclusion now forces Proxy.NO_PROXY; and downloadFile(options, file) / downloadThumbnail(options, file) restore proxy inheritance for downloads (javap confirms both, and KeeperFile's 5-arg constructor and copy() are still intact). Suite at head: 71 tests, 0 failures; ProxyTest 9 tests in 0.003s.

Two items keep this at changes-requested, and one of them is new.

1. The redaction fix doesn't redact. redactProxyUrl() can only rewrite a URL that URI parses into a server-based authority, but its only call site is the branch taken because parsing failed. I compiled the two functions verbatim out of the current file and ran them: http://user:p@ssword@proxy:8080 (an @ in the password makes URI fall back to a registry authority, so getUserInfo() is null) and http://user:pass word@proxy:8080 (URI throws, runCatching{}.getOrNull() ?: proxyUrl returns the raw string) both come back unchanged, and the thrown message is proxyUrl 'http://user:p@ssword@proxy:8080' could not be parsed as a valid proxy URL. Those reserved-character passwords are exactly the inputs that make parsing fail, so the helper is a no-op on essentially every input that reaches it. Redacting has to happen without a successful parse: strip from the first :// to the last @ textually, or just name the host portion and drop the value. The PR description's Security Impact section says credentials are "never logged", which this contradicts.

2. New regression: an unparseable ambient proxy is now read as an exclusion, and forcing direct defeats the JDK's own resolution. openProxiedConnection infers exclusion from resolved == null plus "some proxy property or env var is set", but resolveProxy also returns null when an ambient candidate fails parseProxy, and parseProxy rejects values the JDK itself accepts: -Dhttps.proxyHost=my_proxy (underscore, so URI.getHost() is null) and an IPv6 https.proxyHost (joined unbracketed as ::1:3128). At 65c6c4d those fell through to the no-arg url.openConnection(), which consults the default ProxySelector and proxied correctly; at 818981e they get Proxy.NO_PROXY and go direct. Live repro with a listener on [::1] and a .invalid target, so nothing leaves the machine: the old path sends CONNECT nonexistent-ksm-probe.invalid:443 to the proxy, the new path throws UnknownHostException with nothing reaching it. On an egress-locked network that's a hard break; on a permissive one it's a silent bypass of a mandated proxy. The private isExcluded(targetHost, environment) is in the same file, so asking it directly instead of inferring fixes this and drops the duplicated lookups; an empty -Dhttps.proxyHost= reaches the same path via ":443".

Smaller, all cheap:

3. The new ambient 407 branch over-fires. In both POST catches isAmbientCredentials is resolved != null && !resolved.isExplicit with no check that credentials were actually present, so an unauthenticated ambient proxy answering 407 tells the operator that credentials were detected in the environment when none were. checkedResponseCode gets this right through requiresProxyAuth; the POST paths need the same username != null gate. Separately, that text says to "pass the proxyUrl explicitly to downloadFile/uploadFile/getSecrets" and only downloadFile/downloadThumbnail have such a parameter, so the accurate advice is the SecretsManagerOptions sentence that follows it. Also worth gating the whole rewrite on a proxy having been resolved, so an unrelated IOException whose message happens to contain "407" on a direct connection doesn't come back as proxy-auth remediation.
4. SecretsManagerException gained the (message, cause) constructor but nothing calls it, so the original exception and stack are still dropped, which was the point of the request. Four sites have a real cause in hand: ProxySupport.kt 88 (the caught IllegalArgumentException, currently discarded as an unused lambda parameter) and 185, plus SecretsManager.kt 1254 and 1747.
5. downloadFile(options, file) forwards options.proxyUrl but hardcodes allowUnverifiedCertificate = false, so the new options-taking overloads ignore the flag from the same options object getSecrets(options) honors.
6. Nothing in this commit has a test. ProxyTest is unchanged from round 2, same 9 cases, so redaction, both new explicit-config guards, the 443 default, the blank-env fix, forced Proxy.NO_PROXY, the ambient 407 branch, and the new download overloads are all uncovered. A redaction test with a credentialed URL, which is what I asked for last round, would have caught item 1 on the first run.
7. Docs are untouched, so the README still doesn't mention the new download overloads while line 32 promises the option covers all network calls, and the env-var bullet still doesn't say ambient credentials are ignored. Those were the other halves of round-2 items 2 and 5.

Last thing, procedural rather than technical: the round-1 deferrals still aren't recorded anywhere. The matcher rewrite, the http.proxyHost tier, QueryFunction carriage plus the cachingPostFunction cache-fallback log line, Authenticator chaining/unregister/credential clearing, and the blank guard for systemPropertyProxy are all still open, and the PR description still lists no deferrals and says "Breaking Changes: None". I'm happy to treat every one of them as out of scope, I just need them written down as tickets or a checklist in the description so they don't evaporate. Replies are on all 14 open threads and I resolved the five that are done.

[mgallego-keeper]

Addendum to the round-3 review above, kept separate so the blocking set stays clearly scoped. I did a second pass over 818981e concentrating on the published artifact rather than the source, and found five more things, each verified against the built jar or by executing the shipped shapes rather than by reading.

Two of them I'd fold into the blocking set: the jar now exports a ready-made all-trusting SSLSocketFactory plus the entire proxy internals as public API, and notation-based file downloads still bypass the proxy despite being one word from the fix. The other three (the serialVersionUID change, https:///socks5:// proxy URLs being driven as cleartext HTTP, and proxyUrl being unreachable from Java except through the full 8-argument constructor) are cheap to fix but I'm happy to see them recorded instead if you'd rather close this round out.

None of these is new in 818981e specifically, so they're not "regressions from the last commit"; four are in code this PR introduced and the fifth is a visibility change it made.

[mgallego-keeper]

Round 4, reviewed at 23999c6. Strong round: both round-3 blockers are genuinely fixed, along with most of the addendum. I built at head with the JDK 8 toolchain and verified each item against the built jar or a live local proxy rather than reading the diff. Suite at head: 84 tests, 0 failures, with ProxyTest up from 9 cases to 22 and still running in 0.004s.

Fixed and verified:

• The Proxy.NO_PROXY bypass regression is gone. openProxiedConnection asks isExcluded() directly instead of inferring exclusion from resolved == null. I re-ran the exact round-3 repro and the proxy receives CONNECT nonexistent-ksm-probe.invalid:443 again, where at 818981e it received nothing and the call died with UnknownHostException.
• The headline feature works end to end. Against a local 407-answering proxy: disabledSchemes is cleared before openConnection, the proxy actually receives Proxy-authorization: Basic ... on the retry, and the failure converts to a SecretsManagerException naming the JVM flag, on the correct explicit branch, with no password in the message.
• Redaction now redacts the with-scheme case, including in the thrown message, with the test I asked for.
• Notation file lookups are proxied (getNotationResults uses downloadFile(options, ...)), and both download overloads honor options.allowUnverifiedCertificate.
• trustAllSocketFactory is out of the public surface; javap finds no SSLSocketFactory entry point on SecretsManager.
• Explicit https:// proxies are rejected with an accurate message; the cause is now attached at all three sites; the resolved != null guard stops unrelated IOExceptions from being relabelled as auth failures; the system-property tier got its blank guard.
• SecretsManagerOptions(storage, proxyUrl) reaches the option from Java, and the README covers the download overloads, the ambient-credentials design, and the cachingPostFunction gap. KSM-1268 is filed and accurately scoped.

Two things keep this at changes-requested, both one-liners.

1. The serialVersionUID pin is the wrong value, and that one is my fault. The number in my round-3 comment came from a reconstruction rather than from the artifact. Released 16.6.6, released 17.2.0, and the base branch as compiled all agree on 5401507264959279624; the PR pins 4308841855089494389. A real round-trip from the released class to this one throws InvalidClassException, so the field currently guarantees the failure it was added to prevent, and nothing at build time can catch it because declaring an explicit SUID stops the JDK computing one. Your mechanism is correct, only the constant needs changing (SecretsManagerExceptions.kt:10).

2. Redaction still leaks a scheme-less proxyUrl. redactProxyUrl returns early when there is no :// (ProxySupport.kt:262), but parseProxy supports scheme-less input by prepending http:// (line 72) and that form resolves fine, so proxyUrl = "user:p@ssword@proxy.corp:8080" still throws with the password in cleartext. Redacting the normalized value fixes it, and the current test asserting redactProxyUrl("notaurl") == "notaurl" needs a credentialed scheme-less case beside it.

Everything else I am happy to see fixed cheaply or recorded, none of it blocking:

• Both POST catches still hand an explicit-but-unauthenticated proxy the CVE-2016-5597 remediation on a 407, where the real cause is that no credentials were configured. checkedResponseCode already gets this right, so the three paths disagree.
• uploadFile(options, ...) is the last place that drops options.allowUnverifiedCertificate (SecretsManager.kt:1236, a literal false).
• new SecretsManagerOptions(storage, null) no longer compiles, since the new convenience constructor makes it ambiguous. Verified against both jars. No in-repo caller uses that shape, so exposure looks small, but it is either worth a named factory or a line under Breaking Changes instead of "None".
• ResolvedProxy.toString() prints the password, and the proxy internals remain public in the shipped bytecode (isExcluded was widened this round, so that list grew by one even as trustAllSocketFactory closed).
• Non-http schemes other than https are still driven as HTTP proxies (socks5:// most plausibly).
• Small cleanups: dead javax.net.ssl.* import, unreachable 443 branch, redactProxyUrl mangling an @ in the path, and one test that does not assert the gate its name promises.

On bookkeeping: KSM-1268 plus the README line for cachingPostFunction cover two of the deferral items. The exclusion matcher (I re-probed it: trailing wildcards like 127.* and 192.168.*, the JDK's own documented http.nonProxyHosts form, still do not match, nor do CIDR or host:port, though it no longer over-matches) and the http.proxyHost tier for HTTPS traffic still have no home, and KSM-1268's scope does not include the process-wide jdk.http.auth.tunneling.disabledSchemes mutation. Tickets or a checklist in the description both work; I would just rather the list not live only in these threads.

[mgallego-keeper]

Round 5, reviewed at f66f2372. I built at head with the Zulu 8 aarch64 toolchain and verified every item against the built jar, real Maven Central artifacts, or a live local proxy rather than reading the diff. Suite at head: 85 tests, 0 failures, ProxyTest at 23 cases in 0.006s.

Both round-4 blockers are still open. One received a cosmetic change that does not fix it, and the other was not touched. All 30 threads are marked resolved, so I have replied on the two that are not actually done and reopened them.

Fixed and verified this round:

• ResolvedProxy.toString() redacts: password=<redacted> in every context, confirmed by executing it.
• http.proxyHost tier removed, with a test. The reasoning in the comment is correct.
• Dead 443 branch, unused javax.net.ssl.* import and unused ambientWithCreds local all gone.
• uploadFile(options, ...) honors options.allowUnverifiedCertificate.
• cachingPostFunction logs on cache fallback.
• The serialVersionUID form change is right: javap confirms private static final long on the outer class, which is what ObjectStreamClass looks for.
• The headline feature works end to end at this head. Against a local 407-answering proxy: disabledSchemes goes from null to '' before openConnection, the proxy receives Proxy-authorization: Basic c3ZjOlMzY3IzdA==, and the failure converts to a SecretsManagerException naming the JVM flag, on the correct explicit branch, with no password in the message.

I also enumerated every outbound connection in the SDK. There are exactly three and all now route through openProxiedConnection, with one exception noted below.

The two blockers

1. The serialVersionUID value matches no artifact that exists. Released 16.6.6, 17.2.0 and 17.3.0 all compute 5401507264959279624; this commit pins 4308841855089494389. A real round-trip throws InvalidClassException in both directions, so the field currently guarantees the failure it was added to prevent. The number came from my round-3 comment and it was a reconstruction rather than a measurement, which is my error; round 4 corrected it and the correction did not land. One constant, verified: with 5401507264959279624L the round-trip succeeds against all three released jars and the suite stays at 85/85.

2. Redaction still leaks a scheme-less proxyUrl. redactProxyUrl is unchanged since round 4 and still returns early when there is no ://, so proxyUrl = "user:p@ssword@proxy.corp:8080" throws with the password in cleartext. The reason this is a real config and not a nonsense input: parseProxy supports scheme-less values by prepending http://, and I confirmed svc:S3cret!@proxy.corp:8080 resolves fine with isExplicit=true. Verified fix and the missing test are on the thread.

New this round

3. SecretsManagerOptions.copy() is a hard binary break. Identical bytecode compiled against released 17.3.0 links fine there and throws NoSuchMethodError against head. Same class of break as the KeeperFile one we fixed in round 1, so it wants a decision: accept and document, or keep proxyUrl out of the primary constructor.

4. new SecretsManagerOptions(storage, null) no longer compiles (ambiguous with the new convenience constructor). Carried from round 4. With item 3 this means "Breaking Changes: None" is not accurate.

5. The changelog entry is filed under ## 17.3.0, which is already on Maven Central and has zero proxy references in it. It belongs under ## 17.4.0.

6. Notation.getValue() still downloads attachments unproxied (Notation.kt:33), while the sibling getNotationResults was fixed this cycle. KeeperSecrets carries no options, so it cannot reach proxyUrl even in principle, and the README claims notation file lookups are covered.

Minor, none blocking

Details are inline: the 20-attempt proxy auth amplification on a wrong password (lockout risk), https.proxyPort='' yielding port 80 instead of 443, NO_PROXY='' masking no_proxy, the three-way disagreement on converting a 407 for an unauthenticated proxy, the cachingPostFunction warning naming the proxy on every offline run, the proxy internals still being public in the shipped bytecode (including an access$enableBasicProxyAuthOverTunnel() bridge), the downloadFile(file, url) overload trap, and the test whose name promises a gate it does not assert.

On bookkeeping: the deferrals are now recorded in 23999c6c's commit message and I am happy to treat that as sufficient. The exclusion matcher (re-probed: trailing wildcards and host:port still unmatched, no over-matching) and the process-wide disabledSchemes mutation are the two that remain open there.

Items 1 and 2 are one-liners I have already applied and verified end to end; I reverted my worktree afterwards so every number above describes the true PR head. Happy to pair on any of it.

[mgallego-keeper]

Minor, carried from round 4: keeping this private is right, but because ProxyAuthenticator is a separate class in the same file, the Kotlin compiler emits a bridge for it, and the bridge is public in the shipped bytecode:

public static final void access$enableBasicProxyAuthOverTunnel();

So the jar exports a one-call way for any consumer to clear the process-wide CVE-2016-5597 mitigation. The same applies to the rest of the file's internal surface, which is unmangled and public: resolveProxy, isExcluded, openProxiedConnection, checkedResponseCode, proxyAuthFailureMessage, redactProxyUrl.

trustAllSslSocketFactory is correctly private static final now with no bridge, so the half that mattered most from round 3 is genuinely closed. If you want the rest closed, moving these into an internal object (whose members do get name-mangled) or adding @JvmSynthetic does it; otherwise this is fine to leave recorded.

[mgallego-keeper]

Round 6, reviewed at a3663411. Built at head with the Zulu 8 aarch64 toolchain and verified every item against the built jar, real Maven Central artifacts, or a live local 407-answering proxy rather than by reading the diff. Suite at head: 87 tests, 0 failures, ProxyTest at 25 cases.

Round-5 blocker 1 is fixed and confirmed by measurement. Round-5 blocker 2 is half fixed: the scheme-less case now redacts, but the same commit introduced a wider leak, and the cause is the fix I proposed. Details and a verified one-line correction below.

Fixed and verified this round

• serialVersionUID now matches the released artifacts. I pulled 16.6.6, 17.2.0 and 17.3.0 from Maven Central and computed the value the JDK actually uses: all three are 5401507264959279624, and head now declares exactly that as private static final long. A real round-trip succeeds in all six directions (each released jar to head, and head to each released jar). This one is closed.
• secretsManagerExceptionSerialVersionUidMatchesReleasedJars is the right test. Worth knowing what it does and does not catch: an explicit constant suppresses the computation, so the assertion reads back the declared value and cannot detect a wrong number. What it does catch is the declaration form silently changing (a rename, losing private const, moving it off the companion), which is the realistic regression. That is the most a unit test can do here without shipping a reference jar.
• new SecretsManagerOptions(storage, null) compiles again. javac against head is clean; the ambiguity is gone with the secondary constructor removed. withProxy is public static final in the jar, resolves from Java, and produces the same defaults the old secondary constructor did (allowUnverifiedCertificate=false, loggingEnabled=true). The README's Kotlin form SecretsManagerOptions(storage, proxyUrl = "...") also compiles.
• The credential-less 407 guard is right. Live against a proxy that always answers 407: an explicit proxy with no credentials now rethrows the bare IOException, does not emit the CVE remediation, makes exactly one attempt, and leaves disabledSchemes untouched. Ambient credentials still take the ambient branch and are still (correctly) not sent on the wire.
• The headline feature works end to end at this head. Explicit http://svc:S3cr3t@127.0.0.1:PORT: disabledSchemes goes from null to '' before openConnection, the proxy receives Proxy-authorization: Basic c3ZjOlMzY3IzdA==, the failure converts to a SecretsManagerException on the explicit branch naming the JVM flag, the original IOException is chained as the cause, and no password appears in the message.
• https.proxyPort='' now yields 443, NO_PROXY='' no longer masks no_proxy, downloadFileFromUrl is renamed, and the changelog moved under ## 17.4.0.
• The Notation wording is accurate and specific about which of the two entry points is proxied. Documenting the gap was one of the two options I asked for, so this is done.
• cachingPostFunction reads the cache before claiming to serve it.

Re-confirmed at this head: there are exactly three outbound connection sites (SecretsManager.kt:1213, 1236, 1741) and all three route through openProxiedConnection. The only other URL construction in the SDK is TOTP parsing in CryptoUtils.kt:255, which never opens a connection.

Blocker: redactProxyUrl now leaks any password containing /

ProxySupport.kt:269-270. The scheme-less half of round-5 blocker 2 is fixed, but bounding the @ search to the authority means a / inside the password terminates the search early, so no userinfo is found and the raw value is returned. Measured against the built jars, same harness for both commits:

proxyUrl	f66f2372	a3663411 (head)
http://user:p@ssword@proxy.corp:8080	redacted	redacted
user:p@ssword@proxy.corp:8080	leaks	redacted
http://user:pa ss/word@proxy.corp:8080	redacted	leaks
user:pa ss/word@proxy.corp:8080	leaks	leaks
http://svc:a/b@proxy.corp:8080	redacted	leaks
http://user:S3cr#t/x@proxy:3128	redacted	leaks

All six rows reach the throw at ProxySupport.kt:50: I confirmed parseProxy returns null for every one of them, so each produces proxyUrl '<value>' could not be parsed as a valid proxy URL with the value inline. The commit closes two leaks and opens four, and three of the four worked correctly one commit ago.

http://svc:a/b@proxy.corp:8080 is the row that matters most: an ordinary http:// proxy URL whose password contains a single /, nothing exotic, no space, no @. A / in a generated service-account password is common, and it redacted correctly before this commit.

The authority bound came from my round-5 comment, and it was the wrong call. I optimized for a cosmetic case (an @ in a path being mistaken for a userinfo boundary) and paid for it with a credential leak. For a redaction helper the bias has to run the other way: over-redaction costs a little clarity in one error message, under-redaction puts a password in logs and APM. Dropping the bound restores that bias:

    val authorityStart = if (schemeEnd < 0) 0 else schemeEnd + 3
    val afterScheme = proxyUrl.substring(authorityStart)
    // Search the whole remainder rather than just the authority: an unencoded '/' in a password
    // would end the search early and return the value unredacted. Over-redacting a path is
    // harmless here; under-redacting puts a credential in the error message.
    val atIndex = afterScheme.lastIndexOf('@')

I applied exactly that and re-verified: all 15 inputs in my probe set redact or pass through unchanged with zero leaks, redactProxyUrl("notaurl") still returns "notaurl", and the suite stays at 87/87. The one thing it gives up is the path case, http://user:pass@proxy.corp:8080/p@th becoming http://user:***@th, which is over-redaction of a component parseProxy never reads.

Two tests would have caught this and are worth adding to redactProxyUrlStripsCredentialsTextually, which currently has no /-in-password case at all:

assertEquals("http://svc:***@proxy.corp:8080", redactProxyUrl("http://svc:a/b@proxy.corp:8080"))
assertEquals("user:***@proxy.corp:8080", redactProxyUrl("user:pa ss/word@proxy.corp:8080"))

If you would rather remove the class of bug than the instance, the structural option is to stop echoing the value: this helper exists only to make one message safe, parseProxy has already failed to tell host from password, so any echo is a guess about untrusted-shaped input. SecretsManagerOptions.toString() already prints proxyUrl=<redacted> and the codebase is otherwise consistent about never emitting the value. Naming the form instead of the value (proxyUrl could not be parsed; expected http://user:pass@host:port) makes the leak unreachable and deletes the helper.

Correcting my round-5 item 3: the binary break is wider than copy(), and it is not new

I reported this as a copy() break needing an accept-or-restructure decision. Both halves of that were wrong, and the correction matters because it changes what you should do.

It is wider than copy(). Adding a parameter to the primary constructor also changes the synthetic default constructor, which is what every Kotlin call site using default or named arguments compiles into. Identical Kotlin source compiled against released 17.3.0, run against head:

SecretsManagerOptions(st)                            -> NoSuchMethodError
SecretsManagerOptions(storage = st, loggingEnabled = false) -> NoSuchMethodError
SecretsManagerOptions(st).copy(loggingEnabled = false)      -> NoSuchMethodError
val (s, q, a, l) = options                                  -> NoSuchMethodError
SecretsManagerOptions(st, null, false, false, null, null, null)  -> OK

All four failures are the same missing descriptor, the 17.3.0 synthetic (KeyValueStorage, Function3, ZZ, String, String, Function1, int, DefaultConstructorMarker). Java callers are unaffected: @JvmOverloads keeps every released arity and I confirmed the 7-argument form links and runs.

It is not new. That descriptor has changed on every release in this line:

16.6.6 : (KeyValueStorage, Function3, Z,  int, DefaultConstructorMarker)
17.2.0 : (KeyValueStorage, Function3, ZZ, int, DefaultConstructorMarker)
17.3.0 : (KeyValueStorage, Function3, ZZ, String, String, Function1, int, DefaultConstructorMarker)
head   : (KeyValueStorage, Function3, ZZ, String, String, Function1, String, int, DefaultConstructorMarker)

Kotlin SecretsManagerOptions(storage) compiled against 17.2.0 throws NoSuchMethodError on released 17.3.0 today. So 17.3.0 shipped this identical break with no note, and holding this PR to a standard the last three releases did not meet would be inconsistent. Withdrawing the ask: no design change, and no decision needed. Adding proxyUrl to the primary constructor is the right call, since a body property would silently drop proxyUrl from copy() and lose the named-argument form the README documents.

Two wording fixes are left, both in the note you already added at README.md:8-10:

1. Widen it beyond copy(), since the current text tells a Kotlin caller who never calls copy() that they are unaffected. Something like: "Kotlin callers compiled against 17.3.0 must recompile against 17.4.0. Adding proxyUrl to the SecretsManagerOptions primary constructor changes the constructor and copy() signatures that Kotlin's default-argument calls bind to, so any Kotlin call site using default or named arguments throws NoSuchMethodError against a 17.4.0 jar until recompiled. Java callers are unaffected: all previously published constructor arities are retained."
2. Drop the second line about the new SecretsManagerOptions(storage, proxyUrl) convenience constructor. That constructor only ever existed inside this PR, so a changelog entry telling users it was replaced points at something they never had.

The PR description still says "Breaking Changes: None", which now contradicts the README. That is the one place a consumer looks first.

The merge conflict is self-inflicted and one revert away

mergeable: CONFLICTING. SecretsManager.kt auto-merges cleanly; the only conflict is README.md, and the sole cause is that this PR reflowed the two existing ## 17.4.0 entries (KSM-1081 and KSM-1086) from one line each into two. Base has since added KSM-1248 and KSM-1262 to the same block. Restoring those two entries byte-for-byte makes the merge clean and removes the risk of losing the base's two entries during a hand resolution. Whatever the resolution, please confirm KSM-1248 and KSM-1262 survive it.

Minor, none blocking

• The 20-attempt auth amplification reproduces at this head. One postFunction call against a proxy that always answers 407 produced exactly 20 CONNECTs: one unauthenticated, then 19 carrying the same rejected Proxy-authorization: Basic blob. One typo'd password locks a service account on any proxy with a failure threshold, and the SDK repeats it on every call. Same Authenticator object as the deferred chaining and unregister work, so it belongs in that ticket rather than here.
• One credentials predicate, four spellings, two meanings. ProxySupport.kt:152 and :186 require username and password; SecretsManager.kt:1259 and :1755 require username only. The observable effect is small (an ambient http://user@proxy:8080 with no password gets the helpful ambient message from postFunction and a bare 407 from the download path, which I confirmed live), but this predicate has now drifted in three consecutive rounds because it is written out four times. A single ResolvedProxy.hasCredentials property, used at all four sites, ends that.
• Both IOException catches call resolveProxy(proxyUrl, url) without the environment argument, so they always read the real process environment and cannot be exercised with FakeProxyEnvironment. Threading the already-resolved ResolvedProxy into checkedResponseCode instead would fix the testability and drop the second proxy resolution per request at the same time.
• The https.proxyPort='' and NO_PROXY='' guards both landed without a test. They are one-line takeIf guards next to guards that do have tests (blankSystemPropertyHostDegradesToNull, blankEnvVarDoesNotMaskLowerPriorityVar), so they are exactly the kind of line a future refactor drops silently.
• cachingPostFunction: reading the cache first is correct, but when the cache is missing, getCachedValue() throws "Cached value does not exist" and the original network error is now neither printed nor chained, so the operator loses the reason the request failed. Not a regression against shipped behavior, since base prints nothing at all here, just a gap in the new warning. Wrapping the read and chaining e as the cause covers both, and it is also the natural place to stop printing the proxy paragraph on every ordinary offline run when no proxy is configured anywhere.
• proxyAuthFailureMessageAmbientBranchRequiresCredentials still asserts only message content. Its comment claims the ambient branch "must only fire when username is non-null", which is a property of the four call sites, not of the function under test. Either rename it to match what it checks or assert the gate where it lives.
• access$enableBasicProxyAuthOverTunnel() is still public in the shipped jar, along with resolveProxy, isExcluded, openProxiedConnection, checkedResponseCode, proxyAuthFailureMessage and redactProxyUrl. Recorded rather than asked for, as agreed. parseProxy$default is correctly package-private and trustAllSslSocketFactory does not appear in the jar's public surface at all, so the two that mattered most are closed.

Bookkeeping

23999c6c's message is the agreed record of the deferrals, and it has drifted from the code in five places: the http.proxyHost tier was removed, notation file lookups are not all proxied, the secondary constructor became withProxy, cache-fallback logging is now implemented rather than deferred, and ProxyTest is at 25 cases not 22. Since the base branch merges by squash, that message will be replaced at merge time, so the deferral list needs to be carried into the squash body or into Jira tickets to survive. Two items are worth adding to it: the 407 retry amplification above, and the process-wide jdk.http.auth.tunneling.disabledSchemes mutation.

Unrelated to this PR, for whoever owns the release: build.gradle.kts still says version = "17.3.0" and KEEPER_CLIENT_VERSION is still "mj17.3.0" on a v17.4.0 branch. Both are inherited from base.

The redaction one-liner is the only thing standing between this and an approval from me, and it is verified. Happy to pair on any of it.

[mgallego-keeper]

Blocker: this bound leaks any password containing /, including with-scheme forms that redacted correctly one commit ago.

The scheme-less half of round-5 blocker 2 is fixed. But bounding the @ search to the authority means an unencoded / inside the password terminates the search early, no userinfo is found, and the raw value is returned. Measured with the same harness against both built jars:

proxyUrl	f66f2372	a3663411 (head)
http://user:p@ssword@proxy.corp:8080	redacted	redacted
user:p@ssword@proxy.corp:8080	leaks	redacted
http://user:pa ss/word@proxy.corp:8080	redacted	leaks
user:pa ss/word@proxy.corp:8080	leaks	leaks
http://svc:a/b@proxy.corp:8080	redacted	leaks
http://user:S3cr#t/x@proxy:3128	redacted	leaks

Every row is reachable: I confirmed parseProxy returns null for all six, so each one lands on the throw at line 50 and produces proxyUrl '<value>' could not be parsed as a valid proxy URL with the credential inline. The commit closes two leaks and opens four, three of which worked before it.

http://svc:a/b@proxy.corp:8080 is the row that matters most: an ordinary http:// proxy URL whose password contains a single /. No space, no @, nothing exotic. A / in a generated service-account password is common, and it redacted correctly at f66f2372.

The authority bound came from my round-5 comment, and it was the wrong call. I optimized for a cosmetic case (an @ in a path being mistaken for a userinfo boundary) and paid for it with a credential leak. A redaction helper has an asymmetric cost function: over-redacting costs a little clarity in one error string, under-redacting puts a password into logs and APM. The bias has to run toward over-redaction, which means dropping the bound:

    val authorityStart = if (schemeEnd < 0) 0 else schemeEnd + 3
    val afterScheme = proxyUrl.substring(authorityStart)
    // Search the whole remainder rather than just the authority: an unencoded '/' in a password
    // would end the search early and return the value unredacted. Over-redacting a path is
    // harmless here; under-redacting puts a credential in the error message.
    val atIndex = afterScheme.lastIndexOf('@')

I applied exactly that and re-verified against a rebuilt jar: all 15 inputs in my probe set redact or pass through unchanged with zero leaks, redactProxyUrl("notaurl") still returns "notaurl", and the suite stays at 87/87. The only thing it gives up is http://user:pass@proxy.corp:8080/p@th becoming http://user:***@th, which over-redacts a path component parseProxy never reads.

If you would rather remove the class of bug than the instance: this helper exists only to make one message safe, and by the time it runs parseProxy has already failed to tell host from password, so any echo of the value is a guess about malformed input. Naming the expected form instead of the value (proxyUrl could not be parsed; expected http://user:pass@host:port) makes the leak unreachable and deletes the function. That is also consistent with SecretsManagerOptions.toString(), which already prints proxyUrl=<redacted>.

[mgallego-keeper]

This is the test that would have caught the leak on the first run, and the gap is specific: there is no /-in-password case here. Every input in this block has either an @ or a space in the password, and both of those still redact at head.

Two rows close it:

assertEquals("http://svc:***@proxy.corp:8080", redactProxyUrl("http://svc:a/b@proxy.corp:8080"))
assertEquals("user:***@proxy.corp:8080", redactProxyUrl("user:pa ss/word@proxy.corp:8080"))

Both currently return the input unchanged, password included.

While in this file: the https.proxyPort='' and NO_PROXY='' guards added in this commit both landed without a test, even though the guards right next to them have blankSystemPropertyHostDegradesToNull and blankEnvVarDoesNotMaskLowerPriorityVar. One-line takeIf guards with no test are exactly what a future refactor drops silently.

Separately, proxyAuthFailureMessageAmbientBranchRequiresCredentials asserts only message content, while its comment claims the ambient branch "must only fire when username is non-null". That gate lives in the four call sites, not in the function under test. Either rename the test to match what it checks, or assert the gate where it actually is.

[mgallego-keeper]

Correcting my round-5 item 3: the break is wider than copy(), and it is not new. This needs a wording change, not a design change.

I reported this as a copy() break that wanted an accept-or-restructure decision. Both halves of that were wrong.

Wider than copy(). Adding a parameter to the primary constructor also changes the synthetic default constructor, which is what every Kotlin call site using default or named arguments binds to. Identical Kotlin source compiled against released 17.3.0, run against head:

SecretsManagerOptions(st)                                   -> NoSuchMethodError
SecretsManagerOptions(storage = st, loggingEnabled = false)  -> NoSuchMethodError
SecretsManagerOptions(st).copy(loggingEnabled = false)       -> NoSuchMethodError
val (s, q, a, l) = options                                   -> NoSuchMethodError
SecretsManagerOptions(st, null, false, false, null, null, null) -> OK

All four failures are the same missing descriptor, the 17.3.0 synthetic (KeyValueStorage, Function3, ZZ, String, String, Function1, int, DefaultConstructorMarker). Java callers are unaffected: @JvmOverloads retains every published arity and I confirmed the 7-argument form links and runs.

Not new. That descriptor has changed on every release in this line:

16.6.6 : (KeyValueStorage, Function3, Z,  int, DefaultConstructorMarker)
17.2.0 : (KeyValueStorage, Function3, ZZ, int, DefaultConstructorMarker)
17.3.0 : (KeyValueStorage, Function3, ZZ, String, String, Function1, int, DefaultConstructorMarker)
head   : (KeyValueStorage, Function3, ZZ, String, String, Function1, String, int, DefaultConstructorMarker)

Kotlin SecretsManagerOptions(storage) compiled against 17.2.0 throws NoSuchMethodError on released 17.3.0 today. So 17.3.0 shipped this identical break with no note, and holding this PR to a standard the last three releases did not meet would be inconsistent.

So I am withdrawing the ask. No design change, no decision needed. Keeping proxyUrl in the primary constructor is the right call: a body property would silently drop it from copy() and lose the named-argument form the README documents just above.

Two wording fixes remain in this block:

1. Widen it beyond copy(). As written it tells a Kotlin caller who never calls copy() that they are unaffected, and they are not. Suggested replacement:

   Kotlin callers compiled against 17.3.0 must recompile against 17.4.0. Adding proxyUrl to the SecretsManagerOptions primary constructor changes the constructor and copy() signatures that Kotlin's default-argument calls bind to, so any Kotlin call site using default or named arguments throws NoSuchMethodError against a 17.4.0 jar until recompiled. Java callers are unaffected: all previously published constructor arities are retained.

2. Drop the second line. The new SecretsManagerOptions(storage, proxyUrl) convenience constructor only ever existed inside this PR, so a changelog entry telling users it was replaced points at something they never had. withProxy is already documented as the Java form in the KSM-531 entry below.

The PR description still says "Breaking Changes: None", which now contradicts this section. That is the first place a consumer looks.

One more thing on this file, unrelated to the wording: the reflow of the existing KSM-1081 and KSM-1086 entries from one line each into two is the sole cause of the CONFLICTING merge state. SecretsManager.kt auto-merges cleanly. Base has since added KSM-1248 and KSM-1262 to this same block, so restoring those two entries byte-for-byte makes the merge clean and removes the risk of losing the base's entries during a hand resolution.

[mgallego-keeper]

Round 7, reviewed at a3663411, the same head as round 6: no new commits, so this round is a full verification pass over the entire PR rather than a response to a push. I rebuilt at head with the Zulu 8 aarch64 toolchain and re-verified every claim from all six rounds against the built jar, javap, a live local 407-answering proxy (on both a Java 8 and a JDK 23 runtime), and a merge simulation against today's base tip, plus a comparison against the proxy implementations in the other five KSM SDKs. Suite at head: 87 tests, 0 failures, ProxyTest at 25 cases.

First the good news: nothing regressed, and every fix from rounds 1 through 6 that was marked done is genuinely done. I re-measured the load-bearing ones rather than trusting my own earlier verdicts: the headline flow works end to end on a real Java 8 runtime (disabledSchemes goes null to '' before openConnection, the proxy receives Proxy-authorization: Basic c3ZjOlMzY3IzdA==, the failure converts on the explicit branch with no password in the message); the SUID round-trips; all released binary shapes are intact; unparseable ambient values still fall through to the default ProxySelector; and the Authenticator answers only for RequestorType.PROXY at the registered host:port. Also credited: the PR description now lists the copy() break, so the "Breaking Changes: None" contradiction from round 6 is gone. It needs the same widening as the README note (Kotlin default/named-argument callers, not just copy() callers), but the factual gap is closed.

Two things drive this round: the round-6 blocker is unchanged, and the merge picture changed today in a way that supersedes my round-6 guidance.

Still the one blocker: the redaction bound

ProxySupport.kt:269-270 is byte-identical to round 6, so every measurement stands: all four /-in-password rows still return unredacted, and the throw at ProxySupport.kt:50 still puts them in the message verbatim (proxyUrl 'http://svc:a/b@proxy.corp:8080' could not be parsed as a valid proxy URL). One new variant measured this round, worth knowing because the same one-liner fixes it:

input : http://user:p@ss/word@proxy.corp:8080
redact: http://user:***@ss/word@proxy.corp:8080     <- password tail leaks
parse : resolves "successfully" to host=ss port=80 user=user pass=p

The bounded search mistakes the first @ for the userinfo boundary and leaks the remainder, and URI happens to parse that shape into a nonsense proxy host derived from password text. The verified fix (drop the bound, search the whole remainder with lastIndexOf('@')) and the two missing test rows are on the round-6 thread. This remains the only code change standing between the PR and an approval from me.

The merge guidance from round 6 is stale: base moved today

On the 18th, SecretsManager.kt auto-merged and the README reflow was the sole conflict. That was true then and is not now: base has since taken KSM-1248 and KSM-1262, and today KSM-1203, KSM-1176, and KSM-1207. KSM-1207 (connect/read timeouts on every HttpsURLConnection, plus connectTimeoutMillis/readTimeoutMillis on SecretsManagerOptions) edits the exact functions this PR rewrote. A real merge simulation now conflicts in five hunks of SecretsManager.kt plus the README block: the options constructor, the download-helper block, the postFunction signature, the postFunction body, and the postQuery call site.

The resolution has semantic content, so spelling it out:

1. Options constructor: keep all three new parameters (connectTimeoutMillis, readTimeoutMillis, proxyUrl). Order is free since neither side has shipped, but it fixes the @JvmOverloads arities, so pick once.
2. postFunction: keep this PR's explicit 4-arg overload and extend the full form to (url, transmissionKey, payload, allowUnverifiedCertificate, proxyUrl, connectTimeoutMillis = DEFAULT_CONNECT_TIMEOUT_MS, readTimeoutMillis = DEFAULT_READ_TIMEOUT_MS). This one matters beyond the conflict: KSM-1207 removed the released 4-arg postFunction from base's bytecode. Kotlin default parameters without @JvmOverloads emit only the full form plus a $default synthetic; javap on a jar built from base tip shows exactly (String, TransmissionKey, EncryptedPayload, boolean, int, int) plus the synthetic, while released 17.3.0 ships (String, TransmissionKey, EncryptedPayload, boolean). Any caller of the released shape gets NoSuchMethodError against base tip today. Keeping this PR's 4-arg overload in the resolution repairs a break base introduced on its own.
3. postFunction body: keep the openProxiedConnection structure, keep base's two timeout lines, and drop base's if (allowUnverifiedCertificate) trustAllSocketFactory() branch. The PR deleted that function ( openProxiedConnection handles the flag), so keeping the branch fails compilation, which at least makes this mistake loud.
4. Download/upload helpers: base's timeout lines auto-merge into the try blocks correctly. One decision remains: base applied module-constant timeouts at the private helpers because their old callers carried no options, but this PR's downloadFile(options, file), downloadThumbnail(options, file), and uploadFile(options, ...) do carry options. Thread options.connectTimeoutMillis/readTimeoutMillis through those paths, or the new options fields silently will not apply to downloads and uploads. Same shape as the round-3 allowUnverifiedCertificate item, cheaper to fix at merge time than to rediscover in a round 8.
5. postQuery call: pass options.allowUnverifiedCertificate, options.proxyUrl, options.connectTimeoutMillis, options.readTimeoutMillis.
6. README: take base's 17.4.0 block verbatim (now seven entries: KSM-1203, KSM-1176, KSM-1207, KSM-1081, KSM-1086, KSM-1248, KSM-1262), then add the KSM-531 entry and the Breaking Changes section, and confirm all seven survive the resolution. Since KSM-1207's two fields land in the same synthetic-constructor signature as proxyUrl, the breaking note should describe the joint 17.4.0 constructor change once, with the round-6 wording: Kotlin callers using default or named arguments must recompile; Java callers are unaffected.

New this round

None of these blocks on its own. 1 through 3 are cheap and worth taking in the same push; 4 is a doc pass.

1. An explicit proxy is silently bypassed when the target URL's host is not URI-parseable. ProxySupport.kt:44 returns null when URI(targetUrl).host is null (underscore hosts, which java.net.URI refuses but java.net.URL happily connects to), and that return sits before the explicit branch. Measured against the built jar: resolveProxy("http://proxy.example.com:8080", "https://foo_bar.example.com/api") returns null, so the connection goes direct despite the caller mandating a proxy. This is the last fail-open path in the explicit-is-authoritative contract, same family as the round-3 bypass. The explicit branch never uses targetHost, so the fix is a reorder: hoist it above the target parse.
2. The HTTP_PROXY fallback for HTTPS traffic is a family divergence that contradicts this PR's own reasoning. ProxySupport.kt:58-59 falls back to HTTP_PROXY/http_proxy when no HTTPS variant is set. I surveyed all five other KSM SDKs: none routes HTTPS traffic through HTTP_PROXY (requests, Go's ProxyFromEnvironment, the .NET runtime, and reqwest all scope it to plain-http targets; curl agrees). And the in-code justification for removing the http.proxyHost tier in round 5 (ProxySupport.kt:66-68: applying an http-scoped setting to HTTPS traffic "would silently proxy traffic the operator may not have intended") applies verbatim to these two lines. Either drop them, which is consistent with the sysprop decision, or keep them as a deliberate documented divergence; right now the README presents the fallback as unremarkable.
3. http://:password@host is silently ignored while http://user@host fails loudly. ProxySupport.kt:92-98. Measured: a password-only userinfo resolves with username=null, credentials are never registered, and the caller gets an unexplained 407, while the mirrored partial throws the explicit-config error. Extend the partial-userinfo guard to both halves.
4. README accuracy, beyond the two round-6 wording items:
   • "The single-argument forms downloadFile(file) and downloadThumbnail(file) open a direct connection" is wrong: they route through openProxiedConnection(url, null, false), which applies ambient env and sysprop proxies. The PR description gets this right ("use ambient proxy settings"); the README contradicts it.
   • "Notation lookups that resolve file attachments (getValue) are not proxied" overstates for the same reason: getValue is ambient-proxy-aware, it just cannot carry options.proxyUrl. Say that instead.
   • Missing upgrade note: for existing deployments, 17.4.0 makes every SDK connection start honoring HTTPS_PROXY/HTTP_PROXY env vars the JVM previously ignored. Containers commonly carry these for other tools; one changelog sentence prevents a surprise.
   • The cachingPostFunction stderr text ("does not support a proxy") overstates too: ambient proxies do apply through the default 4-arg postFunction; it cannot carry options.proxyUrl.
   • Precision nit: the CVE default latches when the JDK's HttpURLConnection class initializes, which a plain http:// connection (or an unconnected openConnection()) also triggers, not only "any HTTPS connection". Probe-confirmed with a plain-http latch.
   • The expansion of allowUnverifiedCertificate to download/upload paths is a real behavior change for users who had it enabled (more paths now skip verification) and is unmentioned in the changelog.

For the deferral record, alongside what is already there

• The set-if-null guard cannot see net.properties. The shipped CVE-2016-5597 default lives in jre/lib/net.properties (jdk.http.auth.tunneling.disabledSchemes=Basic), not as a system property, so System.getProperty(...) == null is true on a stock JVM and also on a JVM whose admin hardened the file value (say Basic,Digest,NTLM), and the SDK overwrites that hardening process-wide. The "unless the host app set it explicitly" comment only holds for -D flags.
• The matcher displaces the JDK's implicit exclusions. With https.proxyHost set, the SDK's own matcher takes over from the default ProxySelector and loses the JDK's built-in localhost|127.*|[::1] non-proxy default, so localhost targets that went direct before this PR are now proxied. Belongs with the recorded matcher items (trailing wildcards, host:port, CIDR, bracketed-IPv6 entries).
• ProxyTest pollution: proxyAuthenticatorAnswersOnlyForRegisteredProxy leaves disabledSchemes="" set and proxy.local:8080 credentials registered in the process-wide singleton; the @AfterTest resets only the Authenticator.
• ResolvedProxy.toString() prints the username in clear (the password is redacted). Proxy usernames are often service-account identifiers.
• Credential encoding guidance is missing. Percent-encoding is the supported way to pass reserved characters and it works (measured: p%40ss%2Fword decodes to p@ss/word), but neither the README nor the parse-failure message says so. One hint in each also takes pressure off redactProxyUrl, since encoded credentials parse fine and never reach it. Related, measured on Java 8: Basic auth bytes are Latin-1 with char truncation (0xE4 for a-umlaut, the Euro sign truncated to 0xAC), so non-Latin-1 passwords authenticate with corrupted bytes; a README sentence is all the SDK can do about a JDK behavior.
• Informational, two items. The 407 surfaces through two JDK branches: with a Connection: close proxy response, both Java 8 and JDK 23 return 407 as a code (measured), which takes the HTTP_PROXY_AUTH branch where there is legitimately no exception to chain, so do not be surprised by a cause-less instance of the remediation exception. And on Android, HttpsURLConnection is OkHttp-backed and its tunnel-failure text contains no "407" (from the OkHttp sources, not measured here), so the remediation degrades to the raw IOException there. An optional third message for "proxy demands auth, none configured" (currently a bare IOException by the round-6 agreed design) would cover the most common first-contact failure; fine as a ticket note.

Checked and cleared

So nothing above gets chased that should not be: the sysprop IPv6/underscore case is not a regression at head (the unparseable ambient candidate falls through to the default ProxySelector, which is the round-3 fix working as intended, re-verified); the Authenticator does not leak credentials to server 401s (guarded and tested); and percent-decoding is correct (the .NET and Ruby SDKs get this wrong; Java does not).

Cross-SDK standing

Measured against the family: this PR lands on the correct side of every norm the six SDKs agree on (explicit per-client URL option, explicit beating env and NO_PROXY, fail-closed on unparseable explicit config, decoded credentials), and exceeds the family in three places: the only SDK with a dedicated 407 remediation, the only caching fallback that warns instead of silently masking proxy failures, and the deepest proxy test suite. The two Java-specific divergences (challenge-based auth via the global Authenticator, and the resulting 20-CONNECT amplification on a wrong password, re-measured this round at 1 bare plus 19 authenticated attempts, bounded by http.maxRedirects) are platform constraints and are documented or recorded. The one unforced divergence is item 2 above. The survey also turned up gaps in the other SDKs (.NET and Go do not proxy file downloads, Go fails open on an unparseable explicit proxy, Ruby's caching fallback bypasses the proxy, .NET and Ruby do not decode credentials); those are not this PR's problem and I will ticket them separately.

Where this lands

1. The redaction one-liner plus its two test rows (the verified fix is on the round-6 thread). Only code change gating approval.
2. Merge across today's base using the guidance above; the 4-arg postFunction and the options-timeout threading are the two spots where a mechanical resolution loses something real.
3. Doc pass: the two round-6 wording items, the same widening in the PR description, and the accuracy set in item 4.
4. Items 1 through 3 above are one-liners worth taking in the same push; item 2 needs a decision (drop or document).
5. Fold the deferral additions into the recorded list, and carry that list into the squash body at merge time, since the base branch squash-merges and 23999c6c's message will not survive.

Bookkeeping, unchanged from round 6 and for whoever owns the release: build.gradle.kts still says 17.3.0 and KEEPER_CLIENT_VERSION is still mj17.3.0 on the v17.4.0 branch, so the jar builds as core-17.3.0.jar even at head. All five open threads remain valid at this head; nothing new needed reopening.

Happy to pair on any of it, and the next round should be quick: with the one-liner and a clean merge this is an approval.

[mgallego-keeper]

Round 8, reviewed at 3fd94e07. Rebuilt at head with the Zulu 8 aarch64 toolchain and re-verified every round-6 and round-7 item against the built jar and a live local 407-answering proxy on a Java 8 runtime, rather than by reading the diff. Suite at head: 93 tests, 0 failures, ProxyTest at 25 cases.

Everything asked for is genuinely done. The redaction fix shows zero leaks across my full probe set (all four former /-password rows, the scheme-less forms, and the partial-leak variant), and the thrown parse-failure message now redacts. The explicit-proxy bypass for non-URI-parseable target hosts is fixed, HTTP_PROXY is correctly out of the HTTPS fallback chain, password-only userinfo now fails loudly without echoing the password, hasCredentials unified the predicate at all four sites, options timeouts thread through every outbound path, and the README pass (ambient wording, upgrade note, percent-encoding hint, CVE latch scope, allowUnverifiedCertificate note) is accurate against the code. The headline feature works end to end at this head: disabledSchemes cleared before openConnection, Proxy-authorization: Basic on the wire, and the explicit-branch exception with no credential in the message.

Approving. The remaining merge conflict against today's base tip is resolution work I will handle on my side; the one thing that must survive it is the SecretsManagerException serialVersionUID pin, and this PR's own test enforces that. A short list of residual nits (a stale HTTP_PROXY mention in the ambient 407 message, a couple of stale comments, and five missing one-line tests) can ride the deferral ticket.

Thanks for the thorough set of rounds, Stas. Nice work on this one.