Chiebot-Mirror
/
grpc
8
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

HTTP: Conditionally allow PUT requests (#29397)

Browse Source 
* Maybe fix for PUT deprecation

* Guard PUT request accepting with a flag and add tests

* Reviewer comments

* Add fallthrough notation

* Reviewer comments

Co-authored-by: Craig Tiller <ctiller@google.com>
pull/29400/head
Yash Tibrewal 3 years ago committed by GitHub
parent 5a3cd992b0
commit 5e989cf78d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 85 additions and 15 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 18
      src/core/ext/filters/http/client/http_client_filter.cc
  2. 8
      src/core/ext/filters/http/client/http_client_filter.h
  3. 13
      src/core/ext/filters/http/server/http_server_filter.cc
  4. 6
      src/core/ext/filters/http/server/http_server_filter.h
  5. 6
      src/core/ext/transport/chttp2/transport/hpack_encoder.cc
  6. 1
      src/core/ext/transport/cronet/transport/cronet_transport.cc
  7. 7
      src/core/lib/transport/metadata_batch.h
  8. 41
      test/cpp/end2end/xds/xds_end2end_test.cc

18
src/core/ext/filters/http/client/http_client_filter.cc
Unescape View File

@ -97,7 +97,11 @@ Slice UserAgentFromArgs(const ChannelArgs& args, const char* transport_name) {
ArenaPromise<ServerMetadataHandle> HttpClientFilter::MakeCallPromise(
CallArgs call_args, NextPromiseFactory next_promise_factory) {
auto& md = call_args.client_initial_metadata;
md->Set(HttpMethodMetadata(), HttpMethodMetadata::kPost);
if (test_only_use_put_requests_) {
md->Set(HttpMethodMetadata(), HttpMethodMetadata::kPut);
} else {
md->Set(HttpMethodMetadata(), HttpMethodMetadata::kPost);
}
md->Set(HttpSchemeMetadata(), scheme_);
md->Set(TeMetadata(), TeMetadata::kTrailers);
md->Set(ContentTypeMetadata(), ContentTypeMetadata::kApplicationGrpc);
@ -125,8 +129,11 @@ ArenaPromise<ServerMetadataHandle> HttpClientFilter::MakeCallPromise(
}
HttpClientFilter::HttpClientFilter(HttpSchemeMetadata::ValueType scheme,
Slice user_agent)
: scheme_(scheme), user_agent_(std::move(user_agent)) {}
Slice user_agent,
bool test_only_use_put_requests)
: scheme_(scheme),
user_agent_(std::move(user_agent)),
test_only_use_put_requests_(test_only_use_put_requests) {}
absl::StatusOr<HttpClientFilter> HttpClientFilter::Create(ChannelArgs args,
ChannelFilter::Args) {
@ -134,8 +141,9 @@ absl::StatusOr<HttpClientFilter> HttpClientFilter::Create(ChannelArgs args,
if (transport == nullptr) {
return absl::InvalidArgumentError("HttpClientFilter needs a transport");
}
return HttpClientFilter(SchemeFromArgs(args),
UserAgentFromArgs(args, transport->vtable->name));
return HttpClientFilter(
SchemeFromArgs(args), UserAgentFromArgs(args, transport->vtable->name),
args.GetInt(GRPC_ARG_TEST_ONLY_USE_PUT_REQUESTS).value_or(false));
}
} // namespace grpc_core

8
src/core/ext/filters/http/client/http_client_filter.h
Unescape View File

@ -37,12 +37,18 @@ class HttpClientFilter : public ChannelFilter {
CallArgs call_args, NextPromiseFactory next_promise_factory) override;
private:
HttpClientFilter(HttpSchemeMetadata::ValueType scheme, Slice user_agent);
HttpClientFilter(HttpSchemeMetadata::ValueType scheme, Slice user_agent,
bool test_only_use_put_requests);
HttpSchemeMetadata::ValueType scheme_;
Slice user_agent_;
bool test_only_use_put_requests_;
};
// A test-only channel arg to allow testing gRPC Core server behavior on PUT
// requests.
#define GRPC_ARG_TEST_ONLY_USE_PUT_REQUESTS "grpc.testing.use_put_requests"
} // namespace grpc_core
#endif /* GRPC_CORE_EXT_FILTERS_HTTP_CLIENT_HTTP_CLIENT_FILTER_H */

13
src/core/ext/filters/http/server/http_server_filter.cc
Unescape View File

@ -40,6 +40,8 @@ static void hs_recv_trailing_metadata_ready(void* user_data,
namespace {
bool g_allow_put_requests;
struct call_data {
call_data(grpc_call_element* elem, const grpc_call_element_args& args)
: call_combiner(args.call_combiner) {
@ -76,6 +78,14 @@ struct channel_data {
} // namespace
namespace grpc_core {
void InternalOnlyDoNotUseUnlessYouHavePermissionFromGrpcTeamAllowBrokenPutRequests() {
g_allow_put_requests = true;
}
} // namespace grpc_core
static grpc_error_handle hs_filter_outgoing_metadata(grpc_metadata_batch* b) {
if (grpc_core::Slice* grpc_message =
b->get_pointer(grpc_core::GrpcMessageMetadata())) {
@ -104,6 +114,9 @@ static grpc_error_handle hs_filter_incoming_metadata(grpc_call_element* elem,
switch (*method) {
case grpc_core::HttpMethodMetadata::kPost:
break;
case grpc_core::HttpMethodMetadata::kPut:
if (g_allow_put_requests) break;
ABSL_FALLTHROUGH_INTENDED;
case grpc_core::HttpMethodMetadata::kInvalid:
case grpc_core::HttpMethodMetadata::kGet:
hs_add_error(error_name, &error,

6
src/core/ext/filters/http/server/http_server_filter.h
Unescape View File

@ -26,4 +26,10 @@
/* Processes metadata on the server side for HTTP2 transports */
extern const grpc_channel_filter grpc_http_server_filter;
namespace grpc_core {
// Temporary code that allows servers to accept PUT requests. DO NOT USE WITHOUT
// PERMISSION.
void InternalOnlyDoNotUseUnlessYouHavePermissionFromGrpcTeamAllowBrokenPutRequests();
} // namespace grpc_core
#endif /* GRPC_CORE_EXT_FILTERS_HTTP_SERVER_HTTP_SERVER_FILTER_H */

6
src/core/ext/transport/chttp2/transport/hpack_encoder.cc
Unescape View File

@ -487,6 +487,12 @@ void HPackCompressor::Framer::Encode(HttpMethodMetadata,
case HttpMethodMetadata::ValueType::kGet:
EmitIndexed(2); // :method: GET
break;
case HttpMethodMetadata::ValueType::kPut:
// Right now, we only emit PUT as a method for testing purposes, so it's
// fine to not index it.
EmitLitHdrWithNonBinaryStringKeyNotIdx(Slice::FromStaticString(":method"),
Slice::FromStaticString("PUT"));
break;
case HttpMethodMetadata::ValueType::kInvalid:
GPR_ASSERT(false);
break;

1
src/core/ext/transport/cronet/transport/cronet_transport.cc
Unescape View File

@ -741,6 +741,7 @@ class CronetMetadataEncoder {
break;
case grpc_core::HttpMethodMetadata::kInvalid:
case grpc_core::HttpMethodMetadata::kGet:
case grpc_core::HttpMethodMetadata::kPut:
abort();
}
}

7
src/core/lib/transport/metadata_batch.h
Unescape View File

@ -227,6 +227,7 @@ struct HttpMethodMetadata {
enum ValueType {
kPost,
kGet,
kPut,
kInvalid,
};
using MementoType = ValueType;
@ -236,6 +237,8 @@ struct HttpMethodMetadata {
auto value_string = value.as_string_view();
if (value_string == "POST") {
out = kPost;
} else if (value_string == "PUT") {
out = kPut;
} else if (value_string == "GET") {
out = kGet;
} else {
@ -250,6 +253,8 @@ struct HttpMethodMetadata {
switch (x) {
case kPost:
return StaticSlice::FromStaticString("POST");
case kPut:
return StaticSlice::FromStaticString("PUT");
case kGet:
return StaticSlice::FromStaticString("GET");
default:
@ -262,6 +267,8 @@ struct HttpMethodMetadata {
return "POST";
case kGet:
return "GET";
case kPut:
return "PUT";
default:
return "<discarded-invalid-value>";
}

41
test/cpp/end2end/xds/xds_end2end_test.cc
Unescape View File

@ -59,6 +59,8 @@
#include "src/core/ext/filters/client_channel/backup_poller.h"
#include "src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h"
#include "src/core/ext/filters/client_channel/resolver/fake/fake_resolver.h"
#include "src/core/ext/filters/http/client/http_client_filter.h"
#include "src/core/ext/filters/http/server/http_server_filter.h"
#include "src/core/ext/xds/certificate_provider_registry.h"
#include "src/core/ext/xds/xds_api.h"
#include "src/core/ext/xds/xds_channel_args.h"
@ -7576,12 +7578,16 @@ class XdsServerSecurityTest : public XdsEnd2endTest {
return CreateCustomChannel(uri, channel_creds, args);
}
std::shared_ptr<grpc::Channel> CreateInsecureChannel() {
std::shared_ptr<grpc::Channel> CreateInsecureChannel(
bool use_put_requests = false) {
ChannelArguments args;
// Override target name for host name check
args.SetString(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG,
ipv6_only_ ? "::1" : "127.0.0.1");
args.SetInt(GRPC_ARG_USE_LOCAL_SUBCHANNEL_POOL, 1);
if (use_put_requests) {
args.SetInt(GRPC_ARG_TEST_ONLY_USE_PUT_REQUESTS, 1);
}
std::string uri = absl::StrCat(
ipv6_only_ ? "ipv6:[::1]:" : "ipv4:127.0.0.1:", backends_[0]->port());
return CreateCustomChannel(uri, InsecureChannelCredentials(), args);
@ -9384,8 +9390,11 @@ TEST_P(XdsRbacTestWithActionPermutations, MethodPostPermissionAnyPrincipal) {
SendRpc([this]() { return CreateInsecureChannel(); }, {}, {},
/*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_DENY,
grpc::StatusCode::PERMISSION_DENIED);
// TODO(yashykt): When we start supporting GET/PUT requests in the future,
// this should be modified to test that they are NOT accepted with this rule.
// Test that an RPC with PUT method is handled properly.
SendRpc([this]() { return CreateInsecureChannel(/*use_put_requests=*/true); },
{}, {},
/*test_expects_failure=*/GetParam().rbac_action() != RBAC_Action_DENY,
grpc::StatusCode::PERMISSION_DENIED);
}
TEST_P(XdsRbacTestWithActionPermutations, MethodGetPermissionAnyPrincipal) {
@ -9432,8 +9441,12 @@ TEST_P(XdsRbacTestWithActionPermutations, MethodPutPermissionAnyPrincipal) {
[this]() { return CreateInsecureChannel(); }, {}, {},
/*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_ALLOW,
grpc::StatusCode::PERMISSION_DENIED);
// TODO(yashykt): When we start supporting PUT requests in the future, this
// should be modified to test that they are accepted with this rule.
// Test that an RPC with a PUT method gets accepted
SendRpc(
[this]() { return CreateInsecureChannel(/*use_put_requests=*/true); }, {},
{},
/*test_expects_failure=*/GetParam().rbac_action() != RBAC_Action_ALLOW,
grpc::StatusCode::PERMISSION_DENIED);
}
TEST_P(XdsRbacTestWithActionPermutations, UrlPathPermissionAnyPrincipal) {
@ -9688,8 +9701,11 @@ TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionMethodPostPrincipal) {
SendRpc([this]() { return CreateInsecureChannel(); }, {}, {},
/*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_DENY,
grpc::StatusCode::PERMISSION_DENIED);
// TODO(yashykt): When we start supporting GET/PUT requests in the future,
// this should be modified to test that they are NOT accepted with this rule.
// Test that an RPC with PUT method is handled properly.
SendRpc([this]() { return CreateInsecureChannel(/*use_put_requests=*/true); },
{}, {},
/*test_expects_failure=*/GetParam().rbac_action() != RBAC_Action_DENY,
grpc::StatusCode::PERMISSION_DENIED);
}
TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionMethodGetPrincipal) {
@ -9731,13 +9747,17 @@ TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionMethodPutPrincipal) {
backends_[0]->notifier()->WaitOnServingStatusChange(
absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()),
grpc::StatusCode::OK);
// Test that an RPC with a PUT method gets accepted
SendRpc(
[this]() { return CreateInsecureChannel(/*use_put_requests=*/true); }, {},
{},
/*test_expects_failure=*/GetParam().rbac_action() != RBAC_Action_ALLOW,
grpc::StatusCode::PERMISSION_DENIED);
// Test that an RPC with a POST method gets rejected
SendRpc(
[this]() { return CreateInsecureChannel(); }, {}, {},
/*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_ALLOW,
grpc::StatusCode::PERMISSION_DENIED);
// TODO(yashykt): When we start supporting PUT requests in the future, this
// should be modified to test that they are accepted with this rule.
}
TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionUrlPathPrincipal) {
@ -12008,6 +12028,9 @@ int main(int argc, char** argv) {
// Make the backup poller poll very frequently in order to pick up
// updates from all the subchannels's FDs.
GPR_GLOBAL_CONFIG_SET(grpc_client_channel_backup_poll_interval_ms, 1);
// Allow testing PUT requests.
grpc_core::
InternalOnlyDoNotUseUnlessYouHavePermissionFromGrpcTeamAllowBrokenPutRequests();
#if TARGET_OS_IPHONE
// Workaround Apple CFStream bug
gpr_setenv("grpc_cfstream", "0");

Write Preview
Loading…
Cancel
Save