diff --git a/source/extensions/filters/http/basic_auth/basic_auth_filter.cc b/source/extensions/filters/http/basic_auth/basic_auth_filter.cc index ae7b10e6c573..d9946ae45851 100644 --- a/source/extensions/filters/http/basic_auth/basic_auth_filter.cc +++ b/source/extensions/filters/http/basic_auth/basic_auth_filter.cc @@ -26,13 +26,12 @@ std::string computeSHA1(absl::string_view password) { } // namespace -FilterConfig::FilterConfig(UserMapConstPtr users, const std::string& stats_prefix, - Stats::Scope& scope) +FilterConfig::FilterConfig(UserMap&& users, const std::string& stats_prefix, Stats::Scope& scope) : users_(std::move(users)), stats_(generateStats(stats_prefix + "basic_auth.", scope)) {} bool FilterConfig::validateUser(absl::string_view username, absl::string_view password) const { - auto user = users_->find(username); - if (user == users_->end()) { + auto user = users_.find(username); + if (user == users_.end()) { return false; } @@ -43,46 +42,49 @@ BasicAuthFilter::BasicAuthFilter(FilterConfigConstSharedPtr config) : config_(st Http::FilterHeadersStatus BasicAuthFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { auto auth_header = headers.get(Http::CustomHeaders::get().Authorization); - if (!auth_header.empty()) { - absl::string_view auth_value = auth_header[0]->value().getStringView(); - - if (absl::StartsWith(auth_value, "Basic ")) { - // Extract and decode the Base64 part of the header. - absl::string_view base64Token = auth_value.substr(6); - const std::string decoded = Base64::decodeWithoutPadding(base64Token); - - // The decoded string is in the format "username:password". - const size_t colon_pos = decoded.find(':'); - - if (colon_pos != std::string::npos) { - absl::string_view decoded_view = decoded; - absl::string_view username = decoded_view.substr(0, colon_pos); - absl::string_view password = decoded_view.substr(colon_pos + 1); - - if (config_->validateUser(username, password)) { - config_->stats().allowed_.inc(); - return Http::FilterHeadersStatus::Continue; - } else { - config_->stats().denied_.inc(); - decoder_callbacks_->sendLocalReply( - Http::Code::Unauthorized, - "User authentication failed. Invalid username/password combination", nullptr, - absl::nullopt, "invalid_credential_for_basic_auth"); - return Http::FilterHeadersStatus::StopIteration; - } - } - } + + if (auth_header.empty()) { + return onDenied("User authentication failed. Missing username and password.", + "no_credential_for_basic_auth"); } - config_->stats().denied_.inc(); - decoder_callbacks_->sendLocalReply(Http::Code::Unauthorized, - "User authentication failed. Missing username and password", - nullptr, absl::nullopt, "no_credential_for_basic_auth"); - return Http::FilterHeadersStatus::StopIteration; + absl::string_view auth_value = auth_header[0]->value().getStringView(); + + if (!absl::StartsWith(auth_value, "Basic ")) { + return onDenied("User authentication failed. Expected 'Basic' authentication scheme.", + "invalid_scheme_for_basic_auth"); + } + + // Extract and decode the Base64 part of the header. + absl::string_view base64_token = auth_value.substr(6); + const std::string decoded = Base64::decodeWithoutPadding(base64_token); + + // The decoded string is in the format "username:password". + const size_t colon_pos = decoded.find(':'); + if (colon_pos == std::string::npos) { + return onDenied("User authentication failed. Invalid basic credential format.", + "invalid_format_for_basic_auth"); + } + + absl::string_view decoded_view = decoded; + absl::string_view username = decoded_view.substr(0, colon_pos); + absl::string_view password = decoded_view.substr(colon_pos + 1); + + if (!config_->validateUser(username, password)) { + return onDenied("User authentication failed. Invalid username/password combination.", + "invalid_credential_for_basic_auth"); + } + + config_->stats().allowed_.inc(); + return Http::FilterHeadersStatus::Continue; } -void BasicAuthFilter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { - decoder_callbacks_ = &callbacks; +Http::FilterHeadersStatus BasicAuthFilter::onDenied(absl::string_view body, + absl::string_view response_code_details) { + config_->stats().denied_.inc(); + decoder_callbacks_->sendLocalReply(Http::Code::Unauthorized, body, nullptr, absl::nullopt, + response_code_details); + return Http::FilterHeadersStatus::StopIteration; } } // namespace BasicAuth diff --git a/source/extensions/filters/http/basic_auth/basic_auth_filter.h b/source/extensions/filters/http/basic_auth/basic_auth_filter.h index d900b304eb67..38553f655eb1 100644 --- a/source/extensions/filters/http/basic_auth/basic_auth_filter.h +++ b/source/extensions/filters/http/basic_auth/basic_auth_filter.h @@ -36,15 +36,14 @@ struct User { std::string hash; }; -using UserMapConstPtr = - std::unique_ptr>; // username, User +using UserMap = absl::flat_hash_map; /** * Configuration for the Basic Auth filter. */ class FilterConfig { public: - FilterConfig(UserMapConstPtr users, const std::string& stats_prefix, Stats::Scope& scope); + FilterConfig(UserMap&& users, const std::string& stats_prefix, Stats::Scope& scope); const BasicAuthStats& stats() const { return stats_; } bool validateUser(absl::string_view username, absl::string_view password) const; @@ -53,7 +52,7 @@ class FilterConfig { return BasicAuthStats{ALL_BASIC_AUTH_STATS(POOL_COUNTER_PREFIX(scope, prefix))}; } - UserMapConstPtr users_; + const UserMap users_; BasicAuthStats stats_; }; using FilterConfigConstSharedPtr = std::shared_ptr; @@ -66,11 +65,12 @@ class BasicAuthFilter : public Http::PassThroughDecoderFilter, // Http::StreamDecoderFilter Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, bool) override; - void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override; private: + Http::FilterHeadersStatus onDenied(absl::string_view body, + absl::string_view response_code_details); + // The callback function. - Http::StreamDecoderFilterCallbacks* decoder_callbacks_; FilterConfigConstSharedPtr config_; }; diff --git a/source/extensions/filters/http/basic_auth/config.cc b/source/extensions/filters/http/basic_auth/config.cc index 02a3582cad69..429ba32c3f5f 100644 --- a/source/extensions/filters/http/basic_auth/config.cc +++ b/source/extensions/filters/http/basic_auth/config.cc @@ -12,36 +12,47 @@ using envoy::extensions::filters::http::basic_auth::v3::BasicAuth; namespace { -UserMapConstPtr readHtpasswd(const std::string& htpasswd) { - std::unique_ptr> users = - std::make_unique>(); +UserMap readHtpasswd(const std::string& htpasswd) { + UserMap users; + std::istringstream htpsswd_ss(htpasswd); std::string line; while (std::getline(htpsswd_ss, line)) { + // TODO(wbpcode): should we trim the spaces or empty chars? + + // Skip empty lines and comments. + if (line.empty() || line[0] == '#') { + continue; + } + const size_t colon_pos = line.find(':'); + if (colon_pos == std::string::npos) { + throw EnvoyException("basic auth: invalid htpasswd format, username:password is expected"); + } - if (colon_pos != std::string::npos) { - std::string name = line.substr(0, colon_pos); - std::string hash = line.substr(colon_pos + 1); + std::string name = line.substr(0, colon_pos); + std::string hash = line.substr(colon_pos + 1); - if (name.empty()) { - throw EnvoyException("basic auth: invalid user name"); - } + if (name.empty() || hash.empty()) { + throw EnvoyException("basic auth: empty user name or password"); + } + + if (users.contains(name)) { + throw EnvoyException("basic auth: duplicate users"); + } - if (absl::StartsWith(hash, "{SHA}")) { - hash = hash.substr(5); - // The base64 encoded SHA1 hash is 28 bytes long - if (hash.length() != 28) { - throw EnvoyException("basic auth: invalid SHA hash length"); - } + if (!absl::StartsWith(hash, "{SHA}")) { + throw EnvoyException("basic auth: unsupported htpasswd format: please use {SHA}"); + } - users->insert({name, {name, hash}}); - continue; - } + hash = hash.substr(5); + // The base64 encoded SHA1 hash is 28 bytes long + if (hash.length() != 28) { + throw EnvoyException("basic auth: invalid htpasswd format, invalid SHA hash length"); } - throw EnvoyException("basic auth: unsupported htpasswd format: please use {SHA}"); + users.insert({name, {name, hash}}); } return users; @@ -52,8 +63,8 @@ UserMapConstPtr readHtpasswd(const std::string& htpasswd) { Http::FilterFactoryCb BasicAuthFilterFactory::createFilterFactoryFromProtoTyped( const BasicAuth& proto_config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { - const std::string htpasswd = Config::DataSource::read(proto_config.users(), false, context.api()); - UserMapConstPtr users = readHtpasswd(htpasswd); + UserMap users = + readHtpasswd(Config::DataSource::read(proto_config.users(), false, context.api())); FilterConfigConstSharedPtr config = std::make_unique(std::move(users), stats_prefix, context.scope()); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { diff --git a/test/extensions/filters/http/basic_auth/basic_auth_integration_test.cc b/test/extensions/filters/http/basic_auth/basic_auth_integration_test.cc index 2e70bf9efb5b..c66d98084b1f 100644 --- a/test/extensions/filters/http/basic_auth/basic_auth_integration_test.cc +++ b/test/extensions/filters/http/basic_auth/basic_auth_integration_test.cc @@ -72,7 +72,7 @@ TEST_P(BasicAuthIntegrationTestAllProtocols, NoCredential) { ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("401", response->headers().getStatusValue()); - EXPECT_EQ("User authentication failed. Missing username and password", response->body()); + EXPECT_EQ("User authentication failed. Missing username and password.", response->body()); } // Request without wrong password @@ -91,7 +91,7 @@ TEST_P(BasicAuthIntegrationTestAllProtocols, WrongPasswrod) { ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("401", response->headers().getStatusValue()); - EXPECT_EQ("User authentication failed. Invalid username/password combination", response->body()); + EXPECT_EQ("User authentication failed. Invalid username/password combination.", response->body()); } // Request with none-existed user @@ -110,7 +110,7 @@ TEST_P(BasicAuthIntegrationTestAllProtocols, NoneExistedUser) { ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("401", response->headers().getStatusValue()); - EXPECT_EQ("User authentication failed. Invalid username/password combination", response->body()); + EXPECT_EQ("User authentication failed. Invalid username/password combination.", response->body()); } } // namespace } // namespace BasicAuth diff --git a/test/extensions/filters/http/basic_auth/config_test.cc b/test/extensions/filters/http/basic_auth/config_test.cc index 2be2d5812596..61a3f655547d 100644 --- a/test/extensions/filters/http/basic_auth/config_test.cc +++ b/test/extensions/filters/http/basic_auth/config_test.cc @@ -15,6 +15,7 @@ TEST(Factory, ValidConfig) { const std::string yaml = R"( users: inline_string: |- + # comment line user1:{SHA}tESsBmE/yNY3lb6a0L6vVQEZNqw= user2:{SHA}EJ9LPFDXsN9ynSmbxvjp75Bmlx8= )"; @@ -45,8 +46,27 @@ TEST(Factory, InvalidConfigNoColon) { NiceMock context; - EXPECT_THROW(factory.createFilterFactoryFromProto(*proto_config, "stats", context), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE(factory.createFilterFactoryFromProto(*proto_config, "stats", context), + EnvoyException, + "basic auth: invalid htpasswd format, username:password is expected"); +} + +TEST(Factory, InvalidConfigDuplicateUsers) { + const std::string yaml = R"( + users: + inline_string: |- + user1:{SHA}tESsBmE/yNY3lb6a0L6vVQEZNqw= + user1:{SHA}EJ9LPFDXsN9ynSmbxvjp75Bmlx8= + )"; + + BasicAuthFilterFactory factory; + ProtobufTypes::MessagePtr proto_config = factory.createEmptyRouteConfigProto(); + TestUtility::loadFromYaml(yaml, *proto_config); + + NiceMock context; + + EXPECT_THROW_WITH_MESSAGE(factory.createFilterFactoryFromProto(*proto_config, "stats", context), + EnvoyException, "basic auth: duplicate users"); } TEST(Factory, InvalidConfigNoUser) { @@ -63,8 +83,8 @@ TEST(Factory, InvalidConfigNoUser) { NiceMock context; - EXPECT_THROW(factory.createFilterFactoryFromProto(*proto_config, "stats", context), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE(factory.createFilterFactoryFromProto(*proto_config, "stats", context), + EnvoyException, "basic auth: empty user name or password"); } TEST(Factory, InvalidConfigNoPassword) { @@ -81,8 +101,8 @@ TEST(Factory, InvalidConfigNoPassword) { NiceMock context; - EXPECT_THROW(factory.createFilterFactoryFromProto(*proto_config, "stats", context), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE(factory.createFilterFactoryFromProto(*proto_config, "stats", context), + EnvoyException, "basic auth: empty user name or password"); } TEST(Factory, InvalidConfigNoHash) { @@ -99,8 +119,9 @@ TEST(Factory, InvalidConfigNoHash) { NiceMock context; - EXPECT_THROW(factory.createFilterFactoryFromProto(*proto_config, "stats", context), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE(factory.createFilterFactoryFromProto(*proto_config, "stats", context), + EnvoyException, + "basic auth: invalid htpasswd format, invalid SHA hash length"); } TEST(Factory, InvalidConfigNotSHA) { @@ -117,8 +138,9 @@ TEST(Factory, InvalidConfigNotSHA) { NiceMock context; - EXPECT_THROW(factory.createFilterFactoryFromProto(*proto_config, "stats", context), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE(factory.createFilterFactoryFromProto(*proto_config, "stats", context), + EnvoyException, + "basic auth: unsupported htpasswd format: please use {SHA}"); } } // namespace BasicAuth diff --git a/test/extensions/filters/http/basic_auth/filter_test.cc b/test/extensions/filters/http/basic_auth/filter_test.cc index d8d3f3000be4..375c005705bc 100644 --- a/test/extensions/filters/http/basic_auth/filter_test.cc +++ b/test/extensions/filters/http/basic_auth/filter_test.cc @@ -15,10 +15,9 @@ namespace BasicAuth { class FilterTest : public testing::Test { public: FilterTest() { - std::unique_ptr> users = - std::make_unique>(); - users->insert({"user1", {"user1", "tESsBmE/yNY3lb6a0L6vVQEZNqw="}}); // user1:test1 - users->insert({"user2", {"user2", "EJ9LPFDXsN9ynSmbxvjp75Bmlx8="}}); // user2:test2 + UserMap users; + users.insert({"user1", {"user1", "tESsBmE/yNY3lb6a0L6vVQEZNqw="}}); // user1:test1 + users.insert({"user2", {"user2", "EJ9LPFDXsN9ynSmbxvjp75Bmlx8="}}); // user2:test2 config_ = std::make_unique(std::move(users), "stats", *stats_.rootScope()); filter_ = std::make_shared(config_); filter_->setDecoderFilterCallbacks(decoder_filter_callbacks_); @@ -54,7 +53,7 @@ TEST_F(FilterTest, UserNotExist) { const absl::optional grpc_status, absl::string_view details) { EXPECT_EQ(Http::Code::Unauthorized, code); - EXPECT_EQ("User authentication failed. Invalid username/password combination", body); + EXPECT_EQ("User authentication failed. Invalid username/password combination.", body); EXPECT_EQ(grpc_status, absl::nullopt); EXPECT_EQ(details, "invalid_credential_for_basic_auth"); })); @@ -72,7 +71,7 @@ TEST_F(FilterTest, InvalidPassword) { const absl::optional grpc_status, absl::string_view details) { EXPECT_EQ(Http::Code::Unauthorized, code); - EXPECT_EQ("User authentication failed. Invalid username/password combination", body); + EXPECT_EQ("User authentication failed. Invalid username/password combination.", body); EXPECT_EQ(grpc_status, absl::nullopt); EXPECT_EQ(details, "invalid_credential_for_basic_auth"); })); @@ -89,7 +88,7 @@ TEST_F(FilterTest, NoAuthHeader) { const absl::optional grpc_status, absl::string_view details) { EXPECT_EQ(Http::Code::Unauthorized, code); - EXPECT_EQ("User authentication failed. Missing username and password", body); + EXPECT_EQ("User authentication failed. Missing username and password.", body); EXPECT_EQ(grpc_status, absl::nullopt); EXPECT_EQ(details, "no_credential_for_basic_auth"); })); @@ -97,6 +96,40 @@ TEST_F(FilterTest, NoAuthHeader) { filter_->decodeHeaders(request_headers_user1, true)); } +TEST_F(FilterTest, HasAuthHeaderButNotForBasic) { + Http::TestRequestHeaderMapImpl request_headers_user1{{"Authorization", "Bearer xxxxxxx"}}; + + EXPECT_CALL(decoder_filter_callbacks_, sendLocalReply(_, _, _, _, _)) + .WillOnce(Invoke([&](Http::Code code, absl::string_view body, + std::function, + const absl::optional grpc_status, + absl::string_view details) { + EXPECT_EQ(Http::Code::Unauthorized, code); + EXPECT_EQ("User authentication failed. Expected 'Basic' authentication scheme.", body); + EXPECT_EQ(grpc_status, absl::nullopt); + EXPECT_EQ(details, "invalid_scheme_for_basic_auth"); + })); + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_->decodeHeaders(request_headers_user1, true)); +} + +TEST_F(FilterTest, HasAuthHeaderButNoColon) { + Http::TestRequestHeaderMapImpl request_headers_user1{{"Authorization", "Basic dXNlcjE="}}; + + EXPECT_CALL(decoder_filter_callbacks_, sendLocalReply(_, _, _, _, _)) + .WillOnce(Invoke([&](Http::Code code, absl::string_view body, + std::function, + const absl::optional grpc_status, + absl::string_view details) { + EXPECT_EQ(Http::Code::Unauthorized, code); + EXPECT_EQ("User authentication failed. Invalid basic credential format.", body); + EXPECT_EQ(grpc_status, absl::nullopt); + EXPECT_EQ(details, "invalid_format_for_basic_auth"); + })); + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_->decodeHeaders(request_headers_user1, true)); +} + } // namespace BasicAuth } // namespace HttpFilters } // namespace Extensions