Merge branch 'mablanch/799-RE-optional-TLS' into 'master'

Optional TLS support for remote-execution storage service

Closes #799

See merge request BuildStream/buildstream!1186

3 files changed, 35 insertions, 43 deletions

diff --git a/buildstream/sandbox/_sandboxremote.py b/buildstream/sandbox/_sandboxremote.py
index e9a4a7cb5..e97b37abd 100644
--- a/buildstream/sandbox/_sandboxremote.py
+++ b/buildstream/sandbox/_sandboxremote.py
@@ -95,9 +95,9 @@ class SandboxRemote(Sandbox):
         self.storage_instance = config.storage_service.get('instance-name', None)
 
         self.storage_remote_spec = CASRemoteSpec(self.storage_url, push=True,
-                                                 server_cert=config.storage_service['server-cert'],
-                                                 client_key=config.storage_service['client-key'],
-                                                 client_cert=config.storage_service['client-cert'],
+                                                 server_cert=config.storage_service.get('server-cert'),
+                                                 client_key=config.storage_service.get('client-key'),
+                                                 client_cert=config.storage_service.get('client-cert'),
                                                  instance_name=self.storage_instance)
         self.operation_name = None
 
@@ -121,28 +121,26 @@ class SandboxRemote(Sandbox):
         if remote_config is None:
             return None
 
-        # Maintain some backwards compatibility with older configs, in which 'url' was the only valid key for
-        # remote-execution.
+        service_keys = ['execution-service', 'storage-service', 'action-cache-service']
+
+        _yaml.node_validate(remote_config, ['url'] + service_keys)
+
+        exec_config = require_node(remote_config, 'execution-service')
+        storage_config = require_node(remote_config, 'storage-service')
+        action_config = remote_config.get('action-cache-service', {})
 
         tls_keys = ['client-key', 'client-cert', 'server-cert']
 
-        _yaml.node_validate(
-            remote_config,
-            ['execution-service', 'storage-service', 'url', 'action-cache-service'])
-        remote_exec_service_config = require_node(remote_config, 'execution-service')
-        remote_exec_storage_config = require_node(remote_config, 'storage-service')
-        remote_exec_action_config = remote_config.get('action-cache-service', {})
-
-        _yaml.node_validate(remote_exec_service_config, ['url', 'instance-name'] + tls_keys)
-        _yaml.node_validate(remote_exec_storage_config, ['url', 'instance-name'] + tls_keys)
-        if remote_exec_action_config:
-            _yaml.node_validate(remote_exec_action_config, ['url', 'instance-name'] + tls_keys)
-        else:
-            remote_config['action-service'] = None
+        _yaml.node_validate(exec_config, ['url', 'instance-name'] + tls_keys)
+        _yaml.node_validate(storage_config, ['url', 'instance-name'] + tls_keys)
+        if action_config:
+            _yaml.node_validate(action_config, ['url', 'instance-name'] + tls_keys)
 
+        # Maintain some backwards compatibility with older configs, in which
+        # 'url' was the only valid key for remote-execution:
         if 'url' in remote_config:
             if 'execution-service' not in remote_config:
-                remote_config['execution-service'] = {'url': remote_config['url']}
+                exec_config = {'url': remote_config['url']}
             else:
                 provenance = _yaml.node_get_provenance(remote_config, key='url')
                 raise _yaml.LoadError(_yaml.LoadErrorReason.INVALID_DATA,
@@ -151,13 +149,7 @@ class SandboxRemote(Sandbox):
                                       "You can only specify one of these."
                                       .format(str(provenance)))
 
-        for key in tls_keys:
-            if key not in remote_exec_storage_config:
-                provenance = _yaml.node_get_provenance(remote_config, key='storage-service')
-                raise _yaml.LoadError(_yaml.LoadErrorReason.INVALID_DATA,
-                                      "{}: The keys {} are necessary for the storage-service section of "
-                                      "remote-execution configuration. Your config is missing '{}'."
-                                      .format(str(provenance), tls_keys, key))
+        service_configs = [exec_config, storage_config, action_config]
 
         def resolve_path(path):
             if basedir and path:
@@ -165,17 +157,21 @@ class SandboxRemote(Sandbox):
             else:
                 return path
 
-        for key in tls_keys:
-            for d in (remote_config['execution-service'],
-                      remote_config['storage-service'],
-                      remote_exec_action_config):
-                if key in d:
-                    d[key] = resolve_path(d[key])
-
-        spec = RemoteExecutionSpec(remote_config['execution-service'],
-                                   remote_config['storage-service'],
-                                   remote_exec_action_config)
-        return spec
+        for config_key, config in zip(service_keys, service_configs):
+            # Either both or none of the TLS client key/cert pair must be specified:
+            if ('client-key' in config) != ('client-cert' in config):
+                provenance = _yaml.node_get_provenance(remote_config, key=config_key)
+                raise _yaml.LoadError(_yaml.LoadErrorReason.INVALID_DATA,
+                                      "{}: TLS client key/cert pair is incomplete. "
+                                      "You must specify both 'client-key' and 'client-cert' "
+                                      "for authenticated HTTPS connections."
+                                      .format(str(provenance)))
+
+            for tls_key in tls_keys:
+                if tls_key in config:
+                    config[tls_key] = resolve_path(config[tls_key])
+
+        return RemoteExecutionSpec(*service_configs)
 
     def run_remote_command(self, channel, action_digest):
         # Sends an execution request to the remote execution server.
diff --git a/doc/source/format_project.rst b/doc/source/format_project.rst
index 529816176..a2216100f 100644
--- a/doc/source/format_project.rst
+++ b/doc/source/format_project.rst
@@ -255,8 +255,7 @@ optional for remote execution to work.
 
 The storage service may be the same endpoint used for artifact
 caching. Remote execution cannot work without push access to the
-storage endpoint, so you must specify a client certificate and key,
-and a server certificate.
+storage endpoint though.
 
 Instance name is optional. Instance names separate different shards on
 the same endpoint (url).  You can supply a different instance name for
diff --git a/doc/source/using_config.rst b/doc/source/using_config.rst
index a7ee4dca9..40b763e78 100644
--- a/doc/source/using_config.rst
+++ b/doc/source/using_config.rst
@@ -142,10 +142,7 @@ configuration will be used as fallback.
           url: http://execution.some_project.example.com:50051
           instance-name: main
         storage-service:
-          url: https://storage.some_project.example.com:11002
-          server-cert: /some_project_keys/server.crt
-          client-cert: /some_project_keys/client.crt
-          client-key: /some_project_keys/client.key
+          url: http://storage.some_project.example.com:11002
           instance-name: main
         action-cache-service:
           url: http://cache.some_project.example.com:50052