-
Notifications
You must be signed in to change notification settings - Fork 134
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
Added TLS support in Client #27
Added TLS support in Client #27
Conversation
- Added caCert flag to verify proxy url against. - Added cert and key flag to send client cert and key to proxy.
client/client.go
Outdated
proxyURL = kingpin.Flag("proxy-url", "Push proxy to talk to.").Required().String() | ||
myFqdn = kingpin.Flag("fqdn", "FQDN to register with").Default(fqdn.Get()).String() | ||
proxyURL = kingpin.Flag("proxy-url", "Push proxy to talk to.").Required().String() | ||
caCertFile = kingpin.Flag("cacert", "<file> CA certificate to verify peer against").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.
The flag names could make it more obvious they're tls related
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.
@brian-brazil, fqdn
and proxy-url
were there already. I have added cacert
,cert
and key
referring names and description from curl
command.
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.
These names are a bit terse, and you'd have to read the code to release they were related to tls.
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.
Will prefixing flags with tls_
be sufficient?
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.
tls. would be the usual way
client/client.go
Outdated
if *tlsCert != "" { | ||
cert, err := tls.LoadX509KeyPair(*tlsCert, *tlsKey) | ||
if err != nil { | ||
level.Error(coordinator.logger).Log("msg", "Certificate or Key is invalid") |
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 err should be logged too
- Added `err` in error logging
client/client.go
Outdated
proxyURL = kingpin.Flag("proxy-url", "Push proxy to talk to.").Required().String() | ||
myFqdn = kingpin.Flag("fqdn", "FQDN to register with").Default(fqdn.Get()).String() | ||
proxyURL = kingpin.Flag("proxy-url", "Push proxy to talk to.").Required().String() | ||
caCertFile = kingpin.Flag("tls_cacert", "<file> CA certificate to verify peer against").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.
Use a . rather than a _
Thanks! |
Feature Added: