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

Update TLS config #324

Merged
merged 1 commit into from
Aug 4, 2020
Merged

Conversation

franziskuskiefer
Copy link
Contributor

Fixes https://github.com/zinfra/backend-issues/issues/1628

Note that this also disables TLS versions we don't want and enables TLS 1.3. Le me know when there's a reason why that doesn't work.

Copy link
Contributor

@lucendio lucendio left a comment

Choose a reason for hiding this comment

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

1.15.12 is the nginx version that consumes this configuration. Since the work on backoffice/spar is kinda on hold, would you mind double checking whether the ciphers and TLSv1.3 are supported? (I quick research resulted in yes, but you probably have more knowledge around this)

@franziskuskiefer
Copy link
Contributor Author

1.15.12 is the nginx version that consumes this configuration. Since the work on backoffice/spar is kinda on hold, would you mind double checking whether the ciphers and TLSv1.3 are supported? (I quick research resulted in yes, but you probably have more knowledge around this)

The openssl version installed (1.1.1g-r0) can do TLS1.3. That's at least the version I get when installing now. Not sure what's installed on the docker image ...
I can also move that 1.3 change to a separate PR if it needs more investigation.

@tiago-loureiro
Copy link
Contributor

The openssl version installed (1.1.1g-r0) can do TLS1.3. That's at least the version I get when installing now. Not sure what's installed on the docker image ...
I can also move that 1.3 change to a separate PR if it needs more investigation.

It runs OpenSSL 1.1.1d 10 Sep 2019 so we should be good

@franziskuskiefer franziskuskiefer merged commit c907233 into wireapp:develop Aug 4, 2020
@arianvp
Copy link
Contributor

arianvp commented Aug 5, 2020

I don think backoffice serves its own TLS so im not sure if this change does something? isn't it terminated behind nginx-ingress-controller like all other services?

@lucendio
Copy link
Contributor

lucendio commented Aug 6, 2020

I don think backoffice serves its own TLS so im not sure if this change does something? isn't it terminated behind nginx-ingress-controller like all other services?

🤔 You make an excellent point. But I guess one could argue that

a) if the image is seen as an atomic and agnostic artifact, it can run anywhere not just on an ingress terminating k8s
b) the change affects the container-global nginx configuration. Of cource, it can be overridden by mounting another 'nginx.conf' into it's very location

I guess @franziskuskiefer was just lacking of some context and instead greping around. Which is an absolute valid approach to start.

@tiago-loureiro
Copy link
Contributor

I don think backoffice serves its own TLS so im not sure if this change does something? isn't it terminated behind nginx-ingress-controller like all other services?

The backoffice is not "fronted" by anything, you can see more details on the README

a) if the image is seen as an atomic and agnostic artifact, it can run anywhere not just on an ingress terminating k8s

Right, it's not running like that.

That being said there's no good reason to have a config where undesirable ciphers/tls versions are used.

@arianvp
Copy link
Contributor

arianvp commented Aug 7, 2020

yeh but the backoffice currently doesn't use TLS at all; so i dont see why we even have things for it in the config. that was more my point :)

@tiago-loureiro tiago-loureiro mentioned this pull request Sep 4, 2020
@akshaymankar akshaymankar mentioned this pull request Sep 28, 2020
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.

5 participants