From 93ce9adee7bd93fcd2d6280e8a35cee5c37acbed Mon Sep 17 00:00:00 2001 From: istio-bot Date: Mon, 30 Apr 2018 15:38:46 -0700 Subject: [PATCH 1/5] Update_Dependencies (#1657) --- istio.deps | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/istio.deps b/istio.deps index aee82e911dd..1a9c3861b3b 100644 --- a/istio.deps +++ b/istio.deps @@ -13,4 +13,4 @@ "file": "WORKSPACE", "lastStableSHA": "2b2c299144600fb9e525d21aabf39bf48e64fb1f" } -] \ No newline at end of file +] From a8f88fa56d71105340d04e5a9448fffb157a58e7 Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Fri, 4 May 2018 13:21:10 -0700 Subject: [PATCH 2/5] Use mTLS mode enum to decide authentication result. --- src/envoy/http/authn/authenticator_base.cc | 22 ++- .../http/authn/authenticator_base_test.cc | 157 ++++++++---------- 2 files changed, 84 insertions(+), 95 deletions(-) diff --git a/src/envoy/http/authn/authenticator_base.cc b/src/envoy/http/authn/authenticator_base.cc index 02ee0e796e0..9f06baabd56 100644 --- a/src/envoy/http/authn/authenticator_base.cc +++ b/src/envoy/http/authn/authenticator_base.cc @@ -34,16 +34,28 @@ AuthenticatorBase::~AuthenticatorBase() {} bool AuthenticatorBase::validateX509(const iaapi::MutualTls& mtls, Payload* payload) const { const Network::Connection* connection = filter_context_.connection(); - if (connection == nullptr || connection->ssl() == nullptr) { - // Not a TLS connection + if (connection == nullptr) { + // It's wrong if connection does not exist. return false; } - - bool has_user = + // Always try to get principal and set to output if available. + const bool has_user = + connection->ssl() != nullptr && connection->ssl()->peerCertificatePresented() && Utils::GetSourceUser(connection, payload->mutable_x509()->mutable_user()); - return has_user || mtls.allow_tls(); + // Return value depend on mode: + // - PERMISSIVE: plaintext connection is acceptable, thus return true regardless. + // - TLS_PERMISSIVE: tls connection is required, but certificate is optional. + // - STRICT: must be TLS with valid certificate. + switch (mtls.mode()) { + case iaapi::MutualTls::PERMISSIVE: return true; + case iaapi::MutualTls::TLS_PERMISSIVE: return connection->ssl() != nullptr; + case iaapi::MutualTls::STRICT: return has_user; + default: + ENVOY_LOG(warn, "Invalid mTLS mode {}", iaapi::MutualTls::Mode_Name(mtls.mode())); + return false; + } } bool AuthenticatorBase::validateJwt(const iaapi::Jwt& jwt, Payload* payload) { diff --git a/src/envoy/http/authn/authenticator_base_test.cc b/src/envoy/http/authn/authenticator_base_test.cc index e9b9885e33f..fbcbf936261 100644 --- a/src/envoy/http/authn/authenticator_base_test.cc +++ b/src/envoy/http/authn/authenticator_base_test.cc @@ -55,10 +55,10 @@ class MockAuthenticatorBase : public AuthenticatorBase { MOCK_METHOD1(run, bool(Payload*)); }; -class AuthenticatorBaseTest : public testing::Test, +class ValidateX509Test : public testing::TestWithParam, public Logger::Loggable { public: - virtual ~AuthenticatorBaseTest() {} + virtual ~ValidateX509Test() {} Http::TestHeaderMapImpl request_headers_{}; NiceMock connection_{}; @@ -70,7 +70,10 @@ class AuthenticatorBaseTest : public testing::Test, MockAuthenticatorBase authenticator_{&filter_context_}; - void SetUp() override { payload_ = new Payload(); } + void SetUp() override { + mtls_params_.set_mode(GetParam()); + payload_ = new Payload(); + } void TearDown() override { delete (payload_); } @@ -81,51 +84,33 @@ class AuthenticatorBaseTest : public testing::Test, Payload default_payload_; }; -Http::TestHeaderMapImpl CreateTestHeaderMap(const std::string& header_key, - const std::string& header_value) { - // The base64 encoding is done through Base64::encode(). - // If the test input has special chars, may need to use the counterpart of - // Base64UrlDecode(). - std::string value_base64 = - Base64::encode(header_value.c_str(), header_value.size()); - return Http::TestHeaderMapImpl{{header_key, value_base64}}; -} - -TEST_F(AuthenticatorBaseTest, ValidateMtlsOnPlaintextConnection) { - EXPECT_FALSE(authenticator_.validateX509(mtls_params_, payload_)); - EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); -} - -TEST_F(AuthenticatorBaseTest, ValidateTlsOnPlaintextConnection) { - mtls_params_.set_allow_tls(true); // allow TLS connection - // When requiring TLS, plaintext connection should fail. - EXPECT_FALSE(authenticator_.validateX509(mtls_params_, payload_)); +TEST_P(ValidateX509Test, PlaintextConnection) { + // Should return false except mode is PERMISSIVE (accept plaintext) + if (GetParam() == iaapi::MutualTls::PERMISSIVE) { + EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); + } else { + EXPECT_FALSE(authenticator_.validateX509(mtls_params_, payload_)); + } EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(AuthenticatorBaseTest, ValidateMtlsOnSslConnectionWithNoPeerCert) { +TEST_P(ValidateX509Test, SslConnectionWithNoPeerCert) { EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); EXPECT_CALL(Const(ssl_), peerCertificatePresented()) .Times(1) .WillOnce(Return(false)); - EXPECT_FALSE(authenticator_.validateX509(mtls_params_, payload_)); + // Should return false except mode is PERMISSIVE (accept plaintext) or TLS_PERMISSIVE + if (GetParam() == iaapi::MutualTls::PERMISSIVE || + GetParam() == iaapi::MutualTls::TLS_PERMISSIVE) { + EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); + } else { + EXPECT_FALSE(authenticator_.validateX509(mtls_params_, payload_)); + } EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(AuthenticatorBaseTest, ValidateTlsOnSslConnectionWithNoPeerCert) { - mtls_params_.set_allow_tls(true); // allow TLS connection - EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); - EXPECT_CALL(Const(ssl_), peerCertificatePresented()) - .Times(1) - .WillOnce(Return(false)); - - // When client certificate is not present on TLS, authentication should still - // succeed. - EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); -} - -TEST_F(AuthenticatorBaseTest, ValidateMtlsOnSslConnectionWithPeerCert) { +TEST_P(ValidateX509Test, SslConnectionWithPeerCert) { EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); EXPECT_CALL(Const(ssl_), peerCertificatePresented()) .Times(1) @@ -137,20 +122,7 @@ TEST_F(AuthenticatorBaseTest, ValidateMtlsOnSslConnectionWithPeerCert) { EXPECT_EQ(payload_->x509().user(), "foo"); } -TEST_F(AuthenticatorBaseTest, ValidateTlsOnSslConnectionWithPeerCert) { - mtls_params_.set_allow_tls(true); // allow TLS connection - EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); - EXPECT_CALL(Const(ssl_), peerCertificatePresented()) - .Times(1) - .WillOnce(Return(true)); - EXPECT_CALL(ssl_, uriSanPeerCertificate()).Times(1).WillOnce(Return("foo")); - EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); - // When client certificate is present on TLS, authenticated attribute should - // be extracted. - EXPECT_EQ(payload_->x509().user(), "foo"); -} - -TEST_F(AuthenticatorBaseTest, ValidateMtlsOnSslConnectionWithPeerSpiffeCert) { +TEST_P(ValidateX509Test, SslConnectionWithPeerSpiffeCert) { EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); EXPECT_CALL(Const(ssl_), peerCertificatePresented()) .Times(1) @@ -165,23 +137,7 @@ TEST_F(AuthenticatorBaseTest, ValidateMtlsOnSslConnectionWithPeerSpiffeCert) { EXPECT_EQ(payload_->x509().user(), "foo"); } -TEST_F(AuthenticatorBaseTest, ValidateTlsOnSslConnectionWithPeerSpiffeCert) { - mtls_params_.set_allow_tls(true); // allow TLS connection - EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); - EXPECT_CALL(Const(ssl_), peerCertificatePresented()) - .Times(1) - .WillOnce(Return(true)); - EXPECT_CALL(ssl_, uriSanPeerCertificate()) - .Times(1) - .WillOnce(Return("spiffe://foo")); - EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); - // When client certificate is present on TLS, authenticated attribute should - // be extracted. - EXPECT_EQ(payload_->x509().user(), "foo"); -} - -TEST_F(AuthenticatorBaseTest, - ValidateMtlsOnSslConnectionWithPeerMalformedSpiffeCert) { +TEST_P(ValidateX509Test, SslConnectionWithPeerMalformedSpiffeCert) { EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); EXPECT_CALL(Const(ssl_), peerCertificatePresented()) .Times(1) @@ -198,26 +154,47 @@ TEST_F(AuthenticatorBaseTest, EXPECT_EQ(payload_->x509().user(), "spiffe:foo"); } -TEST_F(AuthenticatorBaseTest, - ValidateTlsOnSslConnectionWithPeerMalformedSpiffeCert) { - mtls_params_.set_allow_tls(true); // allow TLS connection - EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); - EXPECT_CALL(Const(ssl_), peerCertificatePresented()) - .Times(1) - .WillOnce(Return(true)); - EXPECT_CALL(ssl_, uriSanPeerCertificate()) - .Times(1) - .WillOnce(Return("spiffe:foo")); - EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); - // When client certificate is present on TLS and the spiffe subject format is - // wrong - // ("spiffe:foo" instead of "spiffe://foo"), the user attribute should be - // extracted. - EXPECT_EQ(payload_->x509().user(), "spiffe:foo"); -} +INSTANTIATE_TEST_CASE_P(ValidateX509Tests, ValidateX509Test, + testing::Values(iaapi::MutualTls::STRICT, iaapi::MutualTls::TLS_PERMISSIVE, iaapi::MutualTls::PERMISSIVE)); + +class ValidateJwtTest : public testing::Test, + public Logger::Loggable { + public: + virtual ~ValidateJwtTest() {} + + Http::TestHeaderMapImpl request_headers_{}; + NiceMock connection_{}; + NiceMock ssl_{}; + FilterConfig filter_config_{}; + FilterContext filter_context_{&request_headers_, &connection_, + istio::envoy::config::filter::http::authn:: + v2alpha1::FilterConfig::default_instance()}; + + MockAuthenticatorBase authenticator_{&filter_context_}; + + void SetUp() override { payload_ = new Payload(); } + + void TearDown() override { delete (payload_); } + + Http::TestHeaderMapImpl CreateTestHeaderMap(const std::string& header_key, + const std::string& header_value) { + // The base64 encoding is done through Base64::encode(). + // If the test input has special chars, may need to use the counterpart of + // Base64UrlDecode(). + std::string value_base64 = + Base64::encode(header_value.c_str(), header_value.size()); + return Http::TestHeaderMapImpl{{header_key, value_base64}}; + } + + protected: + iaapi::MutualTls mtls_params_; + iaapi::Jwt jwt_; + Payload* payload_; + Payload default_payload_; +}; // TODO: more tests for Jwt. -TEST_F(AuthenticatorBaseTest, ValidateJwtWithNoIstioAuthnConfig) { +TEST_F(ValidateJwtTest, ValidateJwtWithNoIstioAuthnConfig) { jwt_.set_issuer("issuer@foo.com"); // authenticator_ has empty Istio authn config // When there is empty Istio authn config, validateJwt() should return @@ -226,7 +203,7 @@ TEST_F(AuthenticatorBaseTest, ValidateJwtWithNoIstioAuthnConfig) { EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(AuthenticatorBaseTest, ValidateJwtWithNoIssuer) { +TEST_F(ValidateJwtTest, NoIssuer) { // no issuer in jwt google::protobuf::util::JsonParseOptions options; FilterConfig filter_config; @@ -250,7 +227,7 @@ TEST_F(AuthenticatorBaseTest, ValidateJwtWithNoIssuer) { EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(AuthenticatorBaseTest, ValidateJwtWithEmptyJwtOutputPayloadLocations) { +TEST_F(ValidateJwtTest, EmptyJwtOutputPayloadLocations) { jwt_.set_issuer("issuer@foo.com"); Http::TestHeaderMapImpl request_headers_with_jwt = CreateTestHeaderMap( kSecIstioAuthUserInfoHeaderKey, kSecIstioAuthUserinfoHeaderValue); @@ -274,7 +251,7 @@ TEST_F(AuthenticatorBaseTest, ValidateJwtWithEmptyJwtOutputPayloadLocations) { EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(AuthenticatorBaseTest, ValidateJwtWithNoJwtInHeader) { +TEST_F(ValidateJwtTest, NoJwtInHeader) { jwt_.set_issuer("issuer@foo.com"); google::protobuf::util::JsonParseOptions options; FilterConfig filter_config; @@ -297,7 +274,7 @@ TEST_F(AuthenticatorBaseTest, ValidateJwtWithNoJwtInHeader) { EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(AuthenticatorBaseTest, ValidateJwtWithJwtInHeader) { +TEST_F(ValidateJwtTest, JwtInHeader) { jwt_.set_issuer("issuer@foo.com"); Http::TestHeaderMapImpl request_headers_with_jwt = CreateTestHeaderMap( "sec-istio-auth-jwt-output", kSecIstioAuthUserinfoHeaderValue); From 55c5d75bbee7a2d89828e25db6fa4dfbea48527f Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Mon, 7 May 2018 14:20:11 -0700 Subject: [PATCH 3/5] Clang --- src/envoy/http/authn/authenticator_base.cc | 15 ++++++++++----- src/envoy/http/authn/authenticator_base_test.cc | 11 +++++++---- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/envoy/http/authn/authenticator_base.cc b/src/envoy/http/authn/authenticator_base.cc index 9f06baabd56..a4b0d13fde0 100644 --- a/src/envoy/http/authn/authenticator_base.cc +++ b/src/envoy/http/authn/authenticator_base.cc @@ -45,15 +45,20 @@ bool AuthenticatorBase::validateX509(const iaapi::MutualTls& mtls, Utils::GetSourceUser(connection, payload->mutable_x509()->mutable_user()); // Return value depend on mode: - // - PERMISSIVE: plaintext connection is acceptable, thus return true regardless. + // - PERMISSIVE: plaintext connection is acceptable, thus return true + // regardless. // - TLS_PERMISSIVE: tls connection is required, but certificate is optional. // - STRICT: must be TLS with valid certificate. switch (mtls.mode()) { - case iaapi::MutualTls::PERMISSIVE: return true; - case iaapi::MutualTls::TLS_PERMISSIVE: return connection->ssl() != nullptr; - case iaapi::MutualTls::STRICT: return has_user; + case iaapi::MutualTls::PERMISSIVE: + return true; + case iaapi::MutualTls::TLS_PERMISSIVE: + return connection->ssl() != nullptr; + case iaapi::MutualTls::STRICT: + return has_user; default: - ENVOY_LOG(warn, "Invalid mTLS mode {}", iaapi::MutualTls::Mode_Name(mtls.mode())); + ENVOY_LOG(warn, "Invalid mTLS mode {}", + iaapi::MutualTls::Mode_Name(mtls.mode())); return false; } } diff --git a/src/envoy/http/authn/authenticator_base_test.cc b/src/envoy/http/authn/authenticator_base_test.cc index fbcbf936261..f223c0e9433 100644 --- a/src/envoy/http/authn/authenticator_base_test.cc +++ b/src/envoy/http/authn/authenticator_base_test.cc @@ -56,7 +56,7 @@ class MockAuthenticatorBase : public AuthenticatorBase { }; class ValidateX509Test : public testing::TestWithParam, - public Logger::Loggable { + public Logger::Loggable { public: virtual ~ValidateX509Test() {} @@ -100,7 +100,8 @@ TEST_P(ValidateX509Test, SslConnectionWithNoPeerCert) { .Times(1) .WillOnce(Return(false)); - // Should return false except mode is PERMISSIVE (accept plaintext) or TLS_PERMISSIVE + // Should return false except mode is PERMISSIVE (accept plaintext) or + // TLS_PERMISSIVE if (GetParam() == iaapi::MutualTls::PERMISSIVE || GetParam() == iaapi::MutualTls::TLS_PERMISSIVE) { EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); @@ -155,10 +156,12 @@ TEST_P(ValidateX509Test, SslConnectionWithPeerMalformedSpiffeCert) { } INSTANTIATE_TEST_CASE_P(ValidateX509Tests, ValidateX509Test, - testing::Values(iaapi::MutualTls::STRICT, iaapi::MutualTls::TLS_PERMISSIVE, iaapi::MutualTls::PERMISSIVE)); + testing::Values(iaapi::MutualTls::STRICT, + iaapi::MutualTls::TLS_PERMISSIVE, + iaapi::MutualTls::PERMISSIVE)); class ValidateJwtTest : public testing::Test, - public Logger::Loggable { + public Logger::Loggable { public: virtual ~ValidateJwtTest() {} From 288d25c965aa186a7ad22700a71ab3aa961dd4f9 Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Mon, 7 May 2018 15:38:02 -0700 Subject: [PATCH 4/5] Use NOT_REACHED macros for default case. --- src/envoy/http/authn/authenticator_base.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/envoy/http/authn/authenticator_base.cc b/src/envoy/http/authn/authenticator_base.cc index a4b0d13fde0..4117b912fa1 100644 --- a/src/envoy/http/authn/authenticator_base.cc +++ b/src/envoy/http/authn/authenticator_base.cc @@ -13,6 +13,7 @@ * limitations under the License. */ +#include "common/common/assert.h" #include "src/envoy/http/authn/authenticator_base.h" #include "src/envoy/http/authn/authn_utils.h" #include "src/envoy/utils/utils.h" @@ -57,9 +58,7 @@ bool AuthenticatorBase::validateX509(const iaapi::MutualTls& mtls, case iaapi::MutualTls::STRICT: return has_user; default: - ENVOY_LOG(warn, "Invalid mTLS mode {}", - iaapi::MutualTls::Mode_Name(mtls.mode())); - return false; + NOT_REACHED; } } From 407fcf8aa621fb8cd8c75ec9ee87bb4f89aac5ac Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Mon, 7 May 2018 16:26:07 -0700 Subject: [PATCH 5/5] Clang --- src/envoy/http/authn/authenticator_base.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/envoy/http/authn/authenticator_base.cc b/src/envoy/http/authn/authenticator_base.cc index 4117b912fa1..a2dba3de4cc 100644 --- a/src/envoy/http/authn/authenticator_base.cc +++ b/src/envoy/http/authn/authenticator_base.cc @@ -13,8 +13,8 @@ * limitations under the License. */ -#include "common/common/assert.h" #include "src/envoy/http/authn/authenticator_base.h" +#include "common/common/assert.h" #include "src/envoy/http/authn/authn_utils.h" #include "src/envoy/utils/utils.h"