diff --git a/WORKSPACE b/WORKSPACE index 4d1dc388e04..b864fa6513a 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -38,7 +38,7 @@ git_repository( ) # When updating envoy sha manually please update the sha in istio.deps file also -ENVOY_SHA = "d55f4990889da5d6874ab70ec5f4e5e0a9053160" +ENVOY_SHA = "2b2c299144600fb9e525d21aabf39bf48e64fb1f" http_archive( name = "envoy", @@ -60,8 +60,6 @@ load("@com_lyft_protoc_gen_validate//bazel:go_proto_library.bzl", "go_proto_repo go_proto_repositories(shared=0) go_rules_dependencies() go_register_toolchains() -load("@io_bazel_rules_go//proto:def.bzl", "proto_register_toolchains") -proto_register_toolchains() load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository") git_repository( diff --git a/istio.deps b/istio.deps index 7d7444d54b6..d646d377bc2 100644 --- a/istio.deps +++ b/istio.deps @@ -3,7 +3,6 @@ "_comment": "", "name": "ISTIO_API", "repoName": "api", - "prodBranch": "master", "file": "repositories.bzl", "lastStableSHA": "78da6e6eb4ad4f158fb58e02f94efde4abf4cabf" }, @@ -11,8 +10,7 @@ "_comment": "", "name": "ENVOY_SHA", "repoName": "envoyproxy/envoy", - "prodBranch": "master", "file": "WORKSPACE", - "lastStableSHA": "d55f4990889da5d6874ab70ec5f4e5e0a9053160" + "lastStableSHA": "2b2c299144600fb9e525d21aabf39bf48e64fb1f" } ] \ No newline at end of file diff --git a/src/envoy/http/authn/authenticator_base.cc b/src/envoy/http/authn/authenticator_base.cc index 02ee0e796e0..a2dba3de4cc 100644 --- a/src/envoy/http/authn/authenticator_base.cc +++ b/src/envoy/http/authn/authenticator_base.cc @@ -14,6 +14,7 @@ */ #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" @@ -34,16 +35,31 @@ 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: + NOT_REACHED; + } } 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..9c6d69c6510 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, - public Logger::Loggable { +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,34 @@ 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 +123,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 +138,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 +155,49 @@ 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 +206,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 +230,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 +254,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 +277,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); @@ -327,7 +307,8 @@ TEST_F(AuthenticatorBaseTest, ValidateJwtWithJwtInHeader) { "iss": "issuer@foo.com", "sub": "sub@foo.com", "some-other-string-claims": "some-claims-kept" - } + }, + raw_claims: "\n {\n \"iss\": \"issuer@foo.com\",\n \"sub\": \"sub@foo.com\",\n \"aud\": \"aud1\",\n \"non-string-will-be-ignored\": 1512754205,\n \"some-other-string-claims\": \"some-claims-kept\"\n }\n " } } )", diff --git a/src/envoy/http/authn/authn_utils.cc b/src/envoy/http/authn/authn_utils.cc index 2dc57bfed20..78c232e3108 100644 --- a/src/envoy/http/authn/authn_utils.cc +++ b/src/envoy/http/authn/authn_utils.cc @@ -68,6 +68,7 @@ bool AuthnUtils::GetJWTPayloadFromHeaders( jwt_payload_key.get(), value); return false; } + *payload->mutable_raw_claims() = payload_str; ::google::protobuf::Map< ::std::string, ::std::string>* claims = payload->mutable_claims(); Envoy::Json::ObjectSharedPtr json_obj; diff --git a/src/envoy/http/authn/authn_utils_test.cc b/src/envoy/http/authn/authn_utils_test.cc index fe9f3105445..3663476ab0d 100644 --- a/src/envoy/http/authn/authn_utils_test.cc +++ b/src/envoy/http/authn/authn_utils_test.cc @@ -14,6 +14,7 @@ */ #include "src/envoy/http/authn/authn_utils.h" #include "common/common/base64.h" +#include "common/common/utility.h" #include "src/envoy/http/authn/test_utils.h" #include "test/test_common/utility.h" @@ -91,7 +92,8 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderTest) { key: "some-other-string-claims" value: "some-claims-kept" } - )", + raw_claims: ")" + + StringUtil::escape(kSecIstioAuthUserinfoHeaderValue) + R"(")", &expected_payload)); // The payload returned from GetJWTPayloadFromHeaders() should be the same as // the expected. @@ -121,7 +123,9 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderWithNoAudTest) { key: "some-other-string-claims" value: "some-claims-kept" } - )", + raw_claims: ")" + + StringUtil::escape(kSecIstioAuthUserInfoHeaderWithNoAudValue) + + R"(")", &expected_payload)); // The payload returned from GetJWTPayloadFromHeaders() should be the same as // the expected. When there is no aud, the aud is not saved in the payload @@ -154,7 +158,9 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderWithTwoAudTest) { key: "some-other-string-claims" value: "some-claims-kept" } - )", + raw_claims: ")" + + StringUtil::escape(kSecIstioAuthUserInfoHeaderWithTwoAudValue) + + R"(")", &expected_payload)); // The payload returned from GetJWTPayloadFromHeaders() should be the same as diff --git a/src/envoy/http/authn/filter_context.h b/src/envoy/http/authn/filter_context.h index 39a3c79b763..7907479d55f 100644 --- a/src/envoy/http/authn/filter_context.h +++ b/src/envoy/http/authn/filter_context.h @@ -18,6 +18,8 @@ #include "authentication/v1alpha1/policy.pb.h" #include "common/common/logger.h" #include "envoy/config/filter/http/authn/v2alpha1/config.pb.h" +#include "envoy/http/header_map.h" +#include "envoy/network/connection.h" #include "src/istio/authn/context.pb.h" namespace Envoy { diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index 75e172ef87b..e8fe41be1d2 100644 --- a/src/envoy/http/authn/http_filter.cc +++ b/src/envoy/http/authn/http_filter.cc @@ -51,12 +51,14 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap& headers, Payload payload; - if (!createPeerAuthenticator(filter_context_.get())->run(&payload)) { + if (!filter_config_.policy().peer_is_optional() && + !createPeerAuthenticator(filter_context_.get())->run(&payload)) { rejectRequest("Peer authentication failed."); return FilterHeadersStatus::StopIteration; } bool success = + filter_config_.policy().origin_is_optional() || createOriginAuthenticator(filter_context_.get())->run(&payload); // After Istio authn, the JWT headers consumed by Istio authn should be diff --git a/src/envoy/http/authn/http_filter_factory.cc b/src/envoy/http/authn/http_filter_factory.cc index 18ce537248b..dcf344fac78 100644 --- a/src/envoy/http/authn/http_filter_factory.cc +++ b/src/envoy/http/authn/http_filter_factory.cc @@ -34,9 +34,9 @@ const std::string kAuthnFactoryName("istio_authn"); class AuthnFilterConfig : public NamedHttpFilterConfigFactory, public Logger::Loggable { public: - HttpFilterFactoryCb createFilterFactory(const Json::Object& config, - const std::string&, - FactoryContext&) override { + Http::FilterFactoryCb createFilterFactory(const Json::Object& config, + const std::string&, + FactoryContext&) override { ENVOY_LOG(debug, "Called AuthnFilterConfig : {}", __func__); google::protobuf::util::Status status = @@ -54,7 +54,7 @@ class AuthnFilterConfig : public NamedHttpFilterConfigFactory, } } - HttpFilterFactoryCb createFilterFactoryFromProto( + Http::FilterFactoryCb createFilterFactoryFromProto( const Protobuf::Message& proto_config, const std::string&, FactoryContext&) override { filter_config_ = dynamic_cast(proto_config); @@ -69,7 +69,7 @@ class AuthnFilterConfig : public NamedHttpFilterConfigFactory, std::string name() override { return kAuthnFactoryName; } private: - HttpFilterFactoryCb createFilter() { + Http::FilterFactoryCb createFilter() { ENVOY_LOG(debug, "Called AuthnFilterConfig : {}", __func__); return [&](Http::FilterChainFactoryCallbacks& callbacks) -> void { diff --git a/src/envoy/http/authn/http_filter_integration_test.cc b/src/envoy/http/authn/http_filter_integration_test.cc index 3ec0c1197e1..5690ef7766c 100644 --- a/src/envoy/http/authn/http_filter_integration_test.cc +++ b/src/envoy/http/authn/http_filter_integration_test.cc @@ -214,7 +214,8 @@ TEST_P(AuthenticationFilterIntegrationTest, CheckAuthnResultIsExpected) { "aud": "bookstore-esp-echo.cloudendpointsapis.com", "iss": "628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com", "sub": "628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com" - } + }, + raw_claims: "{\"iss\":\"628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com\",\"sub\":\"628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com\",\"aud\":\"bookstore-esp-echo.cloudendpointsapis.com\",\"iat\":1512754205,\"exp\":5112754205}" } } )", diff --git a/src/envoy/http/authn/http_filter_test.cc b/src/envoy/http/authn/http_filter_test.cc index 0c82a5e6959..79b39d7e76a 100644 --- a/src/envoy/http/authn/http_filter_test.cc +++ b/src/envoy/http/authn/http_filter_test.cc @@ -35,12 +35,19 @@ using testing::NiceMock; using testing::StrictMock; using testing::_; +namespace iaapi = istio::authentication::v1alpha1; + namespace Envoy { namespace Http { namespace Istio { namespace AuthN { namespace { +const char ingoreBothPolicy[] = R"( + peer_is_optional: true + origin_is_optional: true +)"; + // Create a fake authenticator for test. This authenticator do nothing except // making the authentication fail. std::unique_ptr createAlwaysFailAuthenticator( @@ -74,8 +81,9 @@ class MockAuthenticationFilter : public AuthenticationFilter { public: // We'll use fake authenticator for test, so policy is not really needed. Use // default config for simplicity. - MockAuthenticationFilter() - : AuthenticationFilter(FilterConfig::default_instance()) {} + MockAuthenticationFilter(const FilterConfig& filter_config) + : AuthenticationFilter(filter_config) {} + ~MockAuthenticationFilter(){}; MOCK_METHOD1(createPeerAuthenticator, @@ -95,9 +103,11 @@ class AuthenticationFilterTest : public testing::Test { } protected: - StrictMock filter_; - NiceMock decoder_callbacks_; + FilterConfig filter_config_ = FilterConfig::default_instance(); + Http::TestHeaderMapImpl request_headers_; + StrictMock filter_{filter_config_}; + NiceMock decoder_callbacks_; }; TEST_F(AuthenticationFilterTest, PeerFail) { @@ -151,6 +161,15 @@ TEST_F(AuthenticationFilterTest, AllPass) { TestUtilities::AuthNResultFromString(R"(peer_user: "foo")"), authn)); } +TEST_F(AuthenticationFilterTest, IgnoreBothFail) { + iaapi::Policy policy_; + ASSERT_TRUE( + Protobuf::TextFormat::ParseFromString(ingoreBothPolicy, &policy_)); + *filter_config_.mutable_policy() = policy_; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_.decodeHeaders(request_headers_, true)); +} + } // namespace } // namespace AuthN } // namespace Istio diff --git a/src/envoy/http/jwt_auth/http_filter_factory.cc b/src/envoy/http/jwt_auth/http_filter_factory.cc index 4656902e786..018b8a65788 100644 --- a/src/envoy/http/jwt_auth/http_filter_factory.cc +++ b/src/envoy/http/jwt_auth/http_filter_factory.cc @@ -27,15 +27,15 @@ namespace Configuration { class JwtVerificationFilterConfig : public NamedHttpFilterConfigFactory { public: - HttpFilterFactoryCb createFilterFactory(const Json::Object& config, - const std::string&, - FactoryContext& context) override { + Http::FilterFactoryCb createFilterFactory(const Json::Object& config, + const std::string&, + FactoryContext& context) override { JwtAuthentication proto_config; MessageUtil::loadFromJson(config.asJsonString(), proto_config); return createFilter(proto_config, context); } - HttpFilterFactoryCb createFilterFactoryFromProto( + Http::FilterFactoryCb createFilterFactoryFromProto( const Protobuf::Message& proto_config, const std::string&, FactoryContext& context) override { return createFilter( @@ -51,8 +51,8 @@ class JwtVerificationFilterConfig : public NamedHttpFilterConfigFactory { std::string name() override { return "jwt-auth"; } private: - HttpFilterFactoryCb createFilter(const JwtAuthentication& proto_config, - FactoryContext& context) { + Http::FilterFactoryCb createFilter(const JwtAuthentication& proto_config, + FactoryContext& context) { auto store_factory = std::make_shared( proto_config, context); Upstream::ClusterManager& cm = context.clusterManager(); diff --git a/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc b/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc index c750f014629..5d53ba3cff3 100644 --- a/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc +++ b/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc @@ -358,8 +358,6 @@ INSTANTIATE_TEST_CASE_P( TEST_P(JwtVerificationFilterIntegrationTestWithInjectedJwtResult, InjectedJwtResultSanitized) { - // Issuer is not called by passing empty pubkey. - std::string pubkey = ""; // Create a request without JWT. // With allow_missing_or_failed option being true, a request without JWT // will reach the backend. This is to test the injected JWT result. diff --git a/src/envoy/http/jwt_auth/token_extractor.h b/src/envoy/http/jwt_auth/token_extractor.h index da8a74ad459..4efd808afa4 100644 --- a/src/envoy/http/jwt_auth/token_extractor.h +++ b/src/envoy/http/jwt_auth/token_extractor.h @@ -17,6 +17,7 @@ #include "common/common/logger.h" #include "envoy/config/filter/http/jwt_authn/v2alpha/config.pb.h" +#include "envoy/http/header_map.h" namespace Envoy { namespace Http { diff --git a/src/envoy/http/mixer/control.cc b/src/envoy/http/mixer/control.cc index fb8fd8bedb2..0ce9e327415 100644 --- a/src/envoy/http/mixer/control.cc +++ b/src/envoy/http/mixer/control.cc @@ -42,8 +42,8 @@ Control::Control(const Config& config, Upstream::ClusterManager& cm, } Utils::CheckTransport::Func Control::GetCheckTransport( - const HeaderMap* headers) { - return Utils::CheckTransport::GetFunc(*check_client_factory_, headers); + Tracing::Span& parent_span) { + return Utils::CheckTransport::GetFunc(*check_client_factory_, parent_span); } // Call controller to get statistics. diff --git a/src/envoy/http/mixer/control.h b/src/envoy/http/mixer/control.h index aacf783c512..07740a4df9e 100644 --- a/src/envoy/http/mixer/control.h +++ b/src/envoy/http/mixer/control.h @@ -41,7 +41,7 @@ class Control final : public ThreadLocal::ThreadLocalObject { ::istio::control::http::Controller* controller() { return controller_.get(); } // Create a per-request Check transport function. - Utils::CheckTransport::Func GetCheckTransport(const HeaderMap* headers); + Utils::CheckTransport::Func GetCheckTransport(Tracing::Span& parent_span); private: // Call controller to get statistics. diff --git a/src/envoy/http/mixer/filter.cc b/src/envoy/http/mixer/filter.cc index a2c67664143..597ec5af51c 100644 --- a/src/envoy/http/mixer/filter.cc +++ b/src/envoy/http/mixer/filter.cc @@ -133,7 +133,8 @@ FilterHeadersStatus Filter::decodeHeaders(HeaderMap& headers, bool) { HeaderUpdate header_update(&headers); headers_ = &headers; cancel_check_ = handler_->Check( - &check_data, &header_update, control_.GetCheckTransport(&headers), + &check_data, &header_update, + control_.GetCheckTransport(decoder_callbacks_->activeSpan()), [this](const Status& status) { completeCheck(status); }); initiating_call_ = false; @@ -211,6 +212,7 @@ void Filter::onDestroy() { void Filter::log(const HeaderMap* request_headers, const HeaderMap* response_headers, + const HeaderMap* response_trailers, const RequestInfo::RequestInfo& request_info) { ENVOY_LOG(debug, "Called Mixer::Filter : {}", __func__); if (!handler_) { @@ -227,7 +229,8 @@ void Filter::log(const HeaderMap* request_headers, handler_->ExtractRequestAttributes(&check_data); } // response trailer header is not counted to response total size. - ReportData report_data(response_headers, request_info, request_total_size_); + ReportData report_data(response_headers, response_trailers, request_info, + request_total_size_); handler_->Report(&report_data); } diff --git a/src/envoy/http/mixer/filter.h b/src/envoy/http/mixer/filter.h index 9b38bb54ccc..6c31add3993 100644 --- a/src/envoy/http/mixer/filter.h +++ b/src/envoy/http/mixer/filter.h @@ -55,6 +55,7 @@ class Filter : public Http::StreamDecoderFilter, // Called when the request is completed. virtual void log(const HeaderMap* request_headers, const HeaderMap* response_headers, + const HeaderMap* response_trailers, const RequestInfo::RequestInfo& request_info) override; private: diff --git a/src/envoy/http/mixer/filter_factory.cc b/src/envoy/http/mixer/filter_factory.cc index f8a5165a342..4f045d479ba 100644 --- a/src/envoy/http/mixer/filter_factory.cc +++ b/src/envoy/http/mixer/filter_factory.cc @@ -30,9 +30,9 @@ namespace Configuration { class MixerConfigFactory : public NamedHttpFilterConfigFactory { public: - HttpFilterFactoryCb createFilterFactory(const Json::Object& config_json, - const std::string& prefix, - FactoryContext& context) override { + Http::FilterFactoryCb createFilterFactory(const Json::Object& config_json, + const std::string& prefix, + FactoryContext& context) override { HttpClientConfig config_pb; if (!Utils::ReadV2Config(config_json, &config_pb) && !Utils::ReadV1Config(config_json, &config_pb)) { @@ -42,7 +42,7 @@ class MixerConfigFactory : public NamedHttpFilterConfigFactory { return createFilterFactory(config_pb, prefix, context); } - HttpFilterFactoryCb createFilterFactoryFromProto( + Http::FilterFactoryCb createFilterFactoryFromProto( const Protobuf::Message& proto_config, const std::string& prefix, FactoryContext& context) override { return createFilterFactory( @@ -58,7 +58,9 @@ class MixerConfigFactory : public NamedHttpFilterConfigFactory { } Router::RouteSpecificFilterConfigConstSharedPtr - createRouteSpecificFilterConfig(const Protobuf::Message& config) override { + createRouteSpecificFilterConfig( + const Protobuf::Message& config, + Envoy::Server::Configuration::FactoryContext&) override { auto obj = std::make_shared(); // TODO: use downcastAndValidate once client_config.proto adds validate // rules. @@ -70,9 +72,9 @@ class MixerConfigFactory : public NamedHttpFilterConfigFactory { std::string name() override { return "mixer"; } private: - HttpFilterFactoryCb createFilterFactory(const HttpClientConfig& config_pb, - const std::string&, - FactoryContext& context) { + Http::FilterFactoryCb createFilterFactory(const HttpClientConfig& config_pb, + const std::string&, + FactoryContext& context) { std::unique_ptr config_obj( new Http::Mixer::Config(config_pb)); auto control_factory = std::make_shared( diff --git a/src/envoy/http/mixer/report_data.h b/src/envoy/http/mixer/report_data.h index 7cd16e5c317..46658b60b5f 100644 --- a/src/envoy/http/mixer/report_data.h +++ b/src/envoy/http/mixer/report_data.h @@ -36,8 +36,8 @@ class ReportData : public ::istio::control::http::ReportData { uint64_t request_total_size_; public: - ReportData(const HeaderMap *headers, const RequestInfo::RequestInfo &info, - uint64_t request_total_size) + ReportData(const HeaderMap *headers, const HeaderMap *response_trailers, + const RequestInfo::RequestInfo &info, uint64_t request_total_size) : headers_(headers), info_(info), response_total_size_(info.bytesSent()), @@ -45,6 +45,9 @@ class ReportData : public ::istio::control::http::ReportData { if (headers != nullptr) { response_total_size_ += headers->byteSize(); } + if (response_trailers != nullptr) { + response_total_size_ += response_trailers->byteSize(); + } } std::map GetResponseHeaders() const override { diff --git a/src/envoy/tcp/mixer/filter_factory.cc b/src/envoy/tcp/mixer/filter_factory.cc index 7290226c1f8..fe9732a8865 100644 --- a/src/envoy/tcp/mixer/filter_factory.cc +++ b/src/envoy/tcp/mixer/filter_factory.cc @@ -26,8 +26,8 @@ namespace Configuration { class FilterFactory : public NamedNetworkFilterConfigFactory { public: - NetworkFilterFactoryCb createFilterFactory(const Json::Object& config_json, - FactoryContext& context) override { + Network::FilterFactoryCb createFilterFactory( + const Json::Object& config_json, FactoryContext& context) override { TcpClientConfig config_pb; if (!Utils::ReadV2Config(config_json, &config_pb) && !Utils::ReadV1Config(config_json, &config_pb)) { @@ -37,7 +37,7 @@ class FilterFactory : public NamedNetworkFilterConfigFactory { return createFilterFactory(config_pb, context); } - NetworkFilterFactoryCb createFilterFactoryFromProto( + Network::FilterFactoryCb createFilterFactoryFromProto( const Protobuf::Message& config, FactoryContext& context) override { return createFilterFactory(dynamic_cast(config), context); @@ -50,8 +50,8 @@ class FilterFactory : public NamedNetworkFilterConfigFactory { std::string name() override { return "mixer"; } private: - NetworkFilterFactoryCb createFilterFactory(const TcpClientConfig& config_pb, - FactoryContext& context) { + Network::FilterFactoryCb createFilterFactory(const TcpClientConfig& config_pb, + FactoryContext& context) { std::unique_ptr config_obj( new Tcp::Mixer::Config(config_pb)); diff --git a/src/envoy/utils/grpc_transport.cc b/src/envoy/utils/grpc_transport.cc index fe7923a2a48..8c2829f13ae 100644 --- a/src/envoy/utils/grpc_transport.cc +++ b/src/envoy/utils/grpc_transport.cc @@ -25,59 +25,23 @@ namespace { // gRPC request timeout const std::chrono::milliseconds kGrpcRequestTimeoutMs(5000); -// HTTP trace headers that should pass to gRPC metadata from origin request. -// x-request-id is added for easy debugging. -const Http::LowerCaseString kRequestId("x-request-id"); -const Http::LowerCaseString kB3TraceId("x-b3-traceid"); -const Http::LowerCaseString kB3SpanId("x-b3-spanid"); -const Http::LowerCaseString kB3ParentSpanId("x-b3-parentspanid"); -const Http::LowerCaseString kB3Sampled("x-b3-sampled"); -const Http::LowerCaseString kB3Flags("x-b3-flags"); -const Http::LowerCaseString kOtSpanContext("x-ot-span-context"); - -inline void CopyHeaderEntry(const Http::HeaderEntry *entry, - const Http::LowerCaseString &key, - Http::HeaderMap &headers) { - if (entry) { - std::string val(entry->value().c_str(), entry->value().size()); - headers.addReferenceKey(key, val); - } -} - } // namespace template GrpcTransport::GrpcTransport( Grpc::AsyncClientPtr async_client, const RequestType &request, - const Http::HeaderMap *headers, ResponseType *response, + ResponseType *response, Tracing::Span &parent_span, istio::mixerclient::DoneFunc on_done) : async_client_(std::move(async_client)), - headers_(headers), response_(response), on_done_(on_done), request_(async_client_->send( - descriptor(), request, *this, Tracing::NullSpan::instance(), + descriptor(), request, *this, parent_span, absl::optional(kGrpcRequestTimeoutMs))) { ENVOY_LOG(debug, "Sending {} request: {}", descriptor().name(), request.DebugString()); } -template -void GrpcTransport::onCreateInitialMetadata( - Http::HeaderMap &metadata) { - if (!headers_) return; - - CopyHeaderEntry(headers_->RequestId(), kRequestId, metadata); - CopyHeaderEntry(headers_->XB3TraceId(), kB3TraceId, metadata); - CopyHeaderEntry(headers_->XB3SpanId(), kB3SpanId, metadata); - CopyHeaderEntry(headers_->XB3ParentSpanId(), kB3ParentSpanId, metadata); - CopyHeaderEntry(headers_->XB3Sampled(), kB3Sampled, metadata); - CopyHeaderEntry(headers_->XB3Flags(), kB3Flags, metadata); - - // This one is NOT inline, need to do linar search. - CopyHeaderEntry(headers_->get(kOtSpanContext), kOtSpanContext, metadata); -} - template void GrpcTransport::onSuccess( std::unique_ptr &&response, Tracing::Span &) { @@ -107,12 +71,13 @@ void GrpcTransport::Cancel() { template typename GrpcTransport::Func GrpcTransport::GetFunc( - Grpc::AsyncClientFactory &factory, const Http::HeaderMap *headers) { - return [&factory, headers](const RequestType &request, ResponseType *response, - istio::mixerclient::DoneFunc on_done) + Grpc::AsyncClientFactory &factory, Tracing::Span &parent_span) { + return [&factory, &parent_span](const RequestType &request, + ResponseType *response, + istio::mixerclient::DoneFunc on_done) -> istio::mixerclient::CancelFunc { auto transport = new GrpcTransport( - factory.create(), request, headers, response, on_done); + factory.create(), request, response, parent_span, on_done); return [transport]() { transport->Cancel(); }; }; } @@ -137,9 +102,9 @@ const google::protobuf::MethodDescriptor &ReportTransport::descriptor() { // explicitly instantiate CheckTransport and ReportTransport template CheckTransport::Func CheckTransport::GetFunc( - Grpc::AsyncClientFactory &factory, const Http::HeaderMap *headers); + Grpc::AsyncClientFactory &factory, Tracing::Span &parent_span); template ReportTransport::Func ReportTransport::GetFunc( - Grpc::AsyncClientFactory &factory, const Http::HeaderMap *headers); + Grpc::AsyncClientFactory &factory, Tracing::Span &parent_span); } // namespace Utils } // namespace Envoy diff --git a/src/envoy/utils/grpc_transport.h b/src/envoy/utils/grpc_transport.h index 2cb60834bec..41bbffe5407 100644 --- a/src/envoy/utils/grpc_transport.h +++ b/src/envoy/utils/grpc_transport.h @@ -39,14 +39,18 @@ class GrpcTransport : public Grpc::TypedAsyncRequestCallbacks, istio::mixerclient::DoneFunc on_done)>; static Func GetFunc(Grpc::AsyncClientFactory& factory, - const Http::HeaderMap* headers = nullptr); + Tracing::Span& parent_span); GrpcTransport(Grpc::AsyncClientPtr async_client, const RequestType& request, - const Http::HeaderMap* headers, ResponseType* response, + ResponseType* response, Tracing::Span& parent_span, istio::mixerclient::DoneFunc on_done); - // Grpc::AsyncRequestCallbacks - void onCreateInitialMetadata(Http::HeaderMap& metadata) override; + void onCreateInitialMetadata(Http::HeaderMap& metadata) override { + // We generate cluster name contains invalid characters, so override the + // authority header temorarily until it can be specified via CDS. + // See https://github.com/envoyproxy/envoy/issues/3297 for details. + metadata.Host()->value("mixer", 5); + } void onSuccess(std::unique_ptr&& response, Tracing::Span& span) override; @@ -60,7 +64,6 @@ class GrpcTransport : public Grpc::TypedAsyncRequestCallbacks, static const google::protobuf::MethodDescriptor& descriptor(); Grpc::AsyncClientPtr async_client_; - const Http::HeaderMap* headers_; ResponseType* response_; ::istio::mixerclient::DoneFunc on_done_; Grpc::AsyncRequest* request_{}; diff --git a/src/envoy/utils/mixer_control.cc b/src/envoy/utils/mixer_control.cc index 01ccaac74b5..082dde704b5 100644 --- a/src/envoy/utils/mixer_control.cc +++ b/src/envoy/utils/mixer_control.cc @@ -61,8 +61,10 @@ void CreateEnvironment(Event::Dispatcher &dispatcher, Grpc::AsyncClientFactory &check_client_factory, Grpc::AsyncClientFactory &report_client_factory, ::istio::mixerclient::Environment *env) { - env->check_transport = CheckTransport::GetFunc(check_client_factory, nullptr); - env->report_transport = ReportTransport::GetFunc(report_client_factory); + env->check_transport = CheckTransport::GetFunc(check_client_factory, + Tracing::NullSpan::instance()); + env->report_transport = ReportTransport::GetFunc( + report_client_factory, Tracing::NullSpan::instance()); env->timer_create_func = [&dispatcher](std::function timer_cb) -> std::unique_ptr<::istio::mixerclient::Timer> { diff --git a/src/istio/authn/context.proto b/src/istio/authn/context.proto index d87385b5d0f..5ea3fdb67d5 100644 --- a/src/istio/authn/context.proto +++ b/src/istio/authn/context.proto @@ -35,6 +35,11 @@ message JwtPayload { // Only raw JWT string claims are kept. map claims = 5; + + // All original claims in JsonString format, which can be parsed into json + // object (map) to access other claims that not cover with the string claims + // map above. + string raw_claims = 6; } // Container to hold authenticated attributes from X509 (mTLS). diff --git a/src/istio/control/attribute_names.cc b/src/istio/control/attribute_names.cc index 3f0326a84af..953957998a9 100644 --- a/src/istio/control/attribute_names.cc +++ b/src/istio/control/attribute_names.cc @@ -78,6 +78,7 @@ const char AttributeName::kRequestAuthPrincipal[] = "request.auth.principal"; const char AttributeName::kRequestAuthAudiences[] = "request.auth.audiences"; const char AttributeName::kRequestAuthPresenter[] = "request.auth.presenter"; const char AttributeName::kRequestAuthClaims[] = "request.auth.claims"; +const char AttributeName::kRequestAuthRawClaims[] = "request.auth.raw_claims"; } // namespace control } // namespace istio diff --git a/src/istio/control/attribute_names.h b/src/istio/control/attribute_names.h index 09bb1488e25..2ed3dfdf46a 100644 --- a/src/istio/control/attribute_names.h +++ b/src/istio/control/attribute_names.h @@ -85,6 +85,7 @@ struct AttributeName { static const char kRequestAuthAudiences[]; static const char kRequestAuthPresenter[]; static const char kRequestAuthClaims[]; + static const char kRequestAuthRawClaims[]; }; } // namespace control diff --git a/src/istio/control/http/attributes_builder.cc b/src/istio/control/http/attributes_builder.cc index 4aac9a7d468..d85bc4f2bcf 100644 --- a/src/istio/control/http/attributes_builder.cc +++ b/src/istio/control/http/attributes_builder.cc @@ -89,6 +89,10 @@ void AttributesBuilder::ExtractAuthAttributes(CheckData *check_data) { builder.AddProtobufStringMap(AttributeName::kRequestAuthClaims, origin.claims()); } + if (!origin.raw_claims().empty()) { + builder.AddString(AttributeName::kRequestAuthRawClaims, + origin.raw_claims()); + } } return; } diff --git a/src/istio/control/http/attributes_builder_test.cc b/src/istio/control/http/attributes_builder_test.cc index 958e4d3f5a2..9ac9019442e 100644 --- a/src/istio/control/http/attributes_builder_test.cc +++ b/src/istio/control/http/attributes_builder_test.cc @@ -398,6 +398,7 @@ TEST(AttributesBuilderTest, TestCheckAttributesWithAuthNResult) { "thisisemail@email.com"; (*result->mutable_origin()->mutable_claims())["iat"] = "1512754205"; (*result->mutable_origin()->mutable_claims())["exp"] = "5112754205"; + result->mutable_origin()->set_raw_claims("test_raw_claims"); return true; })); @@ -414,6 +415,14 @@ TEST(AttributesBuilderTest, TestCheckAttributesWithAuthNResult) { Attributes expected_attributes; ASSERT_TRUE( TextFormat::ParseFromString(kCheckAttributes, &expected_attributes)); + // kCheckAttributes is also used in TestCheckAttributes, which is a deprecated + // way to construct mixer attribute (it was a fallback when authn filter is + // not available, which can be removed after 0.8). For now, modifying expected + // data manually for this test. + (*expected_attributes + .mutable_attributes())[AttributeName::kRequestAuthRawClaims] + .set_string_value("test_raw_claims"); + EXPECT_TRUE( MessageDifferencer::Equals(request.attributes, expected_attributes)); } diff --git a/tools/bazel.rc b/tools/bazel.rc index 0038dfd79ff..4ce27056965 100644 --- a/tools/bazel.rc +++ b/tools/bazel.rc @@ -49,6 +49,7 @@ test --test_env=HEAPCHECK=normal --test_env=PPROF_PATH # Release builds build:release -c opt +build:release --strip=always # Add compile option for all C++ files build --cxxopt -Wnon-virtual-dtor