Skip to content
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.

Sign up for GitHub

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

tls: fix RELEASE_ASSERT when using auto_sni #33637

Merged
merged 3 commits into from
Apr 18, 2024
Merged

tls: fix RELEASE_ASSERT when using auto_sni #33637

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b72b2b
tls: fix RELEASE_ASSERT when using `auto_sni`
ggreenway Apr 17, 2024
e51915b
fix further crash if dns resolution doesn't fail
ggreenway Apr 17, 2024
5ddc04b
fix comments
ggreenway Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelogs/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ minor_behavior_changes:

bug_fixes:
# *Changes expected to improve the state of the world and are unlikely to have negative effects*
- area: tls
change: |
Fix a RELEASE_ASSERT when using :ref:`auto_sni <envoy_v3_api_field_config.core.v3.UpstreamHttpProtocolOptions.auto_sni>`
if the downstream request ``:authority`` was longer than 255 characters.
removed_config_or_runtime:
# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
Expand Down
4 changes: 3 additions & 1 deletion source/common/tls/context_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,9 @@ ClientContextImpl::newSsl(const Network::TransportSocketOptionsConstSharedPtr& o
: server_name_indication_;
if (!server_name_indication.empty()) {
const int rc = SSL_set_tlsext_host_name(ssl_con.get(), server_name_indication.c_str());
RELEASE_ASSERT(rc, Utility::getLastCryptoError().value_or(""));
if (rc != 1) {
return nullptr;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be very confusing to debug if this is happening to people. Can we leave a TODO to somehow add a response code / message specific to this case? Possibly could do a quick WARN_EVERY log message as a stop gap. Up to you.

}

if (options && !options->verifySubjectAltNameListOverride().empty()) {
Expand Down
16 changes: 16 additions & 0 deletions test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,22 @@ TEST_P(ProxyFilterIntegrationTest, UpstreamTlsWithIpHost) {
checkSimpleRequestSuccess(0, 0, response.get());
}

TEST_P(ProxyFilterIntegrationTest, UpstreamTlsWithTooLongSni) {
upstream_tls_ = true;
initializeWithArgs();
std::string too_long_sni(300, 'a');
ASSERT_EQ(too_long_sni.size(), 300); // Validate that the expected constructor was run.
codec_client_ = makeHttpConnection(lookupPort("http"));
const Http::TestRequestHeaderMapImpl request_headers{{":method", "POST"},
{":path", "/test/long/url"},
{":scheme", "http"},
{":authority", too_long_sni}};

auto response = codec_client_->makeHeaderOnlyRequest(request_headers);
ASSERT_TRUE(response->waitForEndStream());
EXPECT_EQ("503", response->headers().getStatusValue());
}

// Verify that auto-SAN verification fails with an incorrect certificate.
TEST_P(ProxyFilterIntegrationTest, UpstreamTlsInvalidSAN) {
upstream_tls_ = true;
Expand Down
Loading