Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
1 change: 1 addition & 0 deletions envoy/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ envoy_cc_library(
deps = [
":address_interface",
":io_handle_interface",
"//envoy/ssl:connection_interface",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
],
)
Expand Down
2 changes: 1 addition & 1 deletion envoy/network/connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ class Connection : public Event::DeferredDeletable,
* @return the address provider backing this connection.
*/
virtual const SocketAddressProvider& addressProvider() const PURE;
virtual SocketAddressProviderSharedPtr addressProviderSharedPtr() const PURE;
virtual SocketAddressSetterSharedPtr addressProviderSharedPtr() const PURE;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: I have to change this due to allow the StreamInfo to set the upstream SSL connection into the AddressProvider.


/**
* Credentials of the peer of a socket as decided by SO_PEERCRED.
Expand Down
27 changes: 26 additions & 1 deletion envoy/network/socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "envoy/config/core/v3/base.pb.h"
#include "envoy/network/address.h"
#include "envoy/network/io_handle.h"
#include "envoy/ssl/connection.h"

#include "absl/strings/string_view.h"
#include "absl/types/optional.h"
Expand Down Expand Up @@ -92,6 +93,18 @@ class SocketAddressProvider {
* @param indent_level the level of indentation.
*/
virtual void dumpState(std::ostream& os, int indent_level) const PURE;

/**
* @return the upstream SSL connection. This will be nullptr if the upstream
* connection does not use SSL.
*/
virtual Ssl::ConnectionInfoConstSharedPtr upstreamSslConnection() const PURE;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this interface should only have the downstreamSslConnection(). All the other fields here are specific to the downstream connection, so including the upstream connection seems misleading.

@soulxu soulxu Jul 22, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, I feel strange too. Although we can keep this in the StreamInfo also, but it still feel strange, since the StreamInfo is only mean to stream for one direction(upstream, or downstream). Also can see the downstream StreamInfo's upstream SSL info is copied from upstream StreamInfo. For example:

getStreamInfo().setUpstreamSslConnection(ssl_info);

So in the end, the upstream SSL info is still mixed with downstream SSL info into downstream StreamInfo. Based on my guess the reason is the filter need the upstream StreamInfo, but other than tcpProxy or HttpRouter filter can't get the upstream StreamInfo, since the upstream StreamInfo is created the last filter (tcpProxy/http router filter).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The streaminfo is a bit odd, and has a mix of upstream/downstream. But SocketAddressProvider is only supposed to have information that should be copied to each request on the connection. The upstream ssl info should not be copied between requests, so I don't think it belongs here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

thanks, got it, let me revert the change to upstream SSL info.


/**
* @return the downstream SSL connection. This will be nullptr if the downstream
* connection does not use SSL.
*/
virtual Ssl::ConnectionInfoConstSharedPtr downstreamSslConnection() const PURE;
Comment thread
soulxu marked this conversation as resolved.
Outdated
};

class SocketAddressSetter : public SocketAddressProvider {
Expand Down Expand Up @@ -131,6 +144,18 @@ class SocketAddressSetter : public SocketAddressProvider {
* @param id Connection ID of the downstream connection.
**/
virtual void setConnectionID(uint64_t id) PURE;

/**
* @param connection_info sets the downstream ssl connection.
*/
virtual void
setDownstreamSslConnection(const Ssl::ConnectionInfoConstSharedPtr& ssl_connection_info) PURE;

/**
* @param connection_info sets the upstream ssl connection.
*/
virtual void
setUpstreamSslConnection(const Ssl::ConnectionInfoConstSharedPtr& ssl_connection_info) PURE;
};

using SocketAddressSetterSharedPtr = std::shared_ptr<SocketAddressSetter>;
Expand All @@ -153,7 +178,7 @@ class Socket {
*/
virtual SocketAddressSetter& addressProvider() PURE;
virtual const SocketAddressProvider& addressProvider() const PURE;
virtual SocketAddressProviderSharedPtr addressProviderSharedPtr() const PURE;
virtual SocketAddressSetterSharedPtr addressProviderSharedPtr() const PURE;
Comment thread
soulxu marked this conversation as resolved.
Outdated

/**
* @return IoHandle for the underlying connection
Expand Down
18 changes: 0 additions & 18 deletions envoy/stream_info/stream_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -467,30 +467,12 @@ class StreamInfo {
*/
virtual const Network::SocketAddressProvider& downstreamAddressProvider() const PURE;

/**
* @param connection_info sets the downstream ssl connection.
*/
virtual void
setDownstreamSslConnection(const Ssl::ConnectionInfoConstSharedPtr& ssl_connection_info) PURE;

/**
* @return the downstream SSL connection. This will be nullptr if the downstream
* connection does not use SSL.
*/
virtual Ssl::ConnectionInfoConstSharedPtr downstreamSslConnection() const PURE;

/**
* @param connection_info sets the upstream ssl connection.
*/
virtual void
setUpstreamSslConnection(const Ssl::ConnectionInfoConstSharedPtr& ssl_connection_info) PURE;

@soulxu soulxu Jul 14, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was kept to avoid change the 'downstreamSslConnection()' interface as non const. I feel change the 'downstreamSslConnection' isn't right direction.

Also this is why I can change the place to set the upstream SSL connection

stream_info_.setUpstreamSslConnection(info.downstreamSslConnection());


/**
* @return the upstream SSL connection. This will be nullptr if the upstream
* connection does not use SSL.
*/
virtual Ssl::ConnectionInfoConstSharedPtr upstreamSslConnection() const PURE;

/**
* @return const Router::RouteEntry* Get the route entry selected for this request. Note: this
* will be nullptr if no route was selected.
Expand Down
16 changes: 10 additions & 6 deletions source/common/formatter/substitution_formatter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -655,11 +655,12 @@ class StreamInfoSslConnectionInfoFieldExtractor : public StreamInfoFormatter::Fi
StreamInfoSslConnectionInfoFieldExtractor(FieldExtractor f) : field_extractor_(f) {}

absl::optional<std::string> extract(const StreamInfo::StreamInfo& stream_info) const override {
if (stream_info.downstreamSslConnection() == nullptr) {
if (stream_info.downstreamAddressProvider().downstreamSslConnection() == nullptr) {
return absl::nullopt;
}

const auto value = field_extractor_(*stream_info.downstreamSslConnection());
const auto value =
field_extractor_(*stream_info.downstreamAddressProvider().downstreamSslConnection());
if (value && value->empty()) {
return absl::nullopt;
}
Expand All @@ -668,11 +669,12 @@ class StreamInfoSslConnectionInfoFieldExtractor : public StreamInfoFormatter::Fi
}

ProtobufWkt::Value extractValue(const StreamInfo::StreamInfo& stream_info) const override {
if (stream_info.downstreamSslConnection() == nullptr) {
if (stream_info.downstreamAddressProvider().downstreamSslConnection() == nullptr) {
return unspecifiedValue();
}

const auto value = field_extractor_(*stream_info.downstreamSslConnection());
const auto value =
field_extractor_(*stream_info.downstreamAddressProvider().downstreamSslConnection());
if (value && value->empty()) {
return unspecifiedValue();
}
Expand Down Expand Up @@ -1335,7 +1337,8 @@ DownstreamPeerCertVStartFormatter::DownstreamPeerCertVStartFormatter(const std::
parseFormat(token, sizeof("DOWNSTREAM_PEER_CERT_V_START(") - 1),
std::make_unique<SystemTimeFormatter::TimeFieldExtractor>(
[](const StreamInfo::StreamInfo& stream_info) -> absl::optional<SystemTime> {
const auto connection_info = stream_info.downstreamSslConnection();
const auto connection_info =
stream_info.downstreamAddressProvider().downstreamSslConnection();
return connection_info != nullptr ? connection_info->validFromPeerCertificate()
: absl::optional<SystemTime>();
})) {}
Expand All @@ -1347,7 +1350,8 @@ DownstreamPeerCertVEndFormatter::DownstreamPeerCertVEndFormatter(const std::stri
parseFormat(token, sizeof("DOWNSTREAM_PEER_CERT_V_END(") - 1),
std::make_unique<SystemTimeFormatter::TimeFieldExtractor>(
[](const StreamInfo::StreamInfo& stream_info) -> absl::optional<SystemTime> {
const auto connection_info = stream_info.downstreamSslConnection();
const auto connection_info =
stream_info.downstreamAddressProvider().downstreamSslConnection();
return connection_info != nullptr ? connection_info->expirationPeerCertificate()
: absl::optional<SystemTime>();
})) {}
Expand Down
1 change: 0 additions & 1 deletion source/common/http/codec_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ RequestEncoder& CodecClient::newStream(ResponseDecoder& response_decoder) {
void CodecClient::onEvent(Network::ConnectionEvent event) {
if (event == Network::ConnectionEvent::Connected) {
ENVOY_CONN_LOG(debug, "connected", *connection_);
connection_->streamInfo().setDownstreamSslConnection(connection_->ssl());
connected_ = true;
}

Expand Down
3 changes: 0 additions & 3 deletions source/common/http/conn_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -648,9 +648,6 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect
connection_manager_.stats_.named_.downstream_rq_http1_total_.inc();
}

filter_manager_.streamInfo().setDownstreamSslConnection(
connection_manager_.read_callbacks_->connection().ssl());

if (connection_manager_.config_.streamIdleTimeout().count()) {
idle_timeout_ms_ = connection_manager_.config_.streamIdleTimeout();
stream_idle_timer_ =
Expand Down
6 changes: 6 additions & 0 deletions source/common/http/filter_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,12 @@ class OverridableRemoteSocketAddressSetterStreamInfo : public StreamInfo::Stream
absl::optional<uint64_t> connectionID() const override {
return StreamInfoImpl::downstreamAddressProvider().connectionID();
}
Ssl::ConnectionInfoConstSharedPtr downstreamSslConnection() const override {
return StreamInfoImpl::downstreamAddressProvider().downstreamSslConnection();
}
Ssl::ConnectionInfoConstSharedPtr upstreamSslConnection() const override {
return StreamInfoImpl::downstreamAddressProvider().upstreamSslConnection();
}
void dumpState(std::ostream& os, int indent_level) const override {
StreamInfoImpl::dumpState(os, indent_level);

Expand Down
1 change: 1 addition & 0 deletions source/common/network/connection_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ ConnectionImpl::ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPt
// TODO(soulxu): generate the connection id inside the addressProvider directly,
// then we don't need a setter or any of the optional stuff.
socket_->addressProvider().setConnectionID(id());
socket_->addressProvider().setDownstreamSslConnection(transport_socket_->ssl());
}

ConnectionImpl::~ConnectionImpl() {
Expand Down
2 changes: 1 addition & 1 deletion source/common/network/connection_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class ConnectionImpl : public ConnectionImplBase, public TransportSocketCallback
const SocketAddressProvider& addressProvider() const override {
return socket_->addressProvider();
}
SocketAddressProviderSharedPtr addressProviderSharedPtr() const override {
SocketAddressSetterSharedPtr addressProviderSharedPtr() const override {
return socket_->addressProviderSharedPtr();
}
absl::optional<UnixDomainSocketPeerCredentials> unixSocketPeerCredentials() const override;
Expand Down
18 changes: 17 additions & 1 deletion source/common/network/socket_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,20 @@ class SocketAddressSetterImpl : public SocketAddressSetter {
}
absl::optional<uint64_t> connectionID() const override { return connection_id_; }
void setConnectionID(uint64_t id) override { connection_id_ = id; }
Ssl::ConnectionInfoConstSharedPtr downstreamSslConnection() const override {
return downstream_ssl_info_;
}
void setDownstreamSslConnection(
const Ssl::ConnectionInfoConstSharedPtr& ssl_connection_info) override {
downstream_ssl_info_ = ssl_connection_info;
}
Ssl::ConnectionInfoConstSharedPtr upstreamSslConnection() const override {
return upstream_ssl_info_;
}
void
setUpstreamSslConnection(const Ssl::ConnectionInfoConstSharedPtr& ssl_connection_info) override {
upstream_ssl_info_ = ssl_connection_info;
}

private:
Address::InstanceConstSharedPtr local_address_;
Expand All @@ -59,6 +73,8 @@ class SocketAddressSetterImpl : public SocketAddressSetter {
Address::InstanceConstSharedPtr direct_remote_address_;
std::string server_name_;
absl::optional<uint64_t> connection_id_;
Ssl::ConnectionInfoConstSharedPtr downstream_ssl_info_;
Ssl::ConnectionInfoConstSharedPtr upstream_ssl_info_;
};

class SocketImpl : public virtual Socket {
Expand All @@ -69,7 +85,7 @@ class SocketImpl : public virtual Socket {
// Network::Socket
SocketAddressSetter& addressProvider() override { return *address_provider_; }
const SocketAddressProvider& addressProvider() const override { return *address_provider_; }
SocketAddressProviderSharedPtr addressProviderSharedPtr() const override {
SocketAddressSetterSharedPtr addressProviderSharedPtr() const override {
return address_provider_;
}
SocketPtr duplicate() override {
Expand Down
2 changes: 1 addition & 1 deletion source/common/quic/quic_filter_manager_connection_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase,
const Network::SocketAddressSetter& addressProvider() const override {
return network_connection_->connectionSocket()->addressProvider();
}
Network::SocketAddressProviderSharedPtr addressProviderSharedPtr() const override {
Network::SocketAddressSetterSharedPtr addressProviderSharedPtr() const override {
return network_connection_->connectionSocket()->addressProviderSharedPtr();
}
absl::optional<Network::Connection::UnixDomainSocketPeerCredentials>
Expand Down
12 changes: 8 additions & 4 deletions source/common/router/config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -502,14 +502,18 @@ bool RouteEntryImplBase::evaluateTlsContextMatch(const StreamInfo::StreamInfo& s
const TlsContextMatchCriteria& criteria = *tlsContextMatchCriteria();

if (criteria.presented().has_value()) {
const bool peer_presented = stream_info.downstreamSslConnection() &&
stream_info.downstreamSslConnection()->peerCertificatePresented();
const bool peer_presented = stream_info.downstreamAddressProvider().downstreamSslConnection() &&
stream_info.downstreamAddressProvider()
.downstreamSslConnection()
->peerCertificatePresented();
matches &= criteria.presented().value() == peer_presented;
}

if (criteria.validated().has_value()) {
const bool peer_validated = stream_info.downstreamSslConnection() &&
stream_info.downstreamSslConnection()->peerCertificateValidated();
const bool peer_validated = stream_info.downstreamAddressProvider().downstreamSslConnection() &&
stream_info.downstreamAddressProvider()
.downstreamSslConnection()
->peerCertificateValidated();
matches &= criteria.validated().value() == peer_validated;
}

Expand Down
4 changes: 2 additions & 2 deletions source/common/router/header_formatter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -224,11 +224,11 @@ parseRequestHeader(absl::string_view param) {
StreamInfoHeaderFormatter::FieldExtractor sslConnectionInfoStringHeaderExtractor(
std::function<std::string(const Ssl::ConnectionInfo& connection_info)> string_extractor) {
return [string_extractor](const StreamInfo::StreamInfo& stream_info) {
if (stream_info.downstreamSslConnection() == nullptr) {
if (stream_info.downstreamAddressProvider().downstreamSslConnection() == nullptr) {
return std::string();
}

return string_extractor(*stream_info.downstreamSslConnection());
return string_extractor(*stream_info.downstreamAddressProvider().downstreamSslConnection());
};
}

Expand Down
7 changes: 4 additions & 3 deletions source/common/router/router.cc
Original file line number Diff line number Diff line change
Expand Up @@ -604,9 +604,10 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,

route_entry_->finalizeRequestHeaders(headers, callbacks_->streamInfo(),
!config_.suppress_envoy_headers_);
FilterUtility::setUpstreamScheme(headers,
callbacks_->streamInfo().downstreamSslConnection() != nullptr,
host->transportSocketFactory().implementsSecureTransport());
FilterUtility::setUpstreamScheme(
headers,
callbacks_->streamInfo().downstreamAddressProvider().downstreamSslConnection() != nullptr,
host->transportSocketFactory().implementsSecureTransport());

// Ensure an http transport scheme is selected before continuing with decoding.
ASSERT(headers.Scheme());
Expand Down
5 changes: 3 additions & 2 deletions source/common/router/upstream_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -412,8 +412,9 @@ void UpstreamRequest::onPoolReady(
stream_info_.setUpstreamLocalAddress(upstream_local_address);
parent_.callbacks()->streamInfo().setUpstreamLocalAddress(upstream_local_address);

stream_info_.setUpstreamSslConnection(info.downstreamSslConnection());
parent_.callbacks()->streamInfo().setUpstreamSslConnection(info.downstreamSslConnection());
stream_info_.setUpstreamSslConnection(info.downstreamAddressProvider().downstreamSslConnection());
parent_.callbacks()->streamInfo().setUpstreamSslConnection(
info.downstreamAddressProvider().downstreamSslConnection());

if (parent_.downstreamEndStream()) {
setupPerTryTimeout();
Expand Down
30 changes: 8 additions & 22 deletions source/common/stream_info/stream_info_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,18 @@ const ReplacementMap& emptySpaceReplacement() {

struct StreamInfoImpl : public StreamInfo {
StreamInfoImpl(TimeSource& time_source,
const Network::SocketAddressProviderSharedPtr& downstream_address_provider,
const Network::SocketAddressSetterSharedPtr& downstream_address_provider,
FilterState::LifeSpan life_span = FilterState::LifeSpan::FilterChain)
: StreamInfoImpl(absl::nullopt, time_source, downstream_address_provider,
std::make_shared<FilterStateImpl>(life_span)) {}

StreamInfoImpl(Http::Protocol protocol, TimeSource& time_source,
const Network::SocketAddressProviderSharedPtr& downstream_address_provider)
const Network::SocketAddressSetterSharedPtr& downstream_address_provider)
: StreamInfoImpl(protocol, time_source, downstream_address_provider,
std::make_shared<FilterStateImpl>(FilterState::LifeSpan::FilterChain)) {}

StreamInfoImpl(Http::Protocol protocol, TimeSource& time_source,
const Network::SocketAddressProviderSharedPtr& downstream_address_provider,
const Network::SocketAddressSetterSharedPtr& downstream_address_provider,
FilterStateSharedPtr parent_filter_state, FilterState::LifeSpan life_span)
: StreamInfoImpl(
protocol, time_source, downstream_address_provider,
Expand Down Expand Up @@ -197,21 +197,8 @@ struct StreamInfoImpl : public StreamInfo {
return *downstream_address_provider_;
}

void
setDownstreamSslConnection(const Ssl::ConnectionInfoConstSharedPtr& connection_info) override {
downstream_ssl_info_ = connection_info;
}

Ssl::ConnectionInfoConstSharedPtr downstreamSslConnection() const override {
return downstream_ssl_info_;
}

void setUpstreamSslConnection(const Ssl::ConnectionInfoConstSharedPtr& connection_info) override {
upstream_ssl_info_ = connection_info;
}

Ssl::ConnectionInfoConstSharedPtr upstreamSslConnection() const override {
return upstream_ssl_info_;
downstream_address_provider_->setUpstreamSslConnection(connection_info);
}

const Router::RouteEntry* routeEntry() const override { return route_entry_; }
Expand Down Expand Up @@ -303,14 +290,14 @@ struct StreamInfoImpl : public StreamInfo {
std::string route_name_;

private:
static Network::SocketAddressProviderSharedPtr emptyDownstreamAddressProvider() {
static Network::SocketAddressSetterSharedPtr emptyDownstreamAddressProvider() {
MUTABLE_CONSTRUCT_ON_FIRST_USE(
Network::SocketAddressProviderSharedPtr,
Network::SocketAddressSetterSharedPtr,
std::make_shared<Network::SocketAddressSetterImpl>(nullptr, nullptr));
}

StreamInfoImpl(absl::optional<Http::Protocol> protocol, TimeSource& time_source,
const Network::SocketAddressProviderSharedPtr& downstream_address_provider,
const Network::SocketAddressSetterSharedPtr& downstream_address_provider,
FilterStateSharedPtr filter_state)
: time_source_(time_source), start_time_(time_source.systemTime()),
start_time_monotonic_(time_source.monotonicTime()), protocol_(protocol),
Expand All @@ -323,8 +310,7 @@ struct StreamInfoImpl : public StreamInfo {
uint64_t bytes_received_{};
uint64_t bytes_sent_{};
Network::Address::InstanceConstSharedPtr upstream_local_address_;
const Network::SocketAddressProviderSharedPtr downstream_address_provider_;
Ssl::ConnectionInfoConstSharedPtr downstream_ssl_info_;
const Network::SocketAddressSetterSharedPtr downstream_address_provider_;
Ssl::ConnectionInfoConstSharedPtr upstream_ssl_info_;
std::string requested_server_name_;
const Http::RequestHeaderMap* request_headers_{};
Expand Down
1 change: 0 additions & 1 deletion source/common/tcp/conn_pool.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ void ActiveTcpClient::onEvent(Network::ConnectionEvent event) {
// This is also necessary for prefetch to be used with such protocols.
if (event == Network::ConnectionEvent::Connected) {
connection_->readDisable(true);
connection_->streamInfo().setDownstreamSslConnection(connection_->ssl());
}
Envoy::ConnectionPool::ActiveClient::onEvent(event);
if (callbacks_) {
Expand Down
Loading