-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 option to set certificate in-memory #6831
Conversation
Thanks @michelvocks . We use I would love some motivation here and the trade-off of having file vs env-var for tls configuration? Operators can always dump cert to a file prior to starting nomad; but setting it through env-var makes it harder to reload tls configuration. Also, it'll be nice to treat client key/cert in mutual tls setting consistently. Should we have env-vars for these as well? Though I feel a bit odd about having private key being an env var. We had a security vulnerability CVE-2019-14802 because nomad env-var processes were accidentally made available to tasks in that's fixed in #6055 . |
27ac0f6
to
3d9701f
Compare
Hi @notnoop! Thanks for the feedback!
I agree. A certificate env-var for Nomad is not necessarily important (that is why I removed it from the PR) for me. What actually interests me is to set the client certificate via the client SDK directly in-memory without the requirement to dump the certificate to disk. That would allow me to implement hashicorp/vault#5619. Since we are using the Nomad client SDK in Vault to communicate with external Nomad instances, we can use this feature to allow Vault operators to define multiple client certificates per Nomad instance. Cheers, |
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.
As an option for using the Nomad SDK this LGTM 👍
Might want to also update api/go.{mod,sum} to pull the correct version of go-rootcerts.
Hi @endocrimes! Good catch! I've updated the go modules files. Cheers, |
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.
Thanks for the context! Indeed, having those flags in /api
package is great. Minor comment about naming, but not critical.
api/api.go
Outdated
@@ -178,6 +178,10 @@ type TLSConfig struct { | |||
// the Nomad server SSL certificate. | |||
CAPath string | |||
|
|||
// CAInMemCert is the PEM-encoded CA cert to use to verify the Nomad server | |||
// SSL certificate. | |||
CAInMemCert []byte |
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 feel a tad uneasy about InMem
as a public field to indicate that this is the raw cert bytes. I'd probably consider using something with bytes, raw, or even Pem, e.g. CACertPEM
.
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.
Agreed. I changed it to CACertPEM
. What do you think?
// CAInMemCert is the PEM-encoded CA cert to use to verify the Nomad server | ||
// SSL certificate. | ||
CAInMemCert []byte | ||
|
||
// ClientCert is the path to the certificate for Nomad communication | ||
ClientCert string |
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.
We probably should expose a raw field for ClientCert
and ClientKey
values. I can follow up with that as well.
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.
Makes sense. I added both.
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.
Awesome! Thanks! This is a good improvement - merging momentarily.
Thanks a lot for your support @notnoop & @endocrimes! |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR adds the option to set an in-memory certificate to the client instead of requiring the certificate to be stored on disk in a file.
@notnoop I saw some weird changes in the vendor.json file. I couldn't find any information regarding the tool used to vendor imports so I used
govendor
. I this still accurate?