-
Notifications
You must be signed in to change notification settings - Fork 19
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
Implements HTTPS support for hieraserver #60
Conversation
1b28bac
to
1cf5236
Compare
1cf5236
to
03f3841
Compare
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.
Great improvement with https! Implementation looks good aside from my nitpicks.
hieraserver/rest/rest.go
Outdated
|
||
var err error | ||
var tlsConfig *tls.Config | ||
tlsConfig, err = makeTLSconfig() |
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.
Please skip preceding var
declarations and use :=
.
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.
done
hieraserver/rest/rest.go
Outdated
certPool := x509.NewCertPool() | ||
ok := certPool.AppendCertsFromPEM(data) | ||
if !ok { | ||
return nil, fmt.Errorf("Failed to load certificate %s", pemFile) |
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.
Please use %q
to quote the path as it's not uncommon that it contains spaces.
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.
done
Prior to this commit, changes in lyraproj#60 caused an initialized but non-functional tlsConfig object to skip the non-SSL listener startup, which would fail. There might be a more sophisticated fix, but this wfm... Fixes lyraproj#62
Hello
I suggest enabling hieraserver to run in HTTPS configuration since it is likely to be serving secrets.