From a7cbf3db129da3d6a969c1e58010b3e15a7dddbb Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Fri, 16 Aug 2019 06:39:35 -0700 Subject: [PATCH 1/5] add dns san support for ext authz Signed-off-by: Rama Chavali --- .../common/ext_authz/check_request_utils.cc | 14 ++++++-- .../ext_authz/check_request_utils_test.cc | 33 ++++++++++++++++++- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/common/ext_authz/check_request_utils.cc b/source/extensions/filters/common/ext_authz/check_request_utils.cc index d243e04c5978..1a0f2d0cff8a 100644 --- a/source/extensions/filters/common/ext_authz/check_request_utils.cc +++ b/source/extensions/filters/common/ext_authz/check_request_utils.cc @@ -45,14 +45,24 @@ void CheckRequestUtils::setAttrContextPeer(envoy::service::auth::v2::AttributeCo if (local) { const auto uriSans = ssl->uriSanLocalCertificate(); if (uriSans.empty()) { - peer.set_principal(ssl->subjectLocalCertificate()); + const auto dnsSans = ssl->dnsSansLocalCertificate(); + if (dnsSans.empty()) { + peer.set_principal(ssl->subjectLocalCertificate()); + } else { + peer.set_principal(dnsSans[0]); + } } else { peer.set_principal(uriSans[0]); } } else { const auto uriSans = ssl->uriSanPeerCertificate(); if (uriSans.empty()) { - peer.set_principal(ssl->subjectPeerCertificate()); + const auto dnsSans = ssl->dnsSansPeerCertificate(); + if (dnsSans.empty()) { + peer.set_principal(ssl->subjectPeerCertificate()); + } else { + peer.set_principal(dnsSans[0]); + } } else { peer.set_principal(uriSans[0]); } diff --git a/test/extensions/filters/common/ext_authz/check_request_utils_test.cc b/test/extensions/filters/common/ext_authz/check_request_utils_test.cc index 7567a9a95023..cb40116c0491 100644 --- a/test/extensions/filters/common/ext_authz/check_request_utils_test.cc +++ b/test/extensions/filters/common/ext_authz/check_request_utils_test.cc @@ -127,7 +127,7 @@ TEST_F(CheckRequestUtilsTest, BasicHttpWithFullBody) { // Verify that createHttpCheck extract the proper attributes from the http request into CheckRequest // proto object. -TEST_F(CheckRequestUtilsTest, CheckAttrContextPeer) { +TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerUriSans) { Http::TestHeaderMapImpl request_headers{{"x-envoy-downstream-service-cluster", "foo"}, {":path", "/bar"}}; envoy::service::auth::v2::CheckRequest request; @@ -155,6 +155,37 @@ TEST_F(CheckRequestUtilsTest, CheckAttrContextPeer) { EXPECT_EQ("value", request.attributes().context_extensions().at("key")); } +TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerDnsSans) { + Http::TestHeaderMapImpl request_headers{{"x-envoy-downstream-service-cluster", "foo"}, + {":path", "/bar"}}; + envoy::service::auth::v2::CheckRequest request; + EXPECT_CALL(callbacks_, connection()).WillRepeatedly(Return(&connection_)); + EXPECT_CALL(connection_, remoteAddress()).WillRepeatedly(ReturnRef(addr_)); + EXPECT_CALL(connection_, localAddress()).WillRepeatedly(ReturnRef(addr_)); + EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); + EXPECT_CALL(callbacks_, streamId()).WillRepeatedly(Return(0)); + EXPECT_CALL(callbacks_, streamInfo()).WillRepeatedly(ReturnRef(req_info_)); + EXPECT_CALL(callbacks_, decodingBuffer()).Times(1); + EXPECT_CALL(req_info_, protocol()).WillRepeatedly(ReturnPointee(&protocol_)); + EXPECT_CALL(ssl_, uriSanPeerCertificate()).WillOnce(Return(std::vector{})); + EXPECT_CALL(ssl_, dnsSansPeerCertificate()).WillOnce(Return(std::vector{"source"})); + + EXPECT_CALL(ssl_, uriSanLocalCertificate()).WillOnce(Return(std::vector{})); + EXPECT_CALL(ssl_, dnsSansLocalCertificate()) + .WillOnce(Return(std::vector{"destination"})); + + Protobuf::Map context_extensions; + context_extensions["key"] = "value"; + + CheckRequestUtils::createHttpCheck(&callbacks_, request_headers, std::move(context_extensions), + request, false); + + EXPECT_EQ("source", request.attributes().source().principal()); + EXPECT_EQ("destination", request.attributes().destination().principal()); + EXPECT_EQ("foo", request.attributes().source().service()); + EXPECT_EQ("value", request.attributes().context_extensions().at("key")); +} + } // namespace } // namespace ExtAuthz } // namespace Common From 3da1534e20b06cf2db74a879a323f7566ff381cd Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Wed, 21 Aug 2019 09:11:58 +0530 Subject: [PATCH 2/5] update comment Signed-off-by: Rama Chavali --- .../filters/common/ext_authz/check_request_utils.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/source/extensions/filters/common/ext_authz/check_request_utils.cc b/source/extensions/filters/common/ext_authz/check_request_utils.cc index 9ac0634389d3..1d968d62c16f 100644 --- a/source/extensions/filters/common/ext_authz/check_request_utils.cc +++ b/source/extensions/filters/common/ext_authz/check_request_utils.cc @@ -37,9 +37,7 @@ void CheckRequestUtils::setAttrContextPeer(envoy::service::auth::v2::AttributeCo Envoy::Network::Utility::addressToProtobufAddress(*connection.remoteAddress(), *addr); } - // Set the principal - // Preferably the SAN from the peer's cert or - // Subject from the peer's cert. + // Set the principal. Preferably the URI SAN, DNS SAN or Subject in that order from the peer's cert. Ssl::ConnectionInfo* ssl = const_cast(connection.ssl()); if (ssl != nullptr) { if (local) { From 98a9866813254e136a6d233d34329015ab41a242 Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Wed, 21 Aug 2019 09:17:41 +0530 Subject: [PATCH 3/5] proto doc and format Signed-off-by: Rama Chavali --- api/envoy/service/auth/v2/attribute_context.proto | 4 ++-- .../filters/common/ext_authz/check_request_utils.cc | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/api/envoy/service/auth/v2/attribute_context.proto b/api/envoy/service/auth/v2/attribute_context.proto index 822e361dd81b..fe5bee033909 100644 --- a/api/envoy/service/auth/v2/attribute_context.proto +++ b/api/envoy/service/auth/v2/attribute_context.proto @@ -50,8 +50,8 @@ message AttributeContext { // The authenticated identity of this peer. // For example, the identity associated with the workload such as a service account. // If an X.509 certificate is used to assert the identity this field should be sourced from - // `Subject` or `Subject Alternative Names`. The primary identity should be the principal. - // The principal format is issuer specific. + // `URI Subject Alternative Names`, `DNS Subject Alternate Names` or `Subject` in that order. + // The primary identity should be the principal. The principal format is issuer specific. // // Example: // * SPIFFE format is `spiffe://trust-domain/path` diff --git a/source/extensions/filters/common/ext_authz/check_request_utils.cc b/source/extensions/filters/common/ext_authz/check_request_utils.cc index 1d968d62c16f..a559b1e4a565 100644 --- a/source/extensions/filters/common/ext_authz/check_request_utils.cc +++ b/source/extensions/filters/common/ext_authz/check_request_utils.cc @@ -37,7 +37,8 @@ void CheckRequestUtils::setAttrContextPeer(envoy::service::auth::v2::AttributeCo Envoy::Network::Utility::addressToProtobufAddress(*connection.remoteAddress(), *addr); } - // Set the principal. Preferably the URI SAN, DNS SAN or Subject in that order from the peer's cert. + // Set the principal. Preferably the URI SAN, DNS SAN or Subject in that order from the peer's + // cert. Ssl::ConnectionInfo* ssl = const_cast(connection.ssl()); if (ssl != nullptr) { if (local) { From 7899e4ffc7f94c54c6617159860ffbca01329293 Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Wed, 21 Aug 2019 10:15:17 +0530 Subject: [PATCH 4/5] refactor tests Signed-off-by: Rama Chavali --- .../ext_authz/check_request_utils_test.cc | 111 +++++++++--------- 1 file changed, 56 insertions(+), 55 deletions(-) diff --git a/test/extensions/filters/common/ext_authz/check_request_utils_test.cc b/test/extensions/filters/common/ext_authz/check_request_utils_test.cc index c0bdc60f14f0..0be215a57f81 100644 --- a/test/extensions/filters/common/ext_authz/check_request_utils_test.cc +++ b/test/extensions/filters/common/ext_authz/check_request_utils_test.cc @@ -30,7 +30,7 @@ class CheckRequestUtilsTest : public testing::Test { buffer_ = CheckRequestUtilsTest::newTestBuffer(8192); }; - void ExpectBasicHttp() { + void expectBasicHttp() { EXPECT_CALL(callbacks_, connection()).Times(2).WillRepeatedly(Return(&connection_)); EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); @@ -41,6 +41,33 @@ class CheckRequestUtilsTest : public testing::Test { EXPECT_CALL(req_info_, protocol()).Times(2).WillRepeatedly(ReturnPointee(&protocol_)); } + void callHttpCheckAndValidateRequestAttributes() { + Http::TestHeaderMapImpl request_headers{{"x-envoy-downstream-service-cluster", "foo"}, + {":path", "/bar"}}; + envoy::service::auth::v2::CheckRequest request; + Protobuf::Map context_extensions; + context_extensions["key"] = "value"; + + envoy::api::v2::core::Metadata metadata_context; + auto metadata_val = MessageUtil::keyValueStruct("foo", "bar"); + (*metadata_context.mutable_filter_metadata())["meta.key"] = metadata_val; + + CheckRequestUtils::createHttpCheck(&callbacks_, request_headers, std::move(context_extensions), + std::move(metadata_context), request, false); + + EXPECT_EQ("source", request.attributes().source().principal()); + EXPECT_EQ("destination", request.attributes().destination().principal()); + EXPECT_EQ("foo", request.attributes().source().service()); + EXPECT_EQ("value", request.attributes().context_extensions().at("key")); + EXPECT_EQ("bar", request.attributes() + .metadata_context() + .filter_metadata() + .at("meta.key") + .fields() + .at("foo") + .string_value()); + } + static Buffer::InstancePtr newTestBuffer(uint64_t size) { auto buffer = std::make_unique(); while (buffer->length() < size) { @@ -84,7 +111,7 @@ TEST_F(CheckRequestUtilsTest, BasicHttp) { // A client supplied EnvoyAuthPartialBody header should be ignored. Http::TestHeaderMapImpl request_headers{{Http::Headers::get().EnvoyAuthPartialBody.get(), "1"}}; - ExpectBasicHttp(); + expectBasicHttp(); CheckRequestUtils::createHttpCheck(&callbacks_, request_headers, Protobuf::Map(), envoy::api::v2::core::Metadata(), request_, size); @@ -101,7 +128,7 @@ TEST_F(CheckRequestUtilsTest, BasicHttpWithPartialBody) { Http::HeaderMapImpl headers_; envoy::service::auth::v2::CheckRequest request_; - ExpectBasicHttp(); + expectBasicHttp(); CheckRequestUtils::createHttpCheck(&callbacks_, headers_, Protobuf::Map(), envoy::api::v2::core::Metadata(), request_, size); @@ -116,7 +143,7 @@ TEST_F(CheckRequestUtilsTest, BasicHttpWithFullBody) { Http::HeaderMapImpl headers_; envoy::service::auth::v2::CheckRequest request_; - ExpectBasicHttp(); + expectBasicHttp(); CheckRequestUtils::createHttpCheck(&callbacks_, headers_, Protobuf::Map(), envoy::api::v2::core::Metadata(), request_, buffer_->length()); @@ -127,59 +154,23 @@ TEST_F(CheckRequestUtilsTest, BasicHttpWithFullBody) { Http::Headers::get().EnvoyAuthPartialBody.get())); } -// Verify that createHttpCheck extract the proper attributes from the http request into CheckRequest -// proto object. +// Verify that createHttpCheck extract the attributes from the http request into CheckRequest +// proto object and Uri SAN is used as principal if present. TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerUriSans) { - Http::TestHeaderMapImpl request_headers{{"x-envoy-downstream-service-cluster", "foo"}, - {":path", "/bar"}}; - envoy::service::auth::v2::CheckRequest request; - EXPECT_CALL(callbacks_, connection()).WillRepeatedly(Return(&connection_)); - EXPECT_CALL(connection_, remoteAddress()).WillRepeatedly(ReturnRef(addr_)); - EXPECT_CALL(connection_, localAddress()).WillRepeatedly(ReturnRef(addr_)); - EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); - EXPECT_CALL(callbacks_, streamId()).WillRepeatedly(Return(0)); - EXPECT_CALL(callbacks_, streamInfo()).WillRepeatedly(ReturnRef(req_info_)); - EXPECT_CALL(callbacks_, decodingBuffer()).Times(1); - EXPECT_CALL(req_info_, protocol()).WillRepeatedly(ReturnPointee(&protocol_)); + expectBasicHttp(); + EXPECT_CALL(ssl_, uriSanPeerCertificate()).WillOnce(Return(std::vector{"source"})); EXPECT_CALL(ssl_, uriSanLocalCertificate()) .WillOnce(Return(std::vector{"destination"})); - Protobuf::Map context_extensions; - context_extensions["key"] = "value"; - - envoy::api::v2::core::Metadata metadata_context; - auto metadata_val = MessageUtil::keyValueStruct("foo", "bar"); - (*metadata_context.mutable_filter_metadata())["meta.key"] = metadata_val; - - CheckRequestUtils::createHttpCheck(&callbacks_, request_headers, std::move(context_extensions), - std::move(metadata_context), request, false); - - EXPECT_EQ("source", request.attributes().source().principal()); - EXPECT_EQ("destination", request.attributes().destination().principal()); - EXPECT_EQ("foo", request.attributes().source().service()); - EXPECT_EQ("value", request.attributes().context_extensions().at("key")); - EXPECT_EQ("bar", request.attributes() - .metadata_context() - .filter_metadata() - .at("meta.key") - .fields() - .at("foo") - .string_value()); + callHttpCheckAndValidateRequestAttributes(); } +// Verify that createHttpCheck extract the attributes from the http request into CheckRequest +// proto object and Dns SAN is used as principal if Uri SAN is absent. TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerDnsSans) { - Http::TestHeaderMapImpl request_headers{{"x-envoy-downstream-service-cluster", "foo"}, - {":path", "/bar"}}; - envoy::service::auth::v2::CheckRequest request; - EXPECT_CALL(callbacks_, connection()).WillRepeatedly(Return(&connection_)); - EXPECT_CALL(connection_, remoteAddress()).WillRepeatedly(ReturnRef(addr_)); - EXPECT_CALL(connection_, localAddress()).WillRepeatedly(ReturnRef(addr_)); - EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); - EXPECT_CALL(callbacks_, streamId()).WillRepeatedly(Return(0)); - EXPECT_CALL(callbacks_, streamInfo()).WillRepeatedly(ReturnRef(req_info_)); - EXPECT_CALL(callbacks_, decodingBuffer()).Times(1); - EXPECT_CALL(req_info_, protocol()).WillRepeatedly(ReturnPointee(&protocol_)); + expectBasicHttp(); + EXPECT_CALL(ssl_, uriSanPeerCertificate()).WillOnce(Return(std::vector{})); EXPECT_CALL(ssl_, dnsSansPeerCertificate()).WillOnce(Return(std::vector{"source"})); @@ -190,13 +181,23 @@ TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerDnsSans) { Protobuf::Map context_extensions; context_extensions["key"] = "value"; - CheckRequestUtils::createHttpCheck(&callbacks_, request_headers, std::move(context_extensions), - request, false); + callHttpCheckAndValidateRequestAttributes(); +} + +// Verify that createHttpCheck extract the attributes from the http request into CheckRequest +// proto object and Subject is used as principal if both Uri SAN and Dns SAN are absent. +TEST_F(CheckRequestUtilsTest, CheckAttrContextSubject) { + expectBasicHttp(); + + EXPECT_CALL(ssl_, uriSanPeerCertificate()).WillOnce(Return(std::vector{})); + EXPECT_CALL(ssl_, dnsSansPeerCertificate()).WillOnce(Return(std::vector{})); + EXPECT_CALL(ssl_, subjectPeerCertificate()).WillOnce(Return("source")); + + EXPECT_CALL(ssl_, uriSanLocalCertificate()).WillOnce(Return(std::vector{})); + EXPECT_CALL(ssl_, dnsSansLocalCertificate()).WillOnce(Return(std::vector{})); + EXPECT_CALL(ssl_, subjectLocalCertificate()).WillOnce(Return("destination")); - EXPECT_EQ("source", request.attributes().source().principal()); - EXPECT_EQ("destination", request.attributes().destination().principal()); - EXPECT_EQ("foo", request.attributes().source().service()); - EXPECT_EQ("value", request.attributes().context_extensions().at("key")); + callHttpCheckAndValidateRequestAttributes(); } } // namespace From 06f669a8195ce1701f7e7cf29f3ab75e45f76fe0 Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Thu, 22 Aug 2019 08:45:41 +0530 Subject: [PATCH 5/5] address review comments Signed-off-by: Rama Chavali --- .../common/ext_authz/check_request_utils.cc | 24 +++++++++---------- .../ext_authz/check_request_utils_test.cc | 12 +++++----- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/source/extensions/filters/common/ext_authz/check_request_utils.cc b/source/extensions/filters/common/ext_authz/check_request_utils.cc index a559b1e4a565..707053f46bf0 100644 --- a/source/extensions/filters/common/ext_authz/check_request_utils.cc +++ b/source/extensions/filters/common/ext_authz/check_request_utils.cc @@ -42,28 +42,28 @@ void CheckRequestUtils::setAttrContextPeer(envoy::service::auth::v2::AttributeCo Ssl::ConnectionInfo* ssl = const_cast(connection.ssl()); if (ssl != nullptr) { if (local) { - const auto uriSans = ssl->uriSanLocalCertificate(); - if (uriSans.empty()) { - const auto dnsSans = ssl->dnsSansLocalCertificate(); - if (dnsSans.empty()) { + const auto uri_sans = ssl->uriSanLocalCertificate(); + if (uri_sans.empty()) { + const auto dns_sans = ssl->dnsSansLocalCertificate(); + if (dns_sans.empty()) { peer.set_principal(ssl->subjectLocalCertificate()); } else { - peer.set_principal(dnsSans[0]); + peer.set_principal(dns_sans[0]); } } else { - peer.set_principal(uriSans[0]); + peer.set_principal(uri_sans[0]); } } else { - const auto uriSans = ssl->uriSanPeerCertificate(); - if (uriSans.empty()) { - const auto dnsSans = ssl->dnsSansPeerCertificate(); - if (dnsSans.empty()) { + const auto uri_sans = ssl->uriSanPeerCertificate(); + if (uri_sans.empty()) { + const auto dns_sans = ssl->dnsSansPeerCertificate(); + if (dns_sans.empty()) { peer.set_principal(ssl->subjectPeerCertificate()); } else { - peer.set_principal(dnsSans[0]); + peer.set_principal(dns_sans[0]); } } else { - peer.set_principal(uriSans[0]); + peer.set_principal(uri_sans[0]); } } } diff --git a/test/extensions/filters/common/ext_authz/check_request_utils_test.cc b/test/extensions/filters/common/ext_authz/check_request_utils_test.cc index 0be215a57f81..bc5df7ec323d 100644 --- a/test/extensions/filters/common/ext_authz/check_request_utils_test.cc +++ b/test/extensions/filters/common/ext_authz/check_request_utils_test.cc @@ -154,8 +154,8 @@ TEST_F(CheckRequestUtilsTest, BasicHttpWithFullBody) { Http::Headers::get().EnvoyAuthPartialBody.get())); } -// Verify that createHttpCheck extract the attributes from the http request into CheckRequest -// proto object and Uri SAN is used as principal if present. +// Verify that createHttpCheck extract the attributes from the HTTP request into CheckRequest +// proto object and URI SAN is used as principal if present. TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerUriSans) { expectBasicHttp(); @@ -166,8 +166,8 @@ TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerUriSans) { callHttpCheckAndValidateRequestAttributes(); } -// Verify that createHttpCheck extract the attributes from the http request into CheckRequest -// proto object and Dns SAN is used as principal if Uri SAN is absent. +// Verify that createHttpCheck extract the attributes from the HTTP request into CheckRequest +// proto object and DNS SAN is used as principal if URI SAN is absent. TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerDnsSans) { expectBasicHttp(); @@ -184,8 +184,8 @@ TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerDnsSans) { callHttpCheckAndValidateRequestAttributes(); } -// Verify that createHttpCheck extract the attributes from the http request into CheckRequest -// proto object and Subject is used as principal if both Uri SAN and Dns SAN are absent. +// Verify that createHttpCheck extract the attributes from the HTTP request into CheckRequest +// proto object and Subject is used as principal if both URI SAN and DNS SAN are absent. TEST_F(CheckRequestUtilsTest, CheckAttrContextSubject) { expectBasicHttp();