Validate server TLS certificate between federators by jschaul · Pull Request #1662 · wireapp/wire-server

jschaul · 2021-07-13T13:14:10Z

This PR sets a reasonably secure TLS configuration for the GRPC client that federator uses to connect to other federators (or rather, nginx ingresses, assuming a wire-server instance on the other side).

Summary of changes

Enabled TLS validation and specifying a CA when creating a GRPC client
Added new configuration options for federator and corresponding docs (docs/reference/config-options.md)
Added integration test for the GRPC client against a local nginx playing the role of ingress
Changed end2end tests so that certificates for the ingress are created on the fly (using correct domains)
Added unit tests for the GRPC client using a mock http2/TLS server

Remaining questions

Do we need to support TLS 1.2?
Should we use a list of "blessed" ciphers?

Checklist

jschaul · 2021-07-13T15:47:23Z

Interestingly, the end2end tests show weirdly-encoded errors as a 500 server error with just the first commit of this PR:

        executeSearchWithDomain, called at test/integration/API/Search/Util.hs:88:26 in main:API.Search.Util
        assertCanFindWithDomain, called at test/integration/Federation/End2end.hs:135:3 in main:Federation.End2end

    get users by ids on multiple backends:                                      FAIL
      Exception: Error executing request: HttpExceptionRequest Request {
        host                 = "brig.test-izcvydyodvs4.svc.cluster.local"
        port                 = 8080
        secure               = False
        requestHeaders       = [("Z-User","91fa9030-7347-4572-8992-9d1aebf3e1f9"),("Content-Type","application/json"),("Content-Type","application/json"),("Accept","application/json")]
        path                 = "list-users"
        queryString          = ""
        method               = "POST"
        proxy                = Nothing
        rawBody              = False
        redirectCount        = 10
        responseTimeout      = ResponseTimeoutDefault
        requestVersion       = HTTP/1.1
      }

       (StatusCodeException (Response {responseStatus = Status {statusCode = 500, statusMessage = "Internal Server Error"},
 responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Tue, 13 Jul 2021 14:12:07 GMT"),("Server","Warp/3.3.13"),("Content-Type","application/json")], 
responseBody = (), responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}) "{\"code\":500,\"message
\":\"\\u001f\239\191\189\\u0008\\u0000\\u0000\\u0000\\u0000\\u0000\\u0004\\u0003\239\191\189\239\191\189Mk\\u00021
\\u0010\239\191\189\239\191\189J\239\191\189\239\191\189\239\191\189f\239\191\189\239\191\189^\\u0016<\\u0014\239\191
\189\\u001f\208\138\239\191\189=uK\\u0019\239\191\189\239\191\189M\239\191\189&K2k\239\191\189J\239\191\189{\239
\191\189\239\191\189\\\"\239\191\189;\239\191\189\&0y\239\191\189y\239\191\189=\239\191\189FF(\\u000e\239\191\189 WP\239\191\189hC\239\191\189\\u0002\239\191\189\239\191\189n\239\191\189%\239\191\189m\239\191\189\\u0010\229\186\147FG\239\191\189@\239\191\189\\u001a\239\191\189K\239\191\189\\u0019\239\191\189L\239\191\189eE\239\191\189\239\191\189\239\191\189\\u001f\239\191\189\239\191\189K\239\191\189;\239\191\189y\239\191\189\239\191\189\239\191\189\&2\239\191\189Wy\220\171\\\\\239\191\189\&6rB\239\191\189Wh\239\191\189\239\191\189\239\191\189\239\191\189.D\239\191\189\239\191\189\239\191\189\239\191\189:-o\239\191\189\239\191\189\\u000cj\239\191\189\\u0011\239\191\189=\239\191\189\\r\239\191\189\\u0012\\u0014\239\191\189\\u000f\239\191\189\239\191\189_.\239\191\189\\\"2r\\u001b\239\191\189q\239\191\189(L\239\191\189b\239\191\189\198\182\239\191\189\239\191\189\211\143\239\191\189\&5\239\191\189\239\191\189\239\191\189j\239\191\189\239\191\189\239\191\189>e\DEL\219\147qR\239\191\189\\u0003:\\u001d+\239\191\189\239\191\189]:$-\\u0006\239\191\189\239\191\189\239\191\189\239\191\189\\u0008\239\191\189\239\191\189\239\191\189V\\u000c\202\178\\u0004E\239\191\189\239\191\189\239\191\189\239\191\189 \\u0002}\239\191\189b\210\133x\239\191\189cM\239\191\189&\239\191\189\200\170\\u0012\239\191\189\239\191\189\239\191\189\239\191\189u\228\191\154\\u0012\239\191\189zY\239\191\189\\n-e\239\191\189s\239\191\189\239\191\189\239\191\189\&9\239\191\189\239\191\189\239\191\189\239\191\189\\u0012Rq\\u0016\239\191\189d/\239\191\189\239\191\189\239\191\189 y,\\u0008\239\191\189\DEL\\u0000F\239\191\189\239\191\189\239\191\189\239\191\189\\u0001\\u0000\\u0000\",\"label\":\"server-error\"}")
      CallStack (from HasCallStack):
        error, called at src/Bilge/Assert.hs:99:18 in bilge-0.22.0-KwPXBVYHFlF3OFbmYHfwv3:Bilge.Assert
        <!!, called at src/Bilge/Assert.hs:107:19 in bilge-0.22.0-KwPXBVYHFlF3OFbmYHfwv3:Bilge.Assert
        !!!, called at test/integration/Federation/End2end.hs:143:3 in main:Federation.End2end

The `catchError` middleware was running over gzipped responses, and this caused the wrapping logic to fail, and to display a gzip-encoded error message.

This prevents the response body to being preemptively converted to Text when wrapping an error response inside a JSON object.

There is no need to use STM in `lazyResponseBody`, since it is not interacting with any other STM action, and just using TVars inside `atomically` blocks.

This should fix the error reporting and show what the actual problem is.

…) tests

The HTTP manager should only be used to perform requests to internal services, so we do not need to set up SSL for it.

- Add configuration option to use system CA store - Propagate error when the custom CA store path cannot be loaded - Add ability to use both a custom and a system CA store

hack/bin/buildah-make-images.sh

services/federator/src/Federator/Options.hs

akshaymankar · 2021-07-22T14:48:00Z

charts/federator/templates/configmap-ca.yaml

+apiVersion: v1
+kind: Secret
+metadata:
+  name: "federator-ca"
+  labels:
+    wireService: federator
+    chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
+    release: {{ .Release.Name }}
+    heritage: {{ .Release.Service }}
+data:
+  # TODO: add validation and fail early during templating: either contents should be provided; or explicitly system trust store enabled
+  {{- if .Values.remoteCAContents }}
+  remote-ca.pem: {{ .Values.remoteCAContents | b64enc | quote }}
+  {{- end }}


@jschaul Why does this have to be a secret? It is the public key anyway. I think we can merge it into the configmap

I did this refactoring the followup PR #1682

services/brig/src/Brig/Run.hs

…vements

Also allow TLS 1.2

jschaul

Generally looks good now, though there seem some CI failures still? We'll need to tweak the federation environments when this is merged; and see whether useSystemCAStore will work (I think it won't, since currently the grpc-ingress doesn't have let's encrypt properly configured)

services/federator/test/unit/Test/Federator/Validation.hs

jschaul · 2021-07-26T10:36:39Z

hack/helm_vars/wire-server/values.yaml

+    requests: {}
  imagePullPolicy: Always
  config:
    optSettings:


for CI, shouldn't this set useSystemCAStore to false?

Yes, I think you're right. Also, should the default assignment useSystemCAStore: true reside in values.yaml or in the template?

hack/bin/selfsigned.sh

jschaul · 2021-07-26T10:39:16Z

docs/reference/config-options.md

+1. `remoteCAStore`: Maybe Filepath. This config option can be used to specify
+   multiple certificates from either a single file (multiple PEM formatted
+   certificates concatenated) or directory (one certificate per file, file names
+   are hashes from certificate).


It would be good to provide an example yaml documentation block with the exact syntax and spelling. And ti would be good to list the default.

I've added some examples in the followup PR #1682 (commit 011e83a2fa55e10554a825b70e2f9325487b57c2), since that also contains new options.

jschaul · 2021-07-26T10:41:29Z

charts/federator/templates/configmap.yaml

+      {{- if $.Values.remoteCAContents }}
+      remoteCAStore: "/etc/wire/federator/ca/remote-ca.pem"
+      {{- end }}
+      federationStrategy:


Note: Remember to update the values in the federation anta/bella/chala environments when this PR gets merged.

This part should be fine in anta/bella/chala, as by default we use the system CA, and those certificates are supposed to be Let's Encrypt. However, it seems the Let's Encrypt certificates are not actually being used, so I guess there is a problem somewhere in the ingress configuration.

The script to be used to generate certificates for end2end tests is selfsigned-kubernetes.sh.

It is currently not so easy to distinguish this particular error from a generic TLS error (see #1662 for more context). Since `InvalidCertificate` is never thrown, this PR simply removes it. Note that this is a breaking change in the federation protobuf.

* Document federation errors Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com> * Remove `InvalidCertificate` federation error It is currently not so easy to distinguish this particular error from a generic TLS error (see #1662 for more context). Since `InvalidCertificate` is never thrown, this PR simply removes it. Note that this is a breaking change in the federation protobuf. * Remove labels from protobuf errors * Improve federation error descriptions Also suggest client behaviour in some cases. Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com>

WIP: perform some TLS validation when talking to remote servers

1e37e83

jschaul changed the title ~~WIP: perform some TLS validation when talking to remote servers~~ WIP: Federation: perform some TLS validation when talking to remote servers Jul 13, 2021

jschaul added 3 commits July 13, 2021 16:08

wiring for integration tests using TLS in federator: WIP

a42976f

locally also listen on ipv6 and use localhost to connect to ingress

349d312

test succeeds locally (only) for tests

f15d8cb

jschaul added 3 commits July 13, 2021 23:23

load trust store file path from configuration

f336ca6

attempt to add CA certificate store to helm chart configurations

906e455

fixup: kube .local directory creation

fef7873

jschaul changed the title ~~WIP: Federation: perform some TLS validation when talking to remote servers~~ WIP: Federation: perform some TLS validation when talking to remote servers [skip ci] Jul 14, 2021

buildah: nginz

81c03e8

jschaul force-pushed the certificates-improvements branch from 17288d9 to 81c03e8 Compare July 14, 2021 18:44

jschaul and others added 5 commits July 14, 2021 21:03

buildah-nginz: fixup

0e39191

more executables?

c550172

Run error catching middleware before gzip

e5c2e5d

The `catchError` middleware was running over gzipped responses, and this caused the wrapping logic to fail, and to display a gzip-encoded error message.

Avoid unnecessary conversion to Text

bf8270d

This prevents the response body to being preemptively converted to Text when wrapping an error response inside a JSON object.

Use IORef instead of TVar for extracting body

388e658

There is no need to use STM in `lazyResponseBody`, since it is not interacting with any other STM action, and just using TVars inside `atomically` blocks.

pcapriotti mentioned this pull request Jul 16, 2021

Improvements to middlewares #1671

Merged

10 tasks

Merge middleware-improvements branch (#1671)

0d71244

This should fix the error reporting and show what the actual problem is.

pcapriotti changed the title ~~WIP: Federation: perform some TLS validation when talking to remote servers [skip ci]~~ WIP: Federation: perform some TLS validation when talking to remote servers Jul 16, 2021

jschaul and others added 3 commits July 19, 2021 16:01

dynamically generate TLS certificates for use in integration (end2end…

7304697

…) tests

Remove SSL settings from HTTP manager

79dfd9e

The HTTP manager should only be used to perform requests to internal services, so we do not need to set up SSL for it.

Extend federator CA store configuration

c1a6b1d

- Add configuration option to use system CA store - Propagate error when the custom CA store path cannot be loaded - Add ability to use both a custom and a system CA store

jschaul commented Jul 19, 2021

View reviewed changes

hack/bin/buildah-make-images.sh Outdated Show resolved Hide resolved

jschaul commented Jul 19, 2021

View reviewed changes

services/federator/src/Federator/Options.hs Outdated Show resolved Hide resolved

pcapriotti added 2 commits July 21, 2021 09:36

Add makefile target to restart nginx ingress

2ac39b0

Strip trailing dot when validating certificate

8c0890c

pcapriotti force-pushed the certificates-improvements branch from f5d21c5 to 8c0890c Compare July 21, 2021 08:39

pcapriotti added 2 commits July 21, 2021 12:19

Load certificate store during initialisation

5649ce5

Move ingress test to its own file

0d0bc1f

Ungitignore federator pem files next to the files

b915368

akshaymankar reviewed Jul 22, 2021

View reviewed changes

pcapriotti marked this pull request as ready for review July 22, 2021 14:52

akshaymankar reviewed Jul 22, 2021

View reviewed changes

services/brig/src/Brig/Run.hs Outdated Show resolved Hide resolved

akshaymankar and others added 6 commits July 22, 2021 17:04

Use discardLogs from Polysemy.TinyLog

9f221ed

Merge remote-tracking branch 'origin/develop' into certificates-impro…

2e57c77

…vements

Ormolu

b5239e9

Federator allow only specific TLS ciphers while connecting to remote

0ae9d6f

Also allow TLS 1.2

Delete stray comment

824ed2b

Ormolu

3368a6f

Hi CI

7976ba5

jschaul commented Jul 26, 2021

View reviewed changes

pcapriotti and others added 8 commits July 26, 2021 12:53

Revert changes to selfsigned.sh script

2f72d39

The script to be used to generate certificates for end2end tests is selfsigned-kubernetes.sh.

Remote out-of-date TODO

1a449f3

Set useSystemCAStore to false for end2end tests

d973ef5

charts/federator: add useSystemCAStore to the configmap

3e40ee4

Add federator DNS name to cert-manager certificate

befabef

charts/federator: configure nginxIngress for integration tests

b756d62

Mount CA volume for federator-integration

5330d8d

Actually specify CA volume to mount

4dbb314

pcapriotti approved these changes Jul 27, 2021

View reviewed changes

pcapriotti merged commit d3db326 into develop Jul 27, 2021

pcapriotti deleted the certificates-improvements branch July 27, 2021 07:53

pcapriotti added a commit that referenced this pull request Jul 28, 2021

Move entry for #1662 to Federation section

775ab37

Comments

Conversation

jschaul commented Jul 13, 2021 • edited by pcapriotti Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Remaining questions

Checklist

Uh oh!

jschaul commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jschaul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jschaul commented Jul 13, 2021 •

edited by pcapriotti

Loading

jschaul commented Jul 13, 2021 •

edited

Loading