Skip to content
Merged
Show file tree
Hide file tree
Changes from 44 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
1e37e83
WIP: perform some TLS validation when talking to remote servers
jschaul Jul 13, 2021
a42976f
wiring for integration tests using TLS in federator: WIP
jschaul Jul 13, 2021
349d312
locally also listen on ipv6 and use localhost to connect to ingress
jschaul Jul 13, 2021
f15d8cb
test succeeds locally (only) for tests
jschaul Jul 13, 2021
f336ca6
load trust store file path from configuration
jschaul Jul 13, 2021
906e455
attempt to add CA certificate store to helm chart configurations
jschaul Jul 13, 2021
fef7873
fixup: kube .local directory creation
jschaul Jul 14, 2021
81c03e8
buildah: nginz
jschaul Jul 14, 2021
0e39191
buildah-nginz: fixup
jschaul Jul 14, 2021
c550172
more executables?
jschaul Jul 14, 2021
e5c2e5d
Run error catching middleware before gzip
pcapriotti Jul 16, 2021
bf8270d
Avoid unnecessary conversion to Text
pcapriotti Jul 16, 2021
388e658
Use IORef instead of TVar for extracting body
pcapriotti Jul 16, 2021
0d71244
Merge middleware-improvements branch (#1671)
pcapriotti Jul 16, 2021
7304697
dynamically generate TLS certificates for use in integration (end2end…
jschaul Jul 19, 2021
79dfd9e
Remove SSL settings from HTTP manager
pcapriotti Jul 16, 2021
c1a6b1d
Extend federator CA store configuration
pcapriotti Jul 19, 2021
2ac39b0
Add makefile target to restart nginx ingress
pcapriotti Jul 21, 2021
8c0890c
Strip trailing dot when validating certificate
pcapriotti Jul 21, 2021
5649ce5
Load certificate store during initialisation
pcapriotti Jul 21, 2021
0d0bc1f
Move ingress test to its own file
pcapriotti Jul 21, 2021
216dae1
Federator: Add happy path test for mkGrpcClient
akshaymankar Jul 21, 2021
ffb9041
Run IngressSpec tests
pcapriotti Jul 22, 2021
7f71f7d
Add comment about new setting
akshaymankar Jul 22, 2021
3a63b96
Merge branch 'develop' into certificates-improvements
akshaymankar Jul 22, 2021
9c457d5
Federator: Add more tests for TLS validation
akshaymankar Jul 22, 2021
1accf60
Handle RemoteErrorTLSException and refactor other RemoteErrors
akshaymankar Jul 22, 2021
1e1769e
Make useSystemCAStore required, put the default in helm chart
akshaymankar Jul 22, 2021
9087125
Fix compilation
akshaymankar Jul 22, 2021
6b7274d
Rename setFederationStrategy to simply federationStrategy
akshaymankar Jul 22, 2021
b0361bc
Minor refactor
akshaymankar Jul 22, 2021
3ab9971
Remove unused pragma
akshaymankar Jul 22, 2021
b7a38cf
Add docs about new federator config options
akshaymankar Jul 22, 2021
07eb851
Changelog and Ormolu
akshaymankar Jul 22, 2021
7b8e3ea
Commit pem files needed for federator unit tests
akshaymankar Jul 22, 2021
25dfd18
Testing another strategy
akshaymankar Jul 22, 2021
b915368
Ungitignore federator pem files next to the files
akshaymankar Jul 22, 2021
9f221ed
Use discardLogs from Polysemy.TinyLog
akshaymankar Jul 22, 2021
2e57c77
Merge remote-tracking branch 'origin/develop' into certificates-impro…
pcapriotti Jul 22, 2021
b5239e9
Ormolu
akshaymankar Jul 22, 2021
0ae9d6f
Federator allow only specific TLS ciphers while connecting to remote
akshaymankar Jul 22, 2021
824ed2b
Delete stray comment
akshaymankar Jul 22, 2021
3368a6f
Ormolu
akshaymankar Jul 22, 2021
7976ba5
Hi CI
akshaymankar Jul 26, 2021
2f72d39
Revert changes to selfsigned.sh script
pcapriotti Jul 26, 2021
1a449f3
Remote out-of-date TODO
pcapriotti Jul 26, 2021
d973ef5
Set useSystemCAStore to false for end2end tests
pcapriotti Jul 26, 2021
3e40ee4
charts/federator: add `useSystemCAStore` to the configmap
akshaymankar Jul 26, 2021
befabef
Add federator DNS name to cert-manager certificate
akshaymankar Jul 26, 2021
b756d62
charts/federator: configure nginxIngress for integration tests
akshaymankar Jul 26, 2021
5330d8d
Mount CA volume for federator-integration
pcapriotti Jul 26, 2021
4dbb314
Actually specify CA volume to mount
pcapriotti Jul 27, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,4 @@ i.yaml
b.yaml
telepresence.log

/.ghci
/.ghci
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Upgrade nginz (#1658)
* Extend feature config API (#1658)
* `fileSharing` feature config (#1652, #1654, #1655)
* Add user_id to csv export (#1663)
* Validate server TLS certificate between federators (#1662)

## Bug fixes and other updates

Expand Down
5 changes: 5 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,11 @@ kind-restart-all: .local/kind-kubeconfig
kubectl delete pod -n $(NAMESPACE) -l release=$(NAMESPACE)-wire-server && \
kubectl delete pod -n $(NAMESPACE)-fed2 -l release=$(NAMESPACE)-fed2-wire-server

kind-restart-nginx-ingress: .local/kind-kubeconfig
export KUBECONFIG=$(CURDIR)/.local/kind-kubeconfig && \
kubectl delete pod -n $(NAMESPACE) -l app=nginx-ingress && \
kubectl delete pod -n $(NAMESPACE)-fed2 -l app=nginx-ingress

kind-restart-%: .local/kind-kubeconfig
export KUBECONFIG=$(CURDIR)/.local/kind-kubeconfig && \
kubectl delete pod -n $(NAMESPACE) -l wireService=$(*) && \
Expand Down
14 changes: 14 additions & 0 deletions charts/federator/templates/configmap-ca.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
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 }}
Comment on lines +1 to +14
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this refactoring the followup PR #1682

15 changes: 10 additions & 5 deletions charts/federator/templates/configmap.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ kind: ConfigMap
metadata:
name: "federator"
labels:
wireService: fedrator
wireService: federator
chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
release: {{ .Release.Name }}
heritage: {{ .Release.Service }}
Expand Down Expand Up @@ -40,12 +40,17 @@ data:

{{- with .optSettings }}
optSettings:
setFederationStrategy:
{{- if .setFederationStrategy.allowAll }}
# Filepath to one or more PEM-encoded server certificates to use as a trust
# store when making grpc requests to remote backends
{{- if $.Values.remoteCAContents }}
remoteCAStore: "/etc/wire/federator/ca/remote-ca.pem"
{{- end }}
federationStrategy:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{{- if .federationStrategy.allowAll }}
allowAll:
{{- else if .setFederationStrategy.allowedDomains }}
{{- else if .federationStrategy.allowedDomains }}
allowedDomains:
{{- range $domain := .setFederationStrategy.allowedDomains }}
{{- range $domain := .federationStrategy.allowedDomains }}
- {{ $domain | quote }}
{{- end }}
{{- else }}
Expand Down
8 changes: 8 additions & 0 deletions charts/federator/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,27 @@ spec:
annotations:
# An annotation of the configmap checksum ensures changes to the configmap cause a redeployment upon `helm upgrade`
checksum/configmap: {{ include (print .Template.BasePath "/configmap.yaml") . | sha256sum }}
checksum/configmap-ca: {{ include (print .Template.BasePath "/configmap-ca.yaml") . | sha256sum }}
fluentbit.io/parser: json
spec:
volumes:
- name: "federator-config"
configMap:
name: "federator"
# federator-ca holds CA certificates to use as a trust store
# when making requests to remote backends
- name: "federator-ca"
secret:
secretName: "federator-ca"
containers:
- name: federator
image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
imagePullPolicy: {{ default "" .Values.imagePullPolicy | quote }}
volumeMounts:
- name: "federator-config"
mountPath: "/etc/wire/federator/conf"
- name: "federator-ca"
mountPath: "/etc/wire/federator/ca"
ports:
- name: internal
containerPort: {{ .Values.service.internalFederatorPort }}
Expand Down
13 changes: 12 additions & 1 deletion charts/federator/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,16 @@ config:
logLevel: Debug
logFormat: JSON
optSettings:
setFederationStrategy:
# Defaults to using system CA store in the federator image for making
# connections to remote federators.
# A custom CA certificate can be provided by specifying
# federator.remoteCAContents
# e.g. from a pem file myca.pem:
# { echo "federator:"; echo " remoteCAContents: |"; sed -e 's/^/ /' myca.pem; } > myca.yaml
# then use '-f myca.yaml' as additional flag to your helm commands.
#
# Using custom CA doesn't automatically disable system CA store, it should
# be disabled explicitly by setting useSystemCAStore to false.
useSystemCAStore: true
federationStrategy:
allowedDomains: []
1 change: 1 addition & 0 deletions deploy/services-demo/conf/nginz/nginx.conf
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ http {
# This applies only locally, as for kubernetes (helm chart) based deployments,
# TLS is terminated at the ingress level, not at nginz level
listen 8443 ssl http2;
listen [::]:8443 ssl http2;

# self-signed certificates generated using wire-server/hack/bin/selfsigned.sh
ssl_certificate integration-leaf.pem;
Expand Down
25 changes: 20 additions & 5 deletions docs/reference/config-options.md
Original file line number Diff line number Diff line change
Expand Up @@ -153,12 +153,12 @@ optSettings:

### Federation allow list

As of 2021-02, federation (whatever is implemented by the time you read this) is turned off by default by means of having an empty allow list:
As of 2021-07, federation (whatever is implemented by the time you read this) is turned off by default by means of having an empty allow list:

```yaml
# federator.yaml
optSettings:
setFederationStrategy:
federationStrategy:
allowedDomains: []
```

Expand All @@ -168,7 +168,7 @@ You can choose to federate with a specific list of allowed servers:
```yaml
# federator.yaml
optSettings:
setFederationStrategy:
federationStrategy:
allowedDomains:
- server1.example.com
- server2.example.com
Expand All @@ -179,14 +179,29 @@ or, you can federate with everyone:
```yaml
# federator.yaml
optSettings:
setFederationStrategy:
federationStrategy:
# note the 'empty' value after 'allowAll'
allowAll:

# when configuring helm charts, this becomes (note 'true' after 'allowAll')
# inside helm_vars/wire-server:
federator:
optSettings:
setFederationStrategy:
federationStrategy:
allowAll: true
```

### Federation TLS Config

When a federator connects with another federator, it does so over HTTPS. There
are two options to configure the CA for this:
1. `useSystemCAStore`: Boolean. If set to `True` it will use the system CA.
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).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Both of these options can be specified, in this case the stores are concatenated
and used for verifying certificates. When `useSystemCAStore` is `False` and
`remoteCAStore` is not set, then all outbound connections will fail with TLS
error as there will be no CA to verify.
2 changes: 1 addition & 1 deletion hack/bin/buildah-make-images.sh
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ EXECUTABLES=${EXECUTABLES:-"cannon brig cargohold galley gundeck federator brig-
CONTAINER_NAME="output"
DOCKER_TAG=${DOCKER_TAG:-$USER}

buildah containers | awk '{print $5}' | grep "$CONTAINER_NAME" || \
buildah containers | awk '{print $5}' | grep "$CONTAINER_NAME" ||
buildah from --name "$CONTAINER_NAME" -v "${TOP_LEVEL}":/src --pull quay.io/wire/alpine-deps:develop

# Only brig needs these templates, but for simplicity we add them to all resulting images (optimization FUTUREWORK)
Expand Down
22 changes: 16 additions & 6 deletions hack/bin/integration-setup.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,22 @@ USAGE="Usage: $0"

set -e

DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
TOP_LEVEL="$DIR/../.."
CHARTS_DIR="${TOP_LEVEL}/.local/charts"

NAMESPACE=${NAMESPACE:-test-integration}
ENABLE_KIND_VALUES=${ENABLE_KIND_VALUES:-0}

kubectl create namespace "${NAMESPACE}" > /dev/null 2>&1 || true
kubectl create namespace "${NAMESPACE}" >/dev/null 2>&1 || true

${DIR}/integration-cleanup.sh

charts=( fake-aws databases-ephemeral wire-server nginx-ingress-controller nginx-ingress-services )
charts=(fake-aws databases-ephemeral wire-server nginx-ingress-controller nginx-ingress-services)

echo "updating recursive dependencies ..."
for chart in "${charts[@]}"; do
"$DIR/update.sh" "$CHARTS_DIR/$chart"
"$DIR/update.sh" "$CHARTS_DIR/$chart"
done

echo "Installing charts..."
Expand All @@ -37,19 +37,26 @@ function printLogs() {

trap printLogs ERR

FEDERATION_DOMAIN="federation-test-helper.$NAMESPACE.svc.cluster.local"
export FEDERATION_DOMAIN_BASE="$NAMESPACE.svc.cluster.local"
FEDERATION_DOMAIN="federation-test-helper.$FEDERATION_DOMAIN_BASE"
"$DIR/selfsigned-kubernetes.sh"

for chart in "${charts[@]}"; do
kubectl -n ${NAMESPACE} get pods
valuesfile="${DIR}/../helm_vars/${chart}/values.yaml"
kindValuesfile="${DIR}/../helm_vars/${chart}/kind-values.yaml"
certificatesValuesfile="${DIR}/../helm_vars/${chart}/certificates.yaml"

declare -a options=()

if [ -f "$valuesfile" ]; then
options+=(-f "$valuesfile")
fi

if [ -f "$certificatesValuesfile" ]; then
options+=(-f "$certificatesValuesfile")
fi

if [[ "$chart" == "nginx-ingress-services" ]]; then
# Federation domain is also the SRV record created by the
# federation-test-helper service. Maybe we can find a way to make these
Expand Down Expand Up @@ -78,7 +85,10 @@ resourcesReady() {
SNS_POD=$(kubectl -n "${NAMESPACE}" get pods | grep fake-aws-sns | grep Running | awk '{print $1}')
kubectl -n "${NAMESPACE}" logs "$SNS_POD" -c initiate-fake-aws-sns | grep created
}
until resourcesReady; do echo 'waiting for SNS resources'; sleep 1; done
until resourcesReady; do
echo 'waiting for SNS resources'
sleep 1
done

kubectl -n ${NAMESPACE} get pods

Expand Down
84 changes: 84 additions & 0 deletions hack/bin/selfsigned-kubernetes.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
#!/usr/bin/env bash

# Create a self-signed x509 certificate in the hack/helm_vars directories (as helm yaml config).
# Requires 'cfssl' to be on your PATH (see https://github.com/cloudflare/cfssl)
# These certificates are only meant for integration tests.
# (The CA certificates are assumed to be re-used across the domains A and B for end2end integration tests.)

set -ex
TEMP=${TEMP:-/tmp}
CSR="$TEMP/csr.json"
OUTPUTNAME_CA="integration-ca"
OUTPUTNAME_LEAF_CERT="integration-leaf"
DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
TOP_LEVEL="$DIR/../.."
OUTPUT_CONFIG_FEDERATOR="$TOP_LEVEL/hack/helm_vars/wire-server/certificates.yaml"
OUTPUT_CONFIG_INGRESS="$TOP_LEVEL/hack/helm_vars/nginx-ingress-services/certificates.yaml"

command -v cfssl >/dev/null 2>&1 || {
echo >&2 "cfssl is not installed, aborting. See https://github.com/cloudflare/cfssl"
exit 1
}
command -v cfssljson >/dev/null 2>&1 || {
echo >&2 "cfssljson is not installed, aborting. See https://github.com/cloudflare/cfssl"
exit 1
}

FEDERATION_DOMAIN_BASE=${FEDERATION_DOMAIN_BASE:?"you must provide a FEDERATION_DOMAIN_BASE env variable"}

# generate CA key and cert
if [ ! -f "$OUTPUTNAME_CA.pem" ]; then
echo "CA file not found, generating CA..."
echo '{
"CN": "ca.example.com",
"key": {
"algo": "rsa",
"size": 2048
}
}' >"$CSR"
cfssl gencert -initca "$CSR" | cfssljson -bare "$OUTPUTNAME_CA"
rm "$OUTPUTNAME_CA.csr"
else
echo "Re-using previous CA"
fi

# For federation end2end tests, only the
# 'federation-test-helper.$FEDERATION_DOMAIN_BASE' is necessary for
# ingress->federator traffic. For other potential traffic in the integration
# tests of the future, we use a wildcard certificate here.
echo '{
"key": {
"algo": "rsa",
"size": 2048
}
}' >"$CSR"
# generate cert and key based on CA given comma-separated hostnames as SANs
cfssl gencert -ca "$OUTPUTNAME_CA.pem" -ca-key "$OUTPUTNAME_CA-key.pem" -hostname="*.$FEDERATION_DOMAIN_BASE" "$CSR" | cfssljson -bare "$OUTPUTNAME_LEAF_CERT"

# the following yaml override file is needed as an override to
# nginx-ingress-services helm chart
# for domain A, ingress@A needs cert+key for A
{
echo "secrets:"
echo " tlsWildcardCert: |"
sed -e 's/^/ /' $OUTPUTNAME_LEAF_CERT.pem
echo " tlsWildcardKey: |"
sed -e 's/^/ /' $OUTPUTNAME_LEAF_CERT-key.pem
} | tee "$OUTPUT_CONFIG_INGRESS"

# the following yaml override file is needed as an override to
# the wire-server (federator) helm chart
# e.g. for installing on domain A, federator@A needs the CA for B
# As a "shortcut" for integration tests, we re-use the same CA for both domains
# A and B.
{
echo "federator:"
echo " remoteCAContents: |"
sed -e 's/^/ /' $OUTPUTNAME_CA.pem
} | tee "$OUTPUT_CONFIG_FEDERATOR"

# cleanup unneeded files
rm "$OUTPUTNAME_LEAF_CERT.csr"
rm "$OUTPUTNAME_LEAF_CERT.pem"
rm "$OUTPUTNAME_LEAF_CERT-key.pem"
rm "$CSR"
Loading