-
Notifications
You must be signed in to change notification settings - Fork 5.3k
access logging: pass downstream TLS information to access logs #6144
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 all commits
3d29052
b37d9af
977476e
000459d
4d03e5f
904145e
a621373
d2690e7
9ffc227
ac8e78d
25b213f
06b07d2
016c27c
16f1921
ab2c1f4
17607f3
3185e3f
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 |
|---|---|---|
|
|
@@ -323,3 +323,28 @@ The following command operators are supported: | |
| TCP | ||
| String value set on ssl connection socket for Server Name Indication (SNI) | ||
|
|
||
| %DOWNSTREAM_LOCAL_URI_SAN% | ||
|
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. Not directly actionable in this PR perhaps, but need to think about how to structurally express those field in logging format and keep file access log feature parity with gRPC. We'll likely have upstream TLS set for this as well, and other properties which is defined in Some random idea would be making the proto representation as first class, and have some expression in file access log to represent extractor from there. |
||
| HTTP | ||
| The URIs present in the SAN of the local certificate used to establish the downstream TLS connection. | ||
| TCP | ||
| The URIs present in the SAN of the local certificate used to establish the downstream TLS connection. | ||
|
|
||
| %DOWNSTREAM_PEER_URI_SAN% | ||
| HTTP | ||
| The URIs present in the SAN of the peer certificate used to establish the downstream TLS connection. | ||
| TCP | ||
| The URIs present in the SAN of the peer certificate used to establish the downstream TLS connection. | ||
|
|
||
| %DOWNSTREAM_LOCAL_SUBJECT% | ||
| HTTP | ||
| The subject present in the local certificate used to establish the downstream TLS connection. | ||
| TCP | ||
| The subject present in the local certificate used to establish the downstream TLS connection. | ||
|
|
||
| %DOWNSTREAM_PEER_SUBJECT% | ||
| HTTP | ||
| The subject present in the peer certificate used to establish the downstream TLS connection. | ||
| TCP | ||
| The subject present in the peer certificate used to establish the downstream TLS connection. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -184,7 +184,8 @@ class Connection : public Event::DeferredDeletable, public FilterManager { | |
| /** | ||
| * @return the const SSL connection data if this is an SSL connection, or nullptr if it is not. | ||
| */ | ||
| virtual const Ssl::Connection* ssl() const PURE; | ||
| // TODO(snowp): Remove this in favor of StreamInfo::downstreamSslConnection. | ||
| virtual const Ssl::ConnectionInfo* ssl() const PURE; | ||
|
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. Is it possible to remove this interface from
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. I'll add a TODO, it seems to require changing a bunch of functions that pass the connection around |
||
|
|
||
| /** | ||
| * @return requested server name (e.g. SNI in TLS), if any. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,20 +14,20 @@ namespace Ssl { | |
| /** | ||
| * Base connection interface for all SSL connections. | ||
| */ | ||
| class Connection { | ||
| class ConnectionInfo { | ||
|
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. +1 |
||
| public: | ||
| virtual ~Connection() {} | ||
| virtual ~ConnectionInfo() {} | ||
|
|
||
| /** | ||
| * @return bool whether the peer certificate is presented. | ||
| **/ | ||
| virtual bool peerCertificatePresented() const PURE; | ||
|
|
||
| /** | ||
| * @return std::string the URI in the SAN field of the local certificate. Returns "" if there is | ||
| * @return std::string the URIs in the SAN field of the local certificate. Returns {} if there is | ||
| * no local certificate, or no SAN field, or no URI. | ||
| **/ | ||
| virtual std::string uriSanLocalCertificate() const PURE; | ||
| virtual std::vector<std::string> uriSanLocalCertificate() const PURE; | ||
|
|
||
| /** | ||
| * @return std::string the subject field of the local certificate in RFC 2253 format. Returns "" | ||
|
|
@@ -54,10 +54,10 @@ class Connection { | |
| virtual std::string subjectPeerCertificate() const PURE; | ||
|
|
||
| /** | ||
| * @return std::string the URI in the SAN field of the peer certificate. Returns "" if there is no | ||
| * peer certificate, or no SAN field, or no URI. | ||
| * @return std::string the URIs in the SAN field of the peer certificate. Returns {} if there is | ||
| *no peer certificate, or no SAN field, or no URI. | ||
| **/ | ||
| virtual std::string uriSanPeerCertificate() const PURE; | ||
| virtual std::vector<std::string> uriSanPeerCertificate() const PURE; | ||
|
|
||
| /** | ||
| * @return std::string the URL-encoded PEM-encoded representation of the peer certificate. Returns | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,6 +160,14 @@ struct StreamInfoImpl : public StreamInfo { | |
| return downstream_remote_address_; | ||
| } | ||
|
|
||
| void setDownstreamSslConnection(const Ssl::ConnectionInfo* connection_info) override { | ||
| downstream_ssl_info_ = connection_info; | ||
|
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'm slightly worried that StreamInfoImpl might outlive Ssl::ConnectionInfo, will that be the case?
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. From what I can tell it seems like
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. No I think it is fine to leave as is. |
||
| } | ||
|
|
||
| const Ssl::ConnectionInfo* downstreamSslConnection() const override { | ||
| return downstream_ssl_info_; | ||
| } | ||
|
|
||
| const Router::RouteEntry* routeEntry() const override { return route_entry_; } | ||
|
|
||
| envoy::api::v2::core::Metadata& dynamicMetadata() override { return metadata_; }; | ||
|
|
@@ -211,6 +219,7 @@ struct StreamInfoImpl : public StreamInfo { | |
| Network::Address::InstanceConstSharedPtr downstream_local_address_; | ||
| Network::Address::InstanceConstSharedPtr downstream_direct_remote_address_; | ||
| Network::Address::InstanceConstSharedPtr downstream_remote_address_; | ||
| const Ssl::ConnectionInfo* downstream_ssl_info_; | ||
| std::string requested_server_name_; | ||
| UpstreamTiming upstream_timing_; | ||
| std::string upstream_transport_failure_reason_; | ||
|
|
||
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.
Is this used?
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.
no, i'll remove
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.
A not implemented annotation is also fine.