Skip to content

feat!: generate a self-signed certificate if no certificates are specified #5973

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

Merged
merged 8 commits into from
Feb 2, 2023

Conversation

kylecarbs
Copy link
Member

@kylecarbs kylecarbs commented Feb 2, 2023

Clouds like AWS automatically navigate to https://. This allows us to bind to that immediately, serve a self-signed certificate, then reroute to the access URL.

@deansheather I'm going to change the tls-redirect-http-to-https flag to redirect-to-access-url instead.

Edit: to automatically listen on 443 then redirect to the access URL:

coder server --tls-enable --tls-address 0.0.0.0:443 --redirect-to-access-url

This will still open the tunnel (because access-url is not set), prompt for a self-signed certificate, then redirect the user to our external tunnel.

…fied

Clouds like AWS automatically navigate to https://<ip-here>. This
allows us to bind to that immediately, serve a self-signed certificate,
then reroute to the access URL.
@kylecarbs kylecarbs self-assigned this Feb 2, 2023
@kylecarbs
Copy link
Member Author

@deansheather Ben needs this to proceed with his work, so I'm going to merge this ASAP and do a release. If you see any issues, just let me know!

@matifali
Copy link
Member

matifali commented Feb 2, 2023

@kylecarbs what about if we do not use tls and use coder behind a reverse proxy like caddy?
Do we still need this self-signed certificate?
@bpmct if this affects the caddy reverse proxy setup then please update that guide https://github.com/coder/coder/tree/main/examples/web-server/caddy

Copy link
Member

@deansheather deansheather left a comment

Choose a reason for hiding this comment

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

Seems OK but this is a breaking change as we no longer redirect by default. You can mark your PR as breaking by adding ! before the : and adding the release/breaking label.

cli/server.go Outdated
Comment on lines 1248 to 1253
var selfSignedCertificate *tls.Certificate
if len(certs) == 0 {
selfSignedCertificate, err = generateSelfSignedCertificate()
if err != nil {
return nil, xerrors.Errorf("generate self signed certificate: %w", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Why not just certs = append(certs, selfSignedCertificate) instead of the extra logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Will fix!

@kylecarbs kylecarbs changed the title feat: generate a self-signed certificate if no certificates are specified feat!: generate a self-signed certificate if no certificates are specified Feb 2, 2023
@kylecarbs kylecarbs added the release/breaking This label is applied to PRs to detect breaking changes as part of the release process label Feb 2, 2023
@kylecarbs kylecarbs enabled auto-merge (squash) February 2, 2023 16:19
@kylecarbs kylecarbs merged commit b9b402c into main Feb 2, 2023
@kylecarbs kylecarbs deleted the insecuretls branch February 2, 2023 17:08
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release/breaking This label is applied to PRs to detect breaking changes as part of the release process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants