-
Notifications
You must be signed in to change notification settings - Fork 604
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
Support HTTPS certs, keys and CAs #261
Conversation
641e2a8
to
d9b5131
Compare
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.
Thanks for doing this! Can you please add tests that verify the new functionality end-to-end?
internal/driver/cli.go
Outdated
// HTTPS options | ||
flagCertFile := flag.String("cert", "", "TLS client certificate file") | ||
flagKeyFile := flag.String("key", "", "TLS private key file") | ||
flagCAFile := flag.String("CA", "", "TLS CA certs file") |
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.
It's a bit strange to have command line flag in capital letters. Is there a precedent for this?
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, my mistake.
tlsConfig.RootCAs = caCertPool | ||
} | ||
|
||
if url.Scheme == "https+insecure" { |
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.
Should internal/symbolizer/symbolizer.go
be updated 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.
Do you mean the postURL()
in symbolizer.go
?
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.
Yes.
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 problem is the source
struct which contains the flag values is in driver
pkg. That's why Symbolizer()
takes mode string
rather than s *source
. In order to keep it clean I would need to create a new package for the source
struct, do you have any preference where it should go?
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 don't think we should pass all of source
struct to the symbolizer, it doesn't need most of that. Maybe something like this in internal/plugin/plugin.go
:
type TLSParams struct {
HTTPSCert, HTTPSKey, HTTPSCA string
}
and then pass it as to the symbolizer and to the fetcher as a new param.
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.
Changed the implementation to use TLSParams
, thoughts?
d9b5131
to
eb91825
Compare
@sjug Did you also see my
request? Just want to make sure it's not missed since it was a whole-PR comment rather than on a specific line and so looks subtle in the GH UI. |
@aalexand Yep, didn't miss it. Will do. |
internal/driver/cli.go
Outdated
@@ -34,6 +34,9 @@ type source struct { | |||
Timeout int | |||
Symbolize string | |||
HTTPHostport string | |||
HTTPSCert 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.
Perhaps move these up to follow Sources field to emphasize that this is about setting up the TLS for fetching the profiles not for the HTTP server mode of pprof itself.
internal/driver/cli.go
Outdated
@@ -61,6 +64,10 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { | |||
flagContentions := flag.Bool("contentions", false, "Display number of delays at each region") | |||
flagMeanDelay := flag.Bool("mean_delay", false, "Display mean delay at each region") | |||
flagTools := flag.String("tools", os.Getenv("PPROF_TOOLS"), "Path for object tool pathnames") | |||
// HTTPS options | |||
flagCertFile := flag.String("cert", "", "TLS client certificate file") |
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 like the brevity of the docs but we should here also clarify that this about the TLS params for profile and symbol fetch.
internal/driver/cli.go
Outdated
@@ -280,6 +290,9 @@ var usageMsgSrc = "\n\n" + | |||
" Source options:\n" + | |||
" -seconds Duration for time-based profile collection\n" + | |||
" -timeout Timeout in seconds for profile collection\n" + | |||
" -key Specify TLS private key\n" + |
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.
Ditto (I like the brevity of the docs but we should here also clarify that this about the TLS params for profile and symbol fetch).
internal/driver/fetch.go
Outdated
@@ -580,17 +581,35 @@ func adjustURL(source string, duration, timeout time.Duration) (string, time.Dur | |||
|
|||
// httpGet is a wrapper around http.Get; it is defined as a variable | |||
// so it can be redefined during for testing. | |||
var httpGet = func(source string, timeout time.Duration) (*http.Response, error) { | |||
var httpGet = func(s *source, source string, timeout time.Duration) (*http.Response, error) { |
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 shouldn't need to pass all of "s *source" here, we only need the TLS params?
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.
Just seemed cleaner to not create anything extra, just reuse the existing struct. If we make the TLSParams
in plugin.go
that should work for Symbolizer()
as well. Isn't having the same HTTPS*
fields in the source
struct redundant?
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 would use TLSParams in the source structure as well to avoid the duplicated fields, but (as I said) would pass the source / URL string and the TLS params alone instead of passing the whole input structure. Passing only what's needed is better than being able to avoid another little struct imo.
tlsConfig.RootCAs = caCertPool | ||
} | ||
|
||
if url.Scheme == "https+insecure" { |
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 don't think we should pass all of source
struct to the symbolizer, it doesn't need most of that. Maybe something like this in internal/plugin/plugin.go
:
type TLSParams struct {
HTTPSCert, HTTPSKey, HTTPSCA string
}
and then pass it as to the symbolizer and to the fetcher as a new param.
cecace9
to
302e336
Compare
302e336
to
efef5de
Compare
The only thing that concerns me is that the TLS parameters are visible in the help and are exposed in the method signatures now regardless of whether the underlying symbolizer and fetcher need them. For example, at Google internally we use a custom fetch implementation which wouldn't need these options. I wonder if there is a better way to abstract these into the implementation of the fetcher and symbolizer. @rauls5382 do you have some ideas? |
@aalexand Has a decision been made as to how this should be implemented so I can move forward with writing tests for this? |
Not yet. I was curious to hear @rauls5382's thoughts. Raul, any thoughts here? |
Ping @rauls5382. |
This functionality was added with #423, so I'm closing this PR. Thanks for all the work here! |
As subject says, allows users to pass
-cert
-key
and-CA
so that pprof can be used against authenticated TLS endpoints.Closes golang/go#20939 and #244