Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CA generation and mount it to /ca in containers #28

Merged
merged 7 commits into from
Nov 11, 2020

Conversation

valkum
Copy link
Contributor

@valkum valkum commented Sep 21, 2020

This PR adds support for CA generation in complement.

The Outbound Federation server will create a CA cert either in $PWD/ca or if run in CI in the Volume mounted to /ca.
On container deploy, depending on CI or not, either $PWD/ca is bind mounted to /ca or the /ca Volume of complement is mounted to /ca in homeserver containers.

@valkum
Copy link
Contributor Author

valkum commented Sep 24, 2020

Not sure if the outbound federation endpoint creation is the best place to create the CA cert. There is probably a better place, right before jumping into blueprints right?

We should specify how the lifetime is for those certs.
The CA should be generated on the start of complement (valid enough until the end of the run), and probably deleted afterwards or overwritten the next time complement starts.

Can someone explain to me how the HS containers get executed?
For each blueprint one or multiple containers get created. Are they then commited?
If I get it right:
If the container creates the local cert during RUN or ENTRYPOINT they would need to check whether the cert got already created during a previous run of that container (small optimization to not create a new cert everytime the container is run). Right?

I will add documentation when we got those specifics figured out.

@kegsay kegsay self-assigned this Oct 1, 2020
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

LGTM

I was debating whether we should be gating this behind an env var but for now let's see how bad the performance hit is.

internal/docker/builder.go Outdated Show resolved Hide resolved
internal/docker/builder.go Show resolved Hide resolved
@valkum
Copy link
Contributor Author

valkum commented Oct 30, 2020

Some things changed while further testing this. Please reapprove, to be safe.
I fixed the logic behind mounting the Volume of the complement container when run in CI.
In addtion, I made sure the certs get created at the beginning of the tests, and we keep only one instance of them.

@kegsay kegsay self-requested a review November 4, 2020 10:24
anoadragon453 added a commit that referenced this pull request Nov 4, 2020
This is necessary until homeserver containers can get access
to the dummy CA that's used to create the certificate complement
federation instances are using. Synapse can't trust those entities
over federation until this happens, so disable verification for now.

A proper fix should be possible after #28 or similar lands.
anoadragon453 added a commit that referenced this pull request Nov 4, 2020
This is necessary until homeserver containers can get access
to the dummy CA that's used to create the certificate complement
federation instances are using. Synapse can't trust those entities
over federation until this happens, so disable verification for now.

A proper fix should be possible after #28 or similar lands.
kegsay pushed a commit that referenced this pull request Nov 4, 2020
This is necessary until homeserver containers can get access
to the dummy CA that's used to create the certificate complement
federation instances are using. Synapse can't trust those entities
over federation until this happens, so disable verification for now.

A proper fix should be possible after #28 or similar lands.
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

I'd like to see this be gated behind something like COMPLEMENT_CA=1 because it adds a fair amount of extra complexity which isn't needed for most HSes which support disabling TLS validation. I'll run a few more tests with Dendrite and then see if there is a big perf hit or not.

internal/docker/builder.go Outdated Show resolved Hide resolved
internal/federation/server.go Outdated Show resolved Hide resolved
Set COMPLEMENT_CA=true to enable Complement PKI
@valkum
Copy link
Contributor Author

valkum commented Nov 9, 2020

I assume that is enough to feature gate all of the PKI stuff.
Oh and I committed the README changes. Thought I included them in the last commit. My bad.

README.md Outdated Show resolved Hide resolved
@kegsay
Copy link
Member

kegsay commented Nov 11, 2020

Fab, thanks! LGTM

@kegsay kegsay merged commit 9acaf3c into matrix-org:master Nov 11, 2020
anoadragon453 added a commit that referenced this pull request Nov 17, 2020
* 'master' of github.com:matrix-org/complement:
  Add request query parameter map to instruction struct (#37)
  Add CA generation and mount it to /ca in containers (#28)
  Federation: return Content-Type header of 'application/json' by default (#35)
  Up the default version check iterations from 50 to 100 (#34)
  Provide an empty json dict to /createRoom instead of no body (#36)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants