Repository navigation
Fix/review findings - #1
Merged
Merged
Conversation
The dial-time address check only unmapped IPv4-mapped IPv6 addresses before testing for private, loopback and link-local ranges. An IPv6 address that embeds an internal IPv4 address in another form passed: a NAT64 address such as 64:ff9b::a9fe:a9fe, which a DNS64/NAT64 gateway translates to 169.254.169.254, an IPv4-compatible address such as ::127.0.0.1, and 6to4 and Teredo addresses. Several IPv4 special-purpose ranges (0.0.0.0/8, 192.0.0.0/24, 198.18.0.0/15, 240.0.0.0/4, the broadcast address and the documentation ranges) were also allowed. The check now requires a global unicast, non-private address, judges a well-known-prefix NAT64 address by the IPv4 address it embeds, and refuses the IANA special-purpose ranges that are not public destinations, including local-use NAT64, IPv4-compatible, 6to4, Teredo and site-local addresses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…llowPrefixes Policy.AllowPrivate turned off every address check, so a deployment that set it to reach an internal mock could also reach 169.254.169.254, fd00:ec2::254 and other cloud metadata endpoints, and every loopback and private range. Link-local addresses (169.254.0.0/16, fe80::/10), well-known cloud metadata addresses, Teredo and local-use NAT64 addresses, and NAT64, IPv4-compatible and 6to4 addresses embedding one of them, are now refused whatever the policy. A new Policy.AllowPrefixes lists the specific non-public ranges a development or test setup may reach, such as 127.0.0.0/8. AllowPrivate still allows the other non-public addresses but is deprecated in favour of AllowPrefixes. BREAKING CHANGE: a policy with AllowPrivate set no longer reaches link-local or cloud metadata addresses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
egress.Client cloned Policy.Transport and replaced only DialContext. When the base transport set DialTLSContext or DialTLS, net/http used that for HTTPS connections instead of DialContext, so the dial-time address check never ran and a provider host resolving to a loopback, private or metadata address was reached. The cloned transport now has DialTLSContext, DialTLS and Dial cleared, so TLS is negotiated over a connection made by the checked dialer. TLS settings, including client certificates, still come from the base transport's TLSClientConfig. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ProviderClient built a new egress client, and so a new http.Transport, for every validation and every HTTPClient call. The transports were never closed, so each kept its idle keep-alive connection and its read and write goroutines until the idle timeout. Repeatedly testing a connection could grow the server's goroutines and file descriptors without bound and open many connections to the provider. The broker now builds the restricted client once per set of manifest hosts and hands out copies that share its transport, so connections are pooled. A caller's change to its copy, such as its timeout, does not affect other callers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reconnect refused a replacement credential for a different account only when both the stored account ID and the newly validated one were present. When the provider returned no account ID, for example an apikey provider without an AccountIDPath or a response without the configured field, the check was skipped and a credential for any account was installed under the existing connection, so every consumer bound to it started reading another account. The check now fails closed. Create refuses a credential whose validation returns no account ID, and Reconnect refuses with the new ErrAccountUnknown when either the provider's or the connection's account ID is missing. An auth method whose provider cannot name accounts opts out with the new AuthMethod.AccountUnverifiable; a reconnect that then cannot compare accounts proceeds and its audit event carries account=unverified. Accounts named on both sides must still match. apikey.New requires an AccountIDPath unless the method is AccountUnverifiable. BREAKING CHANGE: providers whose validation returns no account ID must set AccountUnverifiable on the auth method; connections stored without an account ID for other methods can no longer be reconnected and must be connected again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Test stored the full validation error in Health.LastError, which anyone who may view the connection reads. A provider's refusal embedded its request-ID response header verbatim, with no limit on length or content, and a network or egress failure carried the request URL and the resolved address, including internal addresses an egress refusal named. provider.Classify now keeps at most 128 printable ASCII characters of the request ID. Test records a provider's classified refusal (provider, class, status and the bounded request ID) and, for any other failure, only the provider and the failure's class, such as a transient failure. The full error is still in the audit event's reason. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SwapCredential inserted the new credential version and kept every earlier ciphertext until the connection's credentials were destroyed on disconnect or delete. Each Reconnect therefore left the replaced credential sealed in storage, so a later compromise of the encryption key would expose every historical credential, including one replaced because it was suspected leaked but could not be revoked at the provider. SwapCredential now removes the versions older than the one it installs, in the same operation: in a transaction in sqlstore, under the lock in memstore, which also zeroes the removed ciphertext. The active version is still the only one ever read, and concurrent swaps are still decided by the version check and the unique index. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.