Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
tls: generate server TLS cert from CA #2401
tls: generate server TLS cert from CA #2401
Changes from all commits
c7b5a3b
a6a8685
3b625ec
4dc9598
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the name people will see in the Subject line of the certificate. Should we use "Infra" , or "Infra Server" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Infra Server" seems fine to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this into
internal/server/tls.go
and renamed it togetCertificate
. That's the only place it is used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer used, we'll rely on helm or the user to provide the CA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What implications does this have on the initial deploy (ex: quickstart)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, good question. I added another TODO to the list in the description. We need to generate the CA in helm as part of the deploy. That should make it invisible to the user. They won't have to do anything new, but helm will take care of creating the secret which contains the CA and CA private key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit which does this (generates the CA in helm and stores it in a secret)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other implication is that the server can no longer be served outside of a helm install, e.g. docker, compose, or local build, unless the CA is manually created and configured. It'll be nice to keep this to support those use cases but not critical
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, that's true. We could keep the ability to generate a CA if none is provided for development and demo purposes. I think in practice no real production use case should have the server generate a CA on first install. The operator really needs to save that CA and private key somewhere, and they aren't going to know it exists or where to find it if it was generated on first start.
I'll see about re-adding this for development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the disadvantages to generating the CA by default is that if the user accidentally mis-configured either
ca
orcaPrivateKey
. For example, maybe they set only one of the two, or they setprimaryKey
instead ofcaPrimaryKey
. With the changes in this PR they will get a nice error and know how to fix it.If we always generate things for them by default then it won't fail, and they'll have a harder time debugging.
I think it might be better to require these are set ahead of starting the server. For dev maybe we could add a
--dev
flag and do some setup in the CLI before starting the server.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either Helm or infra can exit error if only one of
ca
orcaPrivateKey
is supplied since both are required. It'll reduce the impact of the scenario you're suggesting. Either way, it can be addressed in a future change