From 934ae9ac0c1e9506e2a514dc0950a5a351d08ef9 Mon Sep 17 00:00:00 2001 From: Tim Emiola Date: Mon, 31 Aug 2015 09:42:59 -0700 Subject: [PATCH] Make insecure construction explicit, to address #2614 --- src/ruby/ext/grpc/rb_server.c | 23 ++++++++++++++--------- src/ruby/spec/client_server_spec.rb | 2 +- src/ruby/spec/generic/active_call_spec.rb | 2 +- src/ruby/spec/generic/client_stub_spec.rb | 2 +- src/ruby/spec/generic/rpc_server_spec.rb | 2 +- src/ruby/spec/pb/health/checker_spec.rb | 2 +- src/ruby/spec/server_spec.rb | 9 ++++++--- 7 files changed, 25 insertions(+), 17 deletions(-) diff --git a/src/ruby/ext/grpc/rb_server.c b/src/ruby/ext/grpc/rb_server.c index 7e76349d2e1..89582375100 100644 --- a/src/ruby/ext/grpc/rb_server.c +++ b/src/ruby/ext/grpc/rb_server.c @@ -49,6 +49,9 @@ static VALUE grpc_rb_cServer = Qnil; /* id_at is the constructor method of the ruby standard Time class. */ static ID id_at; +/* id_insecure_server is used to indicate that a server is insecure */ +static VALUE id_insecure_server; + /* grpc_rb_server wraps a grpc_server. It provides a peer ruby object, 'mark' to minimize copying when a server is created from ruby. */ typedef struct grpc_rb_server { @@ -334,7 +337,7 @@ static VALUE grpc_rb_server_destroy(int argc, VALUE *argv, VALUE self) { call-seq: // insecure port insecure_server = Server.new(cq, {'arg1': 'value1'}) - insecure_server.add_http2_port('mydomain:50051') + insecure_server.add_http2_port('mydomain:50051', :this_port_is_not_secure) // secure port server_creds = ... @@ -342,21 +345,22 @@ static VALUE grpc_rb_server_destroy(int argc, VALUE *argv, VALUE self) { secure_server.add_http_port('mydomain:50051', server_creds) Adds a http2 port to server */ -static VALUE grpc_rb_server_add_http2_port(int argc, VALUE *argv, VALUE self) { - VALUE port = Qnil; - VALUE rb_creds = Qnil; +static VALUE grpc_rb_server_add_http2_port(VALUE self, VALUE port, + VALUE rb_creds) { grpc_rb_server *s = NULL; grpc_server_credentials *creds = NULL; int recvd_port = 0; - /* "11" == 1 mandatory args, 1 (rb_creds) is optional */ - rb_scan_args(argc, argv, "11", &port, &rb_creds); - TypedData_Get_Struct(self, grpc_rb_server, &grpc_rb_server_data_type, s); if (s->wrapped == NULL) { rb_raise(rb_eRuntimeError, "destroyed!"); return Qnil; - } else if (rb_creds == Qnil) { + } else if (TYPE(rb_creds) == T_SYMBOL) { + if (id_insecure_server != SYM2ID(rb_creds)) { + rb_raise(rb_eTypeError, + "bad creds symbol, want :this_port_is_insecure"); + return Qnil; + } recvd_port = grpc_server_add_insecure_http2_port(s->wrapped, StringValueCStr(port)); if (recvd_port == 0) { @@ -398,8 +402,9 @@ void Init_grpc_server() { rb_define_alias(grpc_rb_cServer, "close", "destroy"); rb_define_method(grpc_rb_cServer, "add_http2_port", grpc_rb_server_add_http2_port, - -1); + 2); id_at = rb_intern("at"); + id_insecure_server = rb_intern("this_port_is_insecure"); } /* Gets the wrapped server from the ruby wrapper */ diff --git a/src/ruby/spec/client_server_spec.rb b/src/ruby/spec/client_server_spec.rb index 9aacea906f3..387f2baec28 100644 --- a/src/ruby/spec/client_server_spec.rb +++ b/src/ruby/spec/client_server_spec.rb @@ -396,7 +396,7 @@ describe 'the http client/server' do @client_queue = GRPC::Core::CompletionQueue.new @server_queue = GRPC::Core::CompletionQueue.new @server = GRPC::Core::Server.new(@server_queue, nil) - server_port = @server.add_http2_port(server_host) + server_port = @server.add_http2_port(server_host, :this_port_is_insecure) @server.start @ch = Channel.new("0.0.0.0:#{server_port}", nil) end diff --git a/src/ruby/spec/generic/active_call_spec.rb b/src/ruby/spec/generic/active_call_spec.rb index fcd7bd082f8..b05e3284fe8 100644 --- a/src/ruby/spec/generic/active_call_spec.rb +++ b/src/ruby/spec/generic/active_call_spec.rb @@ -46,7 +46,7 @@ describe GRPC::ActiveCall do @server_queue = GRPC::Core::CompletionQueue.new host = '0.0.0.0:0' @server = GRPC::Core::Server.new(@server_queue, nil) - server_port = @server.add_http2_port(host) + server_port = @server.add_http2_port(host, :this_port_is_insecure) @server.start @ch = GRPC::Core::Channel.new("0.0.0.0:#{server_port}", nil) end diff --git a/src/ruby/spec/generic/client_stub_spec.rb b/src/ruby/spec/generic/client_stub_spec.rb index edcc962a7db..a05433df75e 100644 --- a/src/ruby/spec/generic/client_stub_spec.rb +++ b/src/ruby/spec/generic/client_stub_spec.rb @@ -498,7 +498,7 @@ describe 'ClientStub' do def create_test_server @server_queue = GRPC::Core::CompletionQueue.new @server = GRPC::Core::Server.new(@server_queue, nil) - @server.add_http2_port('0.0.0.0:0') + @server.add_http2_port('0.0.0.0:0', :this_port_is_insecure) end def expect_server_to_be_invoked(notifier) diff --git a/src/ruby/spec/generic/rpc_server_spec.rb b/src/ruby/spec/generic/rpc_server_spec.rb index 1295fd7fddc..e484a9ea505 100644 --- a/src/ruby/spec/generic/rpc_server_spec.rb +++ b/src/ruby/spec/generic/rpc_server_spec.rb @@ -139,7 +139,7 @@ describe GRPC::RpcServer do @server_queue = GRPC::Core::CompletionQueue.new server_host = '0.0.0.0:0' @server = GRPC::Core::Server.new(@server_queue, nil) - server_port = @server.add_http2_port(server_host) + server_port = @server.add_http2_port(server_host, :this_port_is_insecure) @host = "localhost:#{server_port}" @ch = GRPC::Core::Channel.new(@host, nil) end diff --git a/src/ruby/spec/pb/health/checker_spec.rb b/src/ruby/spec/pb/health/checker_spec.rb index 6999a691058..d7b7535cbec 100644 --- a/src/ruby/spec/pb/health/checker_spec.rb +++ b/src/ruby/spec/pb/health/checker_spec.rb @@ -186,7 +186,7 @@ describe Grpc::Health::Checker do @server_queue = GRPC::Core::CompletionQueue.new server_host = '0.0.0.0:0' @server = GRPC::Core::Server.new(@server_queue, nil) - server_port = @server.add_http2_port(server_host) + server_port = @server.add_http2_port(server_host, :this_port_is_insecure) @host = "localhost:#{server_port}" @ch = GRPC::Core::Channel.new(@host, nil) @client_opts = { channel_override: @ch } diff --git a/src/ruby/spec/server_spec.rb b/src/ruby/spec/server_spec.rb index c52fe0d9b6e..439b19fb8db 100644 --- a/src/ruby/spec/server_spec.rb +++ b/src/ruby/spec/server_spec.rb @@ -105,7 +105,7 @@ describe Server do it 'runs without failing' do blk = proc do s = Server.new(@cq, nil) - s.add_http2_port('localhost:0') + s.add_http2_port('localhost:0', :this_port_is_insecure) s.close(@cq) end expect(&blk).to_not raise_error @@ -114,7 +114,10 @@ describe Server do it 'fails if the server is closed' do s = Server.new(@cq, nil) s.close(@cq) - expect { s.add_http2_port('localhost:0') }.to raise_error(RuntimeError) + blk = proc do + s.add_http2_port('localhost:0', :this_port_is_insecure) + end + expect(&blk).to raise_error(RuntimeError) end end @@ -199,7 +202,7 @@ describe Server do def start_a_server s = Server.new(@cq, nil) - s.add_http2_port('0.0.0.0:0') + s.add_http2_port('0.0.0.0:0', :this_port_is_insecure) s.start s end