Support client certificates in federator by pcapriotti · Pull Request #1682 · wireapp/wire-server

pcapriotti · 2021-07-26T07:09:09Z

This adds support for client authentication using TLS certificates when making requests to a remote backend.

Summary of changes

Added new configuration options for client certificate and private key
Configured demo nginz to require client certificates
Read client certificate on federator startup
Use client certificate when creating a GRPC client
Upgrade haskell packages tls and cryptonite
Add script to serve helm charts in a repository running at http://localhost:4001

Checklist

jschaul

I know this is still marked as draft, but I had a look already anyway and made a few suggestions.

charts/federator/templates/configmap.yaml

docs/reference/config-options.md

services/federator/src/Federator/Options.hs

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

akshaymankar

Can you please explain whey we have/need let's encrypt staging? Otherwise, the PR looks good.

charts/nginx-ingress-services/templates/certificate_federator.yaml

charts/nginx-ingress-services/templates/staging_issuer.yaml

services/federator/src/Federator/Run.hs

pcapriotti · 2021-08-02T10:35:38Z

Can you please explain whey we have/need let's encrypt staging? Otherwise, the PR looks good.

I thought we could merge it with staging first to see whether things have been configured correctly. Or we could deploy it (and cert-manager) on the CI environment and check if things work there. If you think it's ok to use production immediately, I can of course change it.

akshaymankar · 2021-08-02T11:08:46Z

Can you please explain whey we have/need let's encrypt staging? Otherwise, the PR looks good.

I thought we could merge it with staging first to see whether things have been configured correctly. Or we could deploy it (and cert-manager) on the CI environment and check if things work there. If you think it's ok to use production immediately, I can of course change it.

I think it makes sense to try all of this out with staging cert manager first, but to do that we already have certificate-manager.apiServerURL config.

wire-server/charts/nginx-ingress-services/templates/issuer.yaml

Line 13 in 31479d9

server: {{ include "certificate-manager.apiServerURL" . | quote }}

pcapriotti · 2021-08-02T11:36:14Z

I think it makes sense to try all of this out with staging cert manager first, but to do that we already have certificate-manager.apiServerURL config.

But we would also need to set the CA to use, since that's different between staging and production.

akshaymankar · 2021-08-02T12:10:49Z

I think it makes sense to try all of this out with staging cert manager first, but to do that we already have certificate-manager.apiServerURL config.

But we would also need to set the CA to use, since that's different between staging and production.

Yes, but we shouldn't need to hardcode the CA into our code. We can make the testing person set that in the values.yaml when they are deploying the charts.

charts/nginx-ingress-services/templates/secret_federator.yaml

Also add client certificate CA to TLS secret

It is now an error to specify a client certificate without a private key or vice versa. We also fail in case the certificate cannot be parsed, instead of returning an empty certificate chain.

Also add examples of federator configuration

Co-authored-by: jschaul <jschaul@users.noreply.github.com>

Use the same issuer for both certificates. Whether we are using Let's Encrypt staging or production is determined by `certManager.inTestMode` so we don't need a separate issuer to enable staging.

It seems cert-manager takes control of the `ca.crt` key in the secret corresponding to a certificate, so it does not seem possible to set it manually when cert-manager is enabled. Unfortunately, it still needs to be a secret, since the ingress requires it. This commit moves the CA to a separate secret. Therefore, it needs to be set explicitly in the chart values for both federator and the nginx-ingress-services, even if federator secret sharing is enabled.

federator-ca is a configmap, not a secret

akshaymankar

Overall looks good, I am not clear on why the client cert is optional.

charts/federator/values.yaml

docs/reference/config-options.md

services/federator/src/Federator/Remote.hs

Renamed to `useSharedFederatorSecret`. Co-authored-by: Akshay Mankar <akshay@wire.com>

Co-authored-by: Akshay Mankar <akshay@wire.com>

jschaul · 2021-08-17T18:06:07Z

Post-merge review: Looks good overall. Few minor questions came to mind:

What was your usecase for the serve-helm-charts-locally script? Which part of the development flow was this useful for?
I see client certificates seem to be optional in the federator configuration, as well as in the config options readme to an operator. Wouldn't the remote backend just reject all requests if there's no valid client certificate passed though? The configuration in the nginx-ingress seems to always turn client certificate validation "on" if I read this correctly (whatever that means; shouldn't this reject the http request if it doesn't contain a client cert?). Perhaps the config-options readme could get an extra line informing a non-backend-engineer server admin on when to provide a client certificate and when this isn't necessary, and what the effects are of not specifying this.

jschaul · 2021-08-17T20:42:45Z

One more thought: the useSharedFederatorSecret setting in the federator (or wire-server) helm chart ties the federator/wire-server helm chart to the nginx-ingress-services helm chart. Yet currently those are installed separately. Is there an ordering concern here? Can I install wire-server with this setting to true before nginx-ingress-controller is installed? Or would I need to first install nginx-ingress-services before I can install wire-server helm chart?
In case there is an ugly ordering concern, can we resolve it by including the nginx-ingress-controller chart as part of wire-server's requirements (see https://github.com/wireapp/wire-server/blob/develop/charts/wire-server/requirements.yaml) ?

akshaymankar · 2021-08-18T07:19:17Z

What was your usecase for the serve-helm-charts-locally script? Which part of the development flow was this useful for?

We wanted to deploy the code in this branch to an environment using wire-server-deploy, but we couldn't without either pushing the charts to some repository or serving them locally. The newly created adhoc-testing environment can use this by adding http://localhost:4001 to its repositories list.

I see client certificates seem to be optional in the federator configuration, as well as in the config options readme to an operator. Wouldn't the remote backend just reject all requests if there's no valid client certificate passed though?

This is a mistake, they were originally optional, but then we realized they shouldn't be. There is no way the federator could work without them. So, the docs in config-options needed update and we forgot.

The configuration in the nginx-ingress seems to always turn client certificate validation "on" if I read this correctly (whatever that means; shouldn't this reject the http request if it doesn't contain a client cert?).

Yes, setting it to always turn "on" will make any requests without a client cert to be rejected.

Perhaps the config-options readme could get an extra line informing a non-backend-engineer server admin on when to provide a client certificate and when this isn't necessary, and what the effects are of not specifying this.

As said earlier, this is a mistake, I will rectify it by marking this as not optional.

One more thought: the useSharedFederatorSecret setting in the federator (or wire-server) helm chart ties the federator/wire-server helm chart to the nginx-ingress-services helm chart. Yet currently those are installed separately. Is there an ordering concern here? Can I install wire-server with this setting to true before nginx-ingress-controller is installed? Or would I need to first install nginx-ingress-services before I can install wire-server helm chart?

If someone turns useSharedFederatorSecret to true, it means that secret must exist before wire-server can be deployed. The secret doesn't have to come from the nginx-ingress-services, it can come from anywhere, for instance someone having their own TLS acquiring mechanism. But I guess in most cases, the operators would want to deploy nginx-ingress-services before wire-server.

In case there is an ugly ordering concern, can we resolve it by including the nginx-ingress-controller chart as part of wire-server's requirements (see https://github.com/wireapp/wire-server/blob/develop/charts/wire-server/requirements.yaml) ?

This is not good, if we want the ingress-controller business to be optional. This is similar to how deploying wire-server requires cassandra, elasticsearch, etc to be there. But we don't want to mandate that people use our given cassandra and elasticsearch. So, we already have this ordering problem and I guess we solve it by making sure all our docs tell users to install those things before wire-server. Currently, it is not the case with ingress-controller and services, so I will make that change to our docs.

pcapriotti changed the base branch from develop to certificates-improvements July 26, 2021 07:09

pcapriotti mentioned this pull request Jul 27, 2021

Validate server TLS certificate between federators #1662

Merged

10 tasks

Base automatically changed from certificates-improvements to develop July 27, 2021 07:53

pcapriotti force-pushed the pcapriotti/client-certificates branch 2 times, most recently from 99719ea to 775ab37 Compare July 28, 2021 06:14

jschaul reviewed Jul 28, 2021

View reviewed changes

pcapriotti marked this pull request as ready for review July 30, 2021 05:41

akshaymankar reviewed Aug 2, 2021

View reviewed changes

pcapriotti force-pushed the pcapriotti/client-certificates branch from d0f7f06 to 9b56467 Compare August 3, 2021 08:56

akshaymankar reviewed Aug 4, 2021

View reviewed changes

charts/nginx-ingress-services/templates/secret_federator.yaml Outdated Show resolved Hide resolved

pcapriotti and others added 16 commits August 9, 2021 12:13

Require client certificates in demo nginz

56b457e

Wrap CA store in a TLSSettings structure

aa445d5

Add settings for client certificate

bcbf93b

Access TLSSettings in Remote

3f98c49

Use leaf as client cert in integration tests

64f1403

Configure client certificate in helm

a768032

Enable client certificates in federator ingress

eda1e5b

Also add client certificate CA to TLS secret

Validate credentials and add tests

32ca046

It is now an error to specify a client certificate without a private key or vice versa. We also fail in case the certificate cannot be parsed, instead of returning an empty certificate chain.

Document client certificate options

6c9973d

Also add examples of federator configuration

Update CHANGELOG

a4bec3f

Fix federator-integration chart

7481032

try out helm templating also when using certificates

e642d0e

Add leaf certificates for integration tests

90af59c

Remove default value for useSystemCAStore from template

dc70198

Co-authored-by: jschaul <jschaul@users.noreply.github.com>

Move defRunSettings to a test module

68579e0

Use interpolate QQ for configuration tests

bdb9490

pcapriotti added 9 commits August 9, 2021 12:13

Fix federator certificate secret chart

8af4fd2

Remove staging issuer

f1df55c

Use the same issuer for both certificates. Whether we are using Let's Encrypt staging or production is determined by `certManager.inTestMode` so we don't need a separate issuer to enable staging.

Hi CI

06e6ff7

Fix whitespace in template

19ad195

Add script to serve helm charts locally

7d99903

Fix secret name when sharing federator secrets

0dfadf2

Add ca volume to federator integration pod

4fd5bfc

Fix one more issue in federator integration chart

24aa191

federator-ca is a configmap, not a secret

pcapriotti force-pushed the pcapriotti/client-certificates branch from 78bdb9b to 24aa191 Compare August 9, 2021 10:13

akshaymankar reviewed Aug 10, 2021

View reviewed changes

charts/federator/values.yaml Outdated Show resolved Hide resolved

docs/reference/config-options.md Show resolved Hide resolved

services/federator/src/Federator/Remote.hs Show resolved Hide resolved

pcapriotti and others added 11 commits August 11, 2021 10:11

Client certificate config with secret sharing

ae4fa09

Change ECDSA certificate to use p256 curve

803fce7

Rename shareFederatorSecret

c7d717a

Renamed to `useSharedFederatorSecret`. Co-authored-by: Akshay Mankar <akshay@wire.com>

Make client certificates required

3322865

Update FUTUREWORK comment

5054ada

Co-authored-by: Akshay Mankar <akshay@wire.com>

Fix helm template syntax

c31d88b

Upgrade to tls 1.5.5

78b97fc

Hi CI

11ca67d

Ensure that tls is enabled when federation is

e80417d

Co-authored-by: Akshay Mankar <akshay@wire.com>

serve-charts.sh: Make compatible with nixos

476e320

charts/federator: Ensure client certs are provided

c575858

akshaymankar approved these changes Aug 16, 2021

View reviewed changes

akshaymankar added 2 commits August 16, 2021 13:42

Merge branch 'develop' into pcapriotti/client-certificates

9249c17

Fix CHANGELOG

fa2698b

akshaymankar merged commit a706a5c into develop Aug 16, 2021

akshaymankar deleted the pcapriotti/client-certificates branch August 16, 2021 13:33

Conversation

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

Uh oh!

Summary of changes

Checklist

Uh oh!

jschaul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akshaymankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pcapriotti commented Aug 2, 2021

Uh oh!

akshaymankar commented Aug 2, 2021

Uh oh!

pcapriotti commented Aug 2, 2021

Uh oh!

akshaymankar commented Aug 2, 2021

Uh oh!

Uh oh!

akshaymankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jschaul commented Aug 17, 2021

Uh oh!

jschaul commented Aug 17, 2021

Uh oh!

akshaymankar commented Aug 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pcapriotti commented Jul 26, 2021 •

edited by akshaymankar

Loading