Skip to content

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