Merge pull request #2758 from jcanizales/make-http-not-subtle

Require very explicit registration of non-SSL hosts in Objective-C.

6 files changed, 47 insertions, 30 deletions

diff --git a/src/objective-c/GRPCClient/GRPCCall+Tests.h b/src/objective-c/GRPCClient/GRPCCall+Tests.h
index 3d617b05d9..cca1614606 100644
--- a/src/objective-c/GRPCClient/GRPCCall+Tests.h
+++ b/src/objective-c/GRPCClient/GRPCCall+Tests.h
@@ -33,13 +33,22 @@
 
 #import "GRPCCall.h"
 
+// Methods to let tune down the security of gRPC connections for specific hosts. These shouldn't be
+// used in releases, but are sometimes needed for testing.
 @interface GRPCCall (Tests)
 
 // Establish all SSL connections to the provided host using the passed SSL target name and the root
 // certificates found in the file at |certsPath|.
-// Must be called before any gRPC call to that host is made.
+//
+// Must be called before any gRPC call to that host is made. It's illegal to pass the same host to
+// more than one invocation of the methods of this category.
 + (void)useTestCertsPath:(NSString *)certsPath
                 testName:(NSString *)testName
                  forHost:(NSString *)host;
 
+// Establish all connections to the provided host using cleartext instead of SSL.
+//
+// Must be called before any gRPC call to that host is made. It's illegal to pass the same host to
+// more than one invocation of the methods of this category.
++ (void)useInsecureConnectionsForHost:(NSString *)host;
 @end
diff --git a/src/objective-c/GRPCClient/GRPCCall+Tests.m b/src/objective-c/GRPCClient/GRPCCall+Tests.m
index 7c5b81d661..bade0b2920 100644
--- a/src/objective-c/GRPCClient/GRPCCall+Tests.m
+++ b/src/objective-c/GRPCClient/GRPCCall+Tests.m
@@ -36,12 +36,18 @@
 #import "private/GRPCHost.h"
 
 @implementation GRPCCall (Tests)
+
 + (void)useTestCertsPath:(NSString *)certsPath
                 testName:(NSString *)testName
                  forHost:(NSString *)host {
   GRPCHost *hostConfig = [GRPCHost hostWithAddress:host];
-  hostConfig.secure = YES;
   hostConfig.pathToCertificates = certsPath;
   hostConfig.hostNameOverride = testName;
 }
+
++ (void)useInsecureConnectionsForHost:(NSString *)host {
+  GRPCHost *hostConfig = [GRPCHost hostWithAddress:host];
+  hostConfig.secure = NO;
+}
+
 @end
diff --git a/src/objective-c/GRPCClient/private/GRPCHost.m b/src/objective-c/GRPCClient/private/GRPCHost.m
index 5d9c48a524..6636c48620 100644
--- a/src/objective-c/GRPCClient/private/GRPCHost.m
+++ b/src/objective-c/GRPCClient/private/GRPCHost.m
@@ -58,22 +58,14 @@
 // Default initializer.
 - (instancetype)initWithAddress:(NSString *)address {
 
-  // Verify and normalize the address, and decide whether to use SSL.
-  if (![address rangeOfString:@"://"].length) {
-    // No scheme provided; assume https.
-    address = [@"https://" stringByAppendingString:address];
+  // To provide a default port, we try to interpret the address. If it's just a host name without
+  // scheme and without port, we'll use port 443. If it has a scheme, we pass it untouched to the C
+  // gRPC library.
+  // TODO(jcanizales): Add unit tests for the types of addresses we want to let pass untouched.
+  NSURL *hostURL = [NSURL URLWithString:[@"https://" stringByAppendingString:address]];
+  if (hostURL && !hostURL.port) {
+    address = [hostURL.host stringByAppendingString:@":443"];
   }
-  NSURL *hostURL = [NSURL URLWithString:address];
-  if (!hostURL) {
-    [NSException raise:NSInvalidArgumentException format:@"Invalid URL: %@", address];
-  }
-  NSString *scheme = hostURL.scheme;
-  if (![scheme isEqualToString:@"https"] && ![scheme isEqualToString:@"http"]) {
-    [NSException raise:NSInvalidArgumentException format:@"URL scheme %@ isn't supported.", scheme];
-  }
-  // If the user didn't specify a port (hostURL.port is nil), provide a default one.
-  NSNumber *port = hostURL.port ?: [scheme isEqualToString:@"https"] ? @443 : @80;
-  address = [@[hostURL.host, port] componentsJoinedByString:@":"];
 
   // Look up the GRPCHost in the cache.
   static NSMutableDictionary *hostCache;
@@ -84,19 +76,15 @@
   @synchronized(hostCache) {
     GRPCHost *cachedHost = hostCache[address];
     if (cachedHost) {
-      // We could verify here that the cached host uses the same protocol that we're expecting. But
-      // creating non-SSL channels by adding "http://" to the address is going away (to make the use
-      // of insecure channels less subtle), so it's not worth it now.
       return cachedHost;
     }
 
-    if ((self = [super init])) {
-      _address = address;
-      _secure = [scheme isEqualToString:@"https"];
-      hostCache[address] = self;
-    }
-    return self;
+  if ((self = [super init])) {
+    _address = address;
+    _secure = YES;
+    hostCache[address] = self;
   }
+  return self;
 }
 
 - (grpc_call *)unmanagedCallWithPath:(NSString *)path completionQueue:(GRPCCompletionQueue *)queue {
@@ -131,4 +119,7 @@
   return _hostNameOverride ?: _address;
 }
 
+// TODO(jcanizales): Don't let set |secure| to |NO| if |pathToCertificates| or |hostNameOverride|
+// have been set. Don't let set either of the latter if |secure| has been set to |NO|.
+
 @end
diff --git a/src/objective-c/tests/GRPCClientTests.m b/src/objective-c/tests/GRPCClientTests.m
index 103e5ca3d4..e5d7e43ed9 100644
--- a/src/objective-c/tests/GRPCClientTests.m
+++ b/src/objective-c/tests/GRPCClientTests.m
@@ -35,6 +35,7 @@
 #import <XCTest/XCTest.h>
 
 #import <GRPCClient/GRPCCall.h>
+#import <GRPCClient/GRPCCall+Tests.h>
 #import <ProtoRPC/ProtoMethod.h>
 #import <RemoteTest/Messages.pbobjc.h>
 #import <RxLibrary/GRXWriteable.h>
@@ -43,8 +44,7 @@
 // These are a few tests similar to InteropTests, but which use the generic gRPC client (GRPCCall)
 // rather than a generated proto library on top of it.
 
-// grpc-test.sandbox.google.com
-static NSString * const kHostAddress = @"http://localhost:5050";
+static NSString * const kHostAddress = @"localhost:5050";
 static NSString * const kPackage = @"grpc.testing";
 static NSString * const kService = @"TestService";
 
@@ -58,6 +58,9 @@ static ProtoMethod *kUnaryCallMethod;
 @implementation GRPCClientTests
 
 - (void)setUp {
+  // Register test server as non-SSL.
+  [GRPCCall useInsecureConnectionsForHost:kHostAddress];
+
   // This method isn't implemented by the remote server.
   kInexistentMethod = [[ProtoMethod alloc] initWithPackage:kPackage
                                                    service:kService
diff --git a/src/objective-c/tests/InteropTests.h b/src/objective-c/tests/InteropTests.h
index c675c8d241..4eb97e9e06 100644
--- a/src/objective-c/tests/InteropTests.h
+++ b/src/objective-c/tests/InteropTests.h
@@ -37,7 +37,7 @@
 // https://github.com/grpc/grpc/blob/master/doc/interop-test-descriptions.md
 
 @interface InteropTests : XCTestCase
-// Returns @"http://localhost:5050".
+// Returns @"localhost:5050".
 // Override in a subclass to perform the same tests against a different address.
 // For interop tests, use @"grpc-test.sandbox.google.com".
 + (NSString *)host;
diff --git a/src/objective-c/tests/InteropTests.m b/src/objective-c/tests/InteropTests.m
index a6611d27be..b61d567464 100644
--- a/src/objective-c/tests/InteropTests.m
+++ b/src/objective-c/tests/InteropTests.m
@@ -35,6 +35,7 @@
 
 #include <grpc/status.h>
 
+#import <GRPCClient/GRPCCall+Tests.h>
 #import <ProtoRPC/ProtoRPC.h>
 #import <RemoteTest/Empty.pbobjc.h>
 #import <RemoteTest/Messages.pbobjc.h>
@@ -75,15 +76,22 @@
 }
 @end
 
+#pragma mark Tests
+
+static NSString * const kLocalCleartextHost = @"localhost:5050";
+
 @implementation InteropTests {
   RMTTestService *_service;
 }
 
 + (NSString *)host {
-  return @"http://localhost:5050";
+  return kLocalCleartextHost;
 }
 
 - (void)setUp {
+  // Register test server as non-SSL.
+  [GRPCCall useInsecureConnectionsForHost:kLocalCleartextHost];
+
   _service = [[RMTTestService alloc] initWithHost:self.class.host];
 }