-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
http: making upstream ALPN accesible. #18884
Changes from 5 commits
8111956
2765421
26a4c95
d3d092b
296facf
1cf8942
2bb138d
60895d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -190,6 +190,18 @@ const std::string& ConnectionInfoImplBase::tlsVersion() const { | |
return cached_tls_version_; | ||
} | ||
|
||
const std::string& ConnectionInfoImplBase::alpn() const { | ||
if (alpn_.empty()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we are hard-coding "h3" in server session right now. Would this cause inconsistency between the ALPN returned via Ssl::ConnectionInfo and the one returned via ConnectionSocket::requestedApplicationProtocols()? requestedApplicationProtocols() is only used in server side, so I guess the upstream connection's connection socket doesn't have ALPN set there, either will it be used. But the changes in this PR applies to downstream as well, hence HCM or the filters can use both the new interface and requestedApplicationProtocols() to get the downstream ALPN and they can be different. Maybe overriding this implementation in QuicSslConnectionInfo according to perspective? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aren't we hard-coding h3 as the thing to negotiate? I think returning what boring returns is correct here. |
||
const unsigned char* proto; | ||
unsigned int proto_len; | ||
SSL_get0_alpn_selected(ssl(), &proto, &proto_len); | ||
if (proto != nullptr) { | ||
alpn_ = std::string(reinterpret_cast<const char*>(proto), proto_len); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LOL. C chars are so annoying. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry quick question: this code must exist somewhere else, right? Like in TLS inspector or somewhere else? Should we factor it out into some utility function? /wait-any |
||
} | ||
} | ||
return alpn_; | ||
} | ||
|
||
const std::string& ConnectionInfoImplBase::serialNumberPeerCertificate() const { | ||
if (!cached_serial_number_peer_certificate_.empty()) { | ||
return cached_serial_number_peer_certificate_; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
#include "envoy/registry/registry.h" | ||
#include "envoy/server/filter_config.h" | ||
|
||
#include "source/extensions/filters/http/common/pass_through_filter.h" | ||
|
||
#include "test/extensions/filters/http/common/empty_http_filter_config.h" | ||
#include "test/integration/filters/common.h" | ||
|
||
#include "gtest/gtest.h" | ||
|
||
namespace Envoy { | ||
|
||
// A filter that sticks stream info into headers for integration testing. | ||
class StreamInfoToHeadersFilter : public Http::PassThroughFilter { | ||
alyssawilk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public: | ||
constexpr static char name[] = "stream-info-to-headers-filter"; | ||
|
||
Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool) override { | ||
return Http::FilterHeadersStatus::Continue; | ||
} | ||
|
||
Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers, bool) override { | ||
if (decoder_callbacks_->streamInfo().upstreamSslConnection()) { | ||
headers.addCopy(Http::LowerCaseString("alpn"), | ||
decoder_callbacks_->streamInfo().upstreamSslConnection()->alpn()); | ||
} | ||
return Http::FilterHeadersStatus::Continue; | ||
} | ||
}; | ||
|
||
constexpr char StreamInfoToHeadersFilter::name[]; | ||
static Registry::RegisterFactory<SimpleFilterConfig<StreamInfoToHeadersFilter>, | ||
Server::Configuration::NamedHttpFilterConfigFactory> | ||
register_; | ||
|
||
} // namespace Envoy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly surprised this doesn't already exist!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's nextProtocol in one of the other classes, but it's not accessible where we need it, and I think it makes more sense here.