-
Notifications
You must be signed in to change notification settings - Fork 5.5k
tls: add functionality to override requested server name in the upstream cluster #4973
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
Changes from 9 commits
2fa0949
9701ce6
ab66f42
5fe6e3e
b6f32d7
6aff2ea
0b2a296
6c9128d
dedd511
27557de
b019a99
09af682
8ff3b25
cfbb753
566570f
173849b
41cf2c7
995d15e
c8afa18
c9ed4a7
bdce6bb
895dafb
088f2d8
88b3d31
e9c0a87
0fa1bd3
7109894
b8f0d3c
dddd8e3
2532513
588fa98
2f23141
b31e6f6
d84b467
d6583b9
505cce2
58efa36
7080014
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 |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| #include "common/network/transport_socket_options_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Network { | ||
| TransportSocketOptionsImpl::TransportSocketOptionsImpl(std::string override_server_name) { | ||
| if (!override_server_name.empty()) { | ||
| override_server_name_ = override_server_name; | ||
| } | ||
| } | ||
|
|
||
| void TransportSocketOptionsImpl::hashKey(std::vector<uint8_t>& key) const { | ||
| if (!override_server_name_.has_value()) { | ||
| return; | ||
| } | ||
|
|
||
| std::hash<std::string> hash_function; | ||
| key.push_back(hash_function(override_server_name_.value())); | ||
|
Member
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. I don't think this does what is expected. The hash function will return a
Contributor
Author
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. |
||
| } | ||
| } // namespace Network | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/network/transport_socket.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Network { | ||
|
|
||
| class TransportSocketOptionsImpl : public TransportSocketOptions { | ||
| public: | ||
| TransportSocketOptionsImpl(std::string override_server_name = ""); | ||
|
Member
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.
Contributor
Author
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. Fixed in 895dafb. |
||
| const absl::optional<std::string>& overrideServerName() const override { | ||
| return override_server_name_; | ||
| } | ||
| void hashKey(std::vector<uint8_t>& key) const override; | ||
|
|
||
| private: | ||
| absl::optional<std::string> override_server_name_; | ||
|
Member
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. Make this
Contributor
Author
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. Fixed in bdce6bb. |
||
| }; | ||
|
|
||
| } // namespace Network | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
|
|
||
| #include "common/ssl/context_manager_impl.h" | ||
|
|
||
| #include "absl/types/optional.h" | ||
| #include "openssl/ssl.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
@@ -41,7 +42,8 @@ struct SslStats { | |
|
|
||
| class ContextImpl : public virtual Context { | ||
| public: | ||
| virtual bssl::UniquePtr<SSL> newSsl() const; | ||
| virtual bssl::UniquePtr<SSL> | ||
| newSsl(const absl::optional<std::string>& override_server_name) const; | ||
|
Member
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. Nit: making this argument an
Contributor
Author
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. Done in 505cce2 |
||
|
|
||
| /** | ||
| * Logs successful TLS handshake and updates stats. | ||
|
|
@@ -142,7 +144,8 @@ class ClientContextImpl : public ContextImpl, public ClientContext { | |
| ClientContextImpl(Stats::Scope& scope, const ClientContextConfig& config, | ||
| TimeSource& time_source); | ||
|
|
||
| bssl::UniquePtr<SSL> newSsl() const override; | ||
| bssl::UniquePtr<SSL> | ||
| newSsl(const absl::optional<std::string>& override_server_name) const override; | ||
|
|
||
| private: | ||
| const std::string server_name_indication_; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| #include "common/stream_info/forward_requested_server_name.h" | ||
|
Member
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. (cont from #4334 (comment)) I don't think this should be here and named like this. Name it "upstream_server_name" or "upstream_transport_socket_options" (if you put the class here).
The consumer of this is tcp_proxy, or you should make this into network namespace, as stream_info is for both L4/L7, while this is specific to transport socket / connection.
"forward" (i.e. propagating downstream server name to upstream) is decided by the filter setting this, this filter state doesn't imply that. Another filter can set this to arbitrary value. The filter is not related directly to the TCP proxy. The filter specifies the intention that the SNI must be forwarded. Other filters, such as TCP proxy, can extract that information and forward the SNI.
Yes, this is right, now it is a struct named TransportSocketOptions.
Contributor
Author
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. @lizan So, making that struct into the network namespace, and calling it Should I make this I am OK with both solutions.
Member
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. Yeah I think upstream_server_name should be OK, then we can keep every filter state small. |
||
|
|
||
| namespace Envoy { | ||
| namespace StreamInfo { | ||
|
|
||
| const std::string ForwardRequestedServerName::Key = | ||
| "envoy.stream_info.forward_requested_server_name"; | ||
|
|
||
| } // namespace StreamInfo | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/stream_info/filter_state.h" | ||
|
|
||
| #include "absl/strings/string_view.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace StreamInfo { | ||
|
|
||
| /** | ||
| * Original Requested Server Name | ||
| */ | ||
| class ForwardRequestedServerName : public FilterState::Object { | ||
| public: | ||
| ForwardRequestedServerName(absl::string_view server_name) : server_name_(server_name) {} | ||
| const std::string& value() const { return server_name_; } | ||
| static const std::string Key; | ||
|
|
||
| private: | ||
| const std::string server_name_; | ||
| }; | ||
|
|
||
| } // namespace StreamInfo | ||
| } // 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.
comment for each method
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.
Done.
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.
Rename to
serverNameOverrideor just simplyserverName; otherwise, it sounds like a mutating method vs. an option.