access logging: pass downstream TLS information to access logs by snowp · Pull Request #6144 · envoyproxy/envoy

snowp · 2019-03-01T15:48:01Z

Passes along the Ssl::ConnectionInfo (renamed from Ssl::Connection)
to StreamInfo, allowing it to be read in access loggers. Updates both
the formatter and gRPC access logger with uri san/subject details,
and adds the SNI to the gRPC access log.

Signed-off-by: Snow Pettersen snowp@squareup.com

Risk Level: Low
Testing: Unit tests
Docs Changes: Added description for new access log formats
Release Notes: n/a
#4926

Passes along the Ssl::ConnectionInfo to StreamInfo, allowing it to be read in access loggers. Updates both the formatter and gRPC access logger with uri san/subject details, and wires up the SNI to the gRPC access log. Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp · 2019-03-01T15:49:28Z

This only exposes the SSL information that I found readily available, but it should be easy to expose whatever bits anyone's interested now by adding getters to Ssl::ConnectionInfo + reading it in the
access logs.

snowp · 2019-03-01T20:52:26Z

/retest

repokitteh-read-only · 2019-03-01T20:52:28Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #6144 (comment) was created by @snowp.

see: more, trace.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

dio · 2019-03-01T21:08:15Z

source/common/access_log/access_log_formatter.cc


+namespace {
+
+// Helper that handles the case when the ConectionInfo is missiring or if the desired value is


nit: s/missiring/missing

lizan · 2019-03-01T21:10:36Z

api/envoy/data/accesslog/v2/accesslog.proto

+
+  message CertificateProperties {
+    // The URI in the SAN field of the certificate.
+    string uri_san = 1;


this is not quite reflect what is in X.509, perhaps something like:

message SubjectAltName { oneof { string uri; string dns; } } repeated SubjectAltName subject_alt_names;

@PiotrSikora?

Yeah, seems like SAN should be a repeated.

Sorry. I did not see this earlier. We already have CertificateDetails message which does what @lizan mentioned above. Probably there is a chance to reuse here

envoy/api/envoy/admin/v2alpha/certs.proto

Line 30 in 4a5f858

message CertificateDetails {

lizan · 2019-03-01T21:11:22Z

include/envoy/network/connection.h

   * @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;
+  virtual const Ssl::ConnectionInfo* ssl() const PURE;


Is it possible to remove this interface from Connection and move into StreamInfo? If the change will be too heavy just leave a TODO.

I'll add a TODO, it seems to require changing a bunch of functions that pass the connection around

lizan · 2019-03-01T21:12:34Z

source/common/stream_info/stream_info_impl.h

  }

+  void setDownstreamSslConnection(const Ssl::ConnectionInfo* connection_info) override {
+    downstream_ssl_info_ = connection_info;


I'm slightly worried that StreamInfoImpl might outlive Ssl::ConnectionInfo, will that be the case?

From what I can tell it seems like Ssl::ConnectionInfo/Network::Connection comes from the FilterManagerImpl which should outlive the Filters that the StreamInfoImpl belongs to. Do you think it would be useful to expose the ConnectionInfo as a shared ptr to avoid potential lifetime issues in the future?

No I think it is fine to leave as is.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

dio

Thanks @snowp, Looks good for me.

dio · 2019-03-01T21:11:02Z

include/envoy/ssl/connection.h

 * Base connection interface for all SSL connections.
 */
-class Connection {
+class ConnectionInfo {


dio · 2019-03-01T21:13:07Z

source/common/access_log/access_log_formatter.cc

+
+// Helper that handles the case when the ConectionInfo is missiring or if the desired value is
+// empty.
+StreamInfoFormatter::FieldExtractor sslConnectionStringInfoExtractor(


Not sure if sslConnectionInfoStringExtractor sounds better.

htuch

LG overall, CI seems b0rk3d.

htuch · 2019-03-04T03:14:39Z

source/common/access_log/access_log_formatter.cc

+// empty.
+StreamInfoFormatter::FieldExtractor sslConnectionStringInfoExtractor(
+    std::function<std::string(const Ssl::ConnectionInfo& connection_info)> string_extractor) {
+  return [=](const StreamInfo::StreamInfo& stream_info) {


Nit: prefer explicit capture.

htuch · 2019-03-04T03:15:02Z

source/common/access_log/access_log_formatter.cc

+
+// Helper that handles the case when the ConectionInfo is missiring or if the desired value is
+// empty.
+StreamInfoFormatter::FieldExtractor sslConnectionStringInfoExtractor(


Signed-off-by: Snow Pettersen <snowp@squareup.com>

lizan · 2019-03-07T06:26:32Z

docs/root/configuration/access_log.rst

  TCP
    String value set on ssl connection socket for Server Name Indication (SNI)

+%DOWNSTREAM_LOCAL_URI_SAN%


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 accesslog.proto.

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.

htuch · 2019-03-07T17:59:33Z

source/common/access_log/access_log_formatter.cc

+  } else if (field_name == "DOWNSTREAM_PEER_URI_SAN") {
+    field_extractor_ =
+        sslConnectionInfoStringExtractor([](const Ssl::ConnectionInfo& connection_info) {
+          return connection_info.uriSanPeerCertificate();


When I look at how SslSocket::uriSanPeerCertificate() implemented today, it seems that it should really be returning all SAN. Should we fix this, so that we create template patterns that are stable and forward compatible? CC @PiotrSikora

Sure I don't mind fixing that. I'll just have all current usage just use the first SAN from the new function

htuch · 2019-03-07T18:00:22Z

api/envoy/data/accesslog/v2/accesslog.proto

+    message SubjectAltName {
+      oneof san {
+        string uri = 1;
+        string dns = 2;


Is this used?

no, i'll remove

A not implemented annotation is also fine.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

htuch · 2019-03-12T05:15:03Z

@snowp need master merge.

htuch

Looks good, just one question (which is somewhat orthogonal to what you're doing here anyway..).

source/common/http/conn_manager_utility.cc

Signed-off-by: Snow Pettersen <snowp@squareup.com>

htuch

LGTM, needs master merge. Do we want release notes?

test/common/http/conn_manager_impl_test.cc

Signed-off-by: Snow Pettersen <snowp@squareup.com>

htuch

Great, thanks @snowp!

snowp requested a review from htuch as a code owner March 1, 2019 15:48

alyssawilk assigned dio and htuch Mar 1, 2019

kick ci

b37d9af

Signed-off-by: Snow Pettersen <snowp@squareup.com>

lizan self-assigned this Mar 1, 2019

dio reviewed Mar 1, 2019

View reviewed changes

lizan reviewed Mar 1, 2019

View reviewed changes

fix TestStreamInfo + typos

977476e

Signed-off-by: Snow Pettersen <snowp@squareup.com>

dio previously approved these changes Mar 1, 2019

View reviewed changes

htuch reviewed Mar 4, 2019

View reviewed changes

lizan mentioned this pull request Mar 5, 2019

ssl: expose upstream TLS client context config #6178

Closed

Snow Pettersen added 2 commits March 5, 2019 23:27

make sans a repeated field

000459d

Signed-off-by: Snow Pettersen <snowp@squareup.com>

add todo, rename functions and fix build

4d03e5f

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp dismissed dio’s stale review via 4d03e5f March 6, 2019 04:54

Snow Pettersen added 2 commits March 6, 2019 01:28

format

904145e

Signed-off-by: Snow Pettersen <snowp@squareup.com>

Merge remote-tracking branch 'envoy/master' into ssl-stream

a621373

Signed-off-by: Snow Pettersen <snowp@squareup.com>

lizan reviewed Mar 7, 2019

View reviewed changes

htuch reviewed Mar 7, 2019

View reviewed changes

htuch added the waiting:any label Mar 7, 2019

repokitteh-read-only bot removed the waiting:any label Mar 11, 2019

htuch added the waiting:any label Mar 11, 2019

mark dns san as not implemented, make uri san helper return full list

d2690e7

Signed-off-by: Snow Pettersen <snowp@squareup.com>

repokitteh-read-only bot removed the waiting:any label Mar 11, 2019

Snow Pettersen added 2 commits March 11, 2019 15:39

fix rbac filter

9ffc227

Signed-off-by: Snow Pettersen <snowp@squareup.com>

fix rbac network filter

ac8e78d

Signed-off-by: Snow Pettersen <snowp@squareup.com>

Snow Pettersen added 3 commits March 11, 2019 16:21

fix listener_manager_impl_test

25b213f

Signed-off-by: Snow Pettersen <snowp@squareup.com>

fix conn_manager_utility_test

06b07d2

Signed-off-by: Snow Pettersen <snowp@squareup.com>

fix tcp_proxy tests

016c27c

Signed-off-by: Snow Pettersen <snowp@squareup.com>

htuch reviewed Mar 12, 2019

View reviewed changes

source/common/http/conn_manager_utility.cc Show resolved Hide resolved

source/common/http/conn_manager_utility.cc Outdated Show resolved Hide resolved

htuch added the waiting label Mar 12, 2019

fix variable name

16f1921

Signed-off-by: Snow Pettersen <snowp@squareup.com>

repokitteh-read-only bot removed the waiting label Mar 13, 2019

fix build failure

ab2c1f4

Signed-off-by: Snow Pettersen <snowp@squareup.com>

htuch suggested changes Mar 15, 2019

View reviewed changes

test/common/http/conn_manager_impl_test.cc Show resolved Hide resolved

htuch added the waiting label Mar 15, 2019

Snow Pettersen added 2 commits March 18, 2019 14:34

Merge remote-tracking branch 'envoy/master' into ssl-stream

17607f3

Signed-off-by: Snow Pettersen <snowp@squareup.com>

add release note

3185e3f

Signed-off-by: Snow Pettersen <snowp@squareup.com>

repokitteh-read-only bot removed the waiting label Mar 18, 2019

htuch approved these changes Mar 19, 2019

View reviewed changes

htuch merged commit 65c05ca into envoyproxy:master Mar 19, 2019

yangminzhu mentioned this pull request Apr 18, 2019

Update envoy sha and fix bulid break istio/proxy#2179

Merged

mikegrass mentioned this pull request May 10, 2019

Add Lua APIs to access downstream TLS session info #3296

Closed


		namespace {

		// Helper that handles the case when the ConectionInfo is missiring or if the desired value is

Conversation

snowp commented Mar 1, 2019

Uh oh!

snowp commented Mar 1, 2019

Uh oh!

snowp commented Mar 1, 2019

Uh oh!

repokitteh-read-only bot commented Mar 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Mar 12, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants