-
Notifications
You must be signed in to change notification settings - Fork 268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
addr: fix to_http_authority panic with IPv6
#976
Merged
Merged
Conversation
This file contains 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
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Currently, the `Addr::to_http_authority` method panics when called on a `SocketAddr` which is an IPv6 address with port 80. This method does not panic when called with IPv4 addresses, or with IPv6 addresses whose ports are *not* port 80. This was initially caught by oss-fuzz; see [here][1] for details. The panic occurs because when an IPv6+ address occurs in an authority, it must be within square brackets, as per [RFC3986, Section 3.2][2]. The square brackets distinguish between colons in the IPv6 address and the colon separating the address and port. When the `SocketAddr`'s port is not port 80, we format it including the port, and the `fmt::Display` output from IPv6 `SocketAddr`s includes the square brackets as expected. However, when the socket's port *is* port 80, we have special logic for eliding the port from the authority. This works fine for IPv4, where we can just call `addr.ip().to_string()` to nicely format the address. However, with IPv6 addresses, this only formats the address itself, *not* the square brackets. According to RFC3986, square brackets are mandatory for *all* IPv6 addresses, even when port 80 is elided. This branch fixes the panic by changing `Addr::to_http_authority` to include square brackets when formatting IPv6 `SocketAddr`s with port 80. I've also improved on @olix0r's original test cases from dbf898a to include IPv6 addrs with and without shorthand, and to test ports that are and are not port 80. These tests helped catch the panic, and may be useful to guard against future regressions. Fixes linkerd/linkerd2#6020 [1]: https://oss-fuzz.com/testcase-detail/6502844766224384 [2]: https://tools.ietf.org/html/rfc3986#section-3.2 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
olix0r
approved these changes
Apr 12, 2021
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
kleimkuhler
approved these changes
Apr 12, 2021
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
Apr 21, 2021
This release primarily improves protocol detection error messages in the admin server so that logs clearly indicate when the client expected a different TLS server identity than that of the running proxy. A number of internal improvements have been made, especially eliminating some potential runtime panics detected by oss-fuzz. It is not expected that these panics could be triggered in typical cluster configurations. --- * discover: replace `linkerd-channel` with `tokio-util` `PollSender` (linkerd/linkerd2-proxy#969) * replace `linkerd-channel` with `tokio-stream` (linkerd/linkerd2-proxy#970) * concurrency-limit: use `tokio-util`'s `PollSemaphore` (linkerd/linkerd2-proxy#968) * http: Do not fail fuzz tests when all IO is not read (linkerd/linkerd2-proxy#973) * transport: Fix orig-dst compilation on non-Linux targets (linkerd/linkerd2-proxy#974) * Update trust-dns to fix possible panic (linkerd/linkerd2-proxy#975) * addr: fix `to_http_authority` panic with IPv6 (linkerd/linkerd2-proxy#976) * outbound: skip logical stacks when no profile is discovered (linkerd/linkerd2-proxy#963) * split: change traffic splits to require a profile (linkerd/linkerd2-proxy#964) * inbound: only build profile route stacks when a profile is resolved (linkerd/linkerd2-proxy#966) * profiles: make receiver param in `route_request` non-optional (linkerd/linkerd2-proxy#967) * outbound: move target types into stack modules (linkerd/linkerd2-proxy#971) * outbound: only build logical stacks for profiles with logical addrs (linkerd/linkerd2-proxy#972) * app: inbound: add fuzzer (linkerd/linkerd2-proxy#977) * admin: Fail connections when HTTP detection fails (linkerd/linkerd2-proxy#979) * reduce error boilerplate with `thiserror` (linkerd/linkerd2-proxy#980) * app: inbound: fuzzer: generalise fuzzers and solve coverage build (linkerd/linkerd2-proxy#978) * admin: Assume meshed connections are HTTP/2 (linkerd/linkerd2-proxy#982) * chore: Fix deprecations on nightly (linkerd/linkerd2-proxy#983)
olix0r
added a commit
that referenced
this pull request
Apr 29, 2021
Currently, the `Addr::to_http_authority` method panics when called on a `SocketAddr` which is an IPv6 address with port 80. This method does not panic when called with IPv4 addresses, or with IPv6 addresses whose ports are *not* port 80. This was initially caught by oss-fuzz; see [here][1] for details. The panic occurs because when an IPv6+ address occurs in an authority, it must be within square brackets, as per [RFC3986, Section 3.2][2]. The square brackets distinguish between colons in the IPv6 address and the colon separating the address and port. When the `SocketAddr`'s port is not port 80, we format it including the port, and the `fmt::Display` output from IPv6 `SocketAddr`s includes the square brackets as expected. However, when the socket's port *is* port 80, we have special logic for eliding the port from the authority. This works fine for IPv4, where we can just call `addr.ip().to_string()` to nicely format the address. However, with IPv6 addresses, this only formats the address itself, *not* the square brackets. According to RFC3986, square brackets are mandatory for *all* IPv6 addresses, even when port 80 is elided. This branch fixes the panic by changing `Addr::to_http_authority` to include square brackets when formatting IPv6 `SocketAddr`s with port 80. I've also improved on @olix0r's original test cases from dbf898a to include IPv6 addrs with and without shorthand, and to test ports that are and are not port 80. These tests helped catch the panic, and may be useful to guard against future regressions. Fixes linkerd/linkerd2#6020 [1]: https://oss-fuzz.com/testcase-detail/6502844766224384 [2]: https://tools.ietf.org/html/rfc3986#section-3.2 Signed-off-by: Eliza Weisman <eliza@buoyant.io> Co-authored-by: Oliver Gould <ver@buoyant.io>
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
May 12, 2021
This release simplifies internals so that endpoint-forwarding logic is completely distinct from handling of load balanced services. The ingress-mode outbound proxy has been simplified to *require* the `l5d-dst-override` header and to fail non-HTTP communication. This ensures that the ingress-mode proxy does not unexpectedly revert to insecure communication. Finally, a regression was recently introduced that caused all proxy logs to be output with ANSI control characters. Logs are now output in plaintext by default --- * discover: replace `linkerd-channel` with `tokio-util` `PollSender` (linkerd/linkerd2-proxy#969) * replace `linkerd-channel` with `tokio-stream` (linkerd/linkerd2-proxy#970) * concurrency-limit: use `tokio-util`'s `PollSemaphore` (linkerd/linkerd2-proxy#968) * http: Do not fail fuzz tests when all IO is not read (linkerd/linkerd2-proxy#973) * transport: Fix orig-dst compilation on non-Linux targets (linkerd/linkerd2-proxy#974) * Update trust-dns to fix possible panic (linkerd/linkerd2-proxy#975) * addr: fix `to_http_authority` panic with IPv6 (linkerd/linkerd2-proxy#976) * outbound: skip logical stacks when no profile is discovered (linkerd/linkerd2-proxy#963) * split: change traffic splits to require a profile (linkerd/linkerd2-proxy#964) * inbound: only build profile route stacks when a profile is resolved (linkerd/linkerd2-proxy#966) * profiles: make receiver param in `route_request` non-optional (linkerd/linkerd2-proxy#967) * outbound: move target types into stack modules (linkerd/linkerd2-proxy#971) * outbound: only build logical stacks for profiles with logical addrs (linkerd/linkerd2-proxy#972) * app: inbound: add fuzzer (linkerd/linkerd2-proxy#977) * admin: Fail connections when HTTP detection fails (linkerd/linkerd2-proxy#979) * reduce error boilerplate with `thiserror` (linkerd/linkerd2-proxy#980) * app: inbound: fuzzer: generalise fuzzers and solve coverage build (linkerd/linkerd2-proxy#978) * admin: Assume meshed connections are HTTP/2 (linkerd/linkerd2-proxy#982) * chore: Fix deprecations on nightly (linkerd/linkerd2-proxy#983) * fuzz: Add logging to fuzz targets (linkerd/linkerd2-proxy#985) * fuzz: don't panic when the proxy closes the conn (linkerd/linkerd2-proxy#986) * Commit lock files for fuzzers (linkerd/linkerd2-proxy#984) * fuzz: allow client requests to fail (linkerd/linkerd2-proxy#989) * tower: update dependency to 0.4.7 (linkerd/linkerd2-proxy#990) * outbound: Make the Endpoint::logical_addr field optional (linkerd/linkerd2-proxy#991) * trace: explicitly disable ANSI terminal colors (linkerd/linkerd2-proxy#994) * ingress: Require the l5d-dst-override header (linkerd/linkerd2-proxy#992) * outbound: Do not support TCP-forwarding in ingress-mode (linkerd/linkerd2-proxy#995) * Decouple tcp forward stack from Endpoint target (linkerd/linkerd2-proxy#996) * Pickup linkerd-await wrapper in docker build (linkerd/linkerd2-proxy#999) * docs: Add fuzzing report (linkerd/linkerd2-proxy#1000) * tests: Use io::Error in mocked connector (linkerd/linkerd2-proxy#1001) * outbound: Decouple endpoint & logical stacks (linkerd/linkerd2-proxy#1002)
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
May 18, 2021
This release simplifies internals so that endpoint-forwarding logic is completely distinct from handling of load balanced services. The ingress-mode outbound proxy has been simplified to *require* the `l5d-dst-override` header and to fail non-HTTP communication. This ensures that the ingress-mode proxy does not unexpectedly revert to insecure communication. Finally, a regression was recently introduced that caused all proxy logs to be output with ANSI control characters. Logs are now output in plaintext by default --- * discover: replace `linkerd-channel` with `tokio-util` `PollSender` (linkerd/linkerd2-proxy#969) * replace `linkerd-channel` with `tokio-stream` (linkerd/linkerd2-proxy#970) * concurrency-limit: use `tokio-util`'s `PollSemaphore` (linkerd/linkerd2-proxy#968) * http: Do not fail fuzz tests when all IO is not read (linkerd/linkerd2-proxy#973) * transport: Fix orig-dst compilation on non-Linux targets (linkerd/linkerd2-proxy#974) * Update trust-dns to fix possible panic (linkerd/linkerd2-proxy#975) * addr: fix `to_http_authority` panic with IPv6 (linkerd/linkerd2-proxy#976) * outbound: skip logical stacks when no profile is discovered (linkerd/linkerd2-proxy#963) * split: change traffic splits to require a profile (linkerd/linkerd2-proxy#964) * inbound: only build profile route stacks when a profile is resolved (linkerd/linkerd2-proxy#966) * profiles: make receiver param in `route_request` non-optional (linkerd/linkerd2-proxy#967) * outbound: move target types into stack modules (linkerd/linkerd2-proxy#971) * outbound: only build logical stacks for profiles with logical addrs (linkerd/linkerd2-proxy#972) * app: inbound: add fuzzer (linkerd/linkerd2-proxy#977) * admin: Fail connections when HTTP detection fails (linkerd/linkerd2-proxy#979) * reduce error boilerplate with `thiserror` (linkerd/linkerd2-proxy#980) * app: inbound: fuzzer: generalise fuzzers and solve coverage build (linkerd/linkerd2-proxy#978) * admin: Assume meshed connections are HTTP/2 (linkerd/linkerd2-proxy#982) * chore: Fix deprecations on nightly (linkerd/linkerd2-proxy#983) * fuzz: Add logging to fuzz targets (linkerd/linkerd2-proxy#985) * fuzz: don't panic when the proxy closes the conn (linkerd/linkerd2-proxy#986) * Commit lock files for fuzzers (linkerd/linkerd2-proxy#984) * fuzz: allow client requests to fail (linkerd/linkerd2-proxy#989) * tower: update dependency to 0.4.7 (linkerd/linkerd2-proxy#990) * outbound: Make the Endpoint::logical_addr field optional (linkerd/linkerd2-proxy#991) * trace: explicitly disable ANSI terminal colors (linkerd/linkerd2-proxy#994) * ingress: Require the l5d-dst-override header (linkerd/linkerd2-proxy#992) * outbound: Do not support TCP-forwarding in ingress-mode (linkerd/linkerd2-proxy#995) * Decouple tcp forward stack from Endpoint target (linkerd/linkerd2-proxy#996) * Pickup linkerd-await wrapper in docker build (linkerd/linkerd2-proxy#999) * docs: Add fuzzing report (linkerd/linkerd2-proxy#1000) * tests: Use io::Error in mocked connector (linkerd/linkerd2-proxy#1001) * outbound: Decouple endpoint & logical stacks (linkerd/linkerd2-proxy#1002)
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.
Currently, the
Addr::to_http_authoritymethod panics when called on aSocketAddrwhich is an IPv6 address with port 80. This method does notpanic when called with IPv4 addresses, or with IPv6 addresses whose
ports are not port 80. This was initially caught by oss-fuzz; see
here for details.
The panic occurs because when an IPv6+ address occurs in an authority,
it must be within square brackets, as per RFC3986, Section 3.2. The
square brackets distinguish between colons in the IPv6 address and the
colon separating the address and port. When the
SocketAddr's port isnot port 80, we format it including the port, and the
fmt::Displayoutput from IPv6
SocketAddrs includes the square brackets as expected.However, when the socket's port is port 80, we have special logic for
eliding the port from the authority. This works fine for IPv4, where we
can just call
addr.ip().to_string()to nicely format the address.However, with IPv6 addresses, this only formats the address itself,
not the square brackets. According to RFC3986, square brackets are
mandatory for all IPv6 addresses, even when port 80 is elided.
This branch fixes the panic by changing
Addr::to_http_authoritytoinclude square brackets when formatting IPv6
SocketAddrs with port 80.I've also improved on @olix0r's original test cases from
dbf898a to include IPv6 addrs with and without
shorthand, and to test ports that are and are not port 80. These tests
helped catch the panic, and may be useful to guard against future
regressions.
Fixes linkerd/linkerd2#6020
Signed-off-by: Eliza Weisman eliza@buoyant.io