From 16c5d0e4b765347eeccb44c2d0309ea049713bbf Mon Sep 17 00:00:00 2001 From: Matt Bostock Date: Thu, 28 Sep 2017 11:10:59 +0100 Subject: [PATCH 1/4] Always verify TLS unless explicitly told otherwise TLS was not being verified in a number of places: - connections to grafana.com - connections to OAuth providers when TLS client authentication was enabled - connections to self-hosted Grafana installations when using the CLI tool TLS should always be verified unless the user explicitly enables an option to skip verification. Removes some instances where `InsecureSkipVerify` is explicitly set to `false`, the default, to help avoid confusion and make it more difficult to regress on this fix by accident. Adds a `--insecure` flag to `grafana-cli` to skip TLS verification. Adds a `tls_skip_verify_insecure` setting for OAuth. Adds a `app_tls_skip_verify_insecure` setting under a new `[plugins]` section. I'm not super happy with the way the global setting is used by `pkg/api/app_routes.go` but that seems to be the existing pattern used. --- pkg/api/app_routes.go | 29 +++++++++++++----------- pkg/api/grafana_com_proxy.go | 4 +--- pkg/api/login_oauth.go | 2 +- pkg/cmd/grafana-cli/main.go | 10 ++++++-- pkg/cmd/grafana-cli/services/services.go | 7 +++--- pkg/setting/setting.go | 6 +++++ pkg/setting/setting_oauth.go | 1 + pkg/social/social.go | 1 + 8 files changed, 38 insertions(+), 22 deletions(-) diff --git a/pkg/api/app_routes.go b/pkg/api/app_routes.go index 8992f8f66d6..3d1c4e96bd2 100644 --- a/pkg/api/app_routes.go +++ b/pkg/api/app_routes.go @@ -13,24 +13,27 @@ import ( "github.com/grafana/grafana/pkg/middleware" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" ) -var pluginProxyTransport = &http.Transport{ - TLSClientConfig: &tls.Config{ - InsecureSkipVerify: true, - Renegotiation: tls.RenegotiateFreelyAsClient, - }, - Proxy: http.ProxyFromEnvironment, - Dial: (&net.Dialer{ - Timeout: 30 * time.Second, - KeepAlive: 30 * time.Second, - DualStack: true, - }).Dial, - TLSHandshakeTimeout: 10 * time.Second, -} +var pluginProxyTransport *http.Transport func InitAppPluginRoutes(r *macaron.Macaron) { + pluginProxyTransport = &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: setting.PluginAppsSkipVerifyTLS, + Renegotiation: tls.RenegotiateFreelyAsClient, + }, + Proxy: http.ProxyFromEnvironment, + Dial: (&net.Dialer{ + Timeout: 30 * time.Second, + KeepAlive: 30 * time.Second, + DualStack: true, + }).Dial, + TLSHandshakeTimeout: 10 * time.Second, + } + for _, plugin := range plugins.Apps { for _, route := range plugin.Routes { url := util.JoinUrlFragments("/api/plugin-proxy/"+plugin.Id, route.Path) diff --git a/pkg/api/grafana_com_proxy.go b/pkg/api/grafana_com_proxy.go index 015f690adda..a2a446b48eb 100644 --- a/pkg/api/grafana_com_proxy.go +++ b/pkg/api/grafana_com_proxy.go @@ -1,7 +1,6 @@ package api import ( - "crypto/tls" "net" "net/http" "net/http/httputil" @@ -14,8 +13,7 @@ import ( ) var grafanaComProxyTransport = &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: false}, - Proxy: http.ProxyFromEnvironment, + Proxy: http.ProxyFromEnvironment, Dial: (&net.Dialer{ Timeout: 30 * time.Second, KeepAlive: 30 * time.Second, diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index 4be49915fd9..426f3f048b4 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -97,7 +97,7 @@ func OAuthLogin(ctx *middleware.Context) { tr := &http.Transport{ TLSClientConfig: &tls.Config{ - InsecureSkipVerify: true, + InsecureSkipVerify: setting.OAuthService.OAuthInfos[name].TlsSkipVerify, Certificates: []tls.Certificate{cert}, RootCAs: caCertPool, }, diff --git a/pkg/cmd/grafana-cli/main.go b/pkg/cmd/grafana-cli/main.go index 73548c3b159..86eb6bc271a 100644 --- a/pkg/cmd/grafana-cli/main.go +++ b/pkg/cmd/grafana-cli/main.go @@ -17,8 +17,6 @@ var version = "master" func main() { setupLogging() - services.Init(version) - app := cli.NewApp() app.Name = "Grafana cli" app.Usage = "" @@ -44,12 +42,20 @@ func main() { Value: "", EnvVar: "GF_PLUGIN_URL", }, + cli.BoolFlag{ + Name: "insecure", + Usage: "Skip TLS verification (insecure)", + }, cli.BoolFlag{ Name: "debug, d", Usage: "enable debug logging", }, } + app.Before = func(c *cli.Context) error { + services.Init(version, c.GlobalBool("insecure")) + return nil + } app.Commands = commands.Commands app.CommandNotFound = cmdNotFound diff --git a/pkg/cmd/grafana-cli/services/services.go b/pkg/cmd/grafana-cli/services/services.go index d3a05430944..d13e90d6a2f 100644 --- a/pkg/cmd/grafana-cli/services/services.go +++ b/pkg/cmd/grafana-cli/services/services.go @@ -22,7 +22,7 @@ var ( grafanaVersion string ) -func Init(version string) { +func Init(version string, skipTLSVerify bool) { grafanaVersion = version tr := &http.Transport{ @@ -36,8 +36,9 @@ func Init(version string) { IdleConnTimeout: 90 * time.Second, TLSHandshakeTimeout: 10 * time.Second, ExpectContinueTimeout: 1 * time.Second, - - TLSClientConfig: &tls.Config{InsecureSkipVerify: false}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: skipTLSVerify, + }, } HttpClient = http.Client{ diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index f2ba16fa675..ca65fe581af 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -122,6 +122,9 @@ var ( // Basic Auth BasicAuthEnabled bool + // Plugin settings + PluginAppsSkipVerifyTLS bool + // Session settings. SessionOptions session.Options @@ -560,6 +563,9 @@ func NewConfigContext(args *CommandLineArgs) error { authBasic := Cfg.Section("auth.basic") BasicAuthEnabled = authBasic.Key("enabled").MustBool(true) + // global plugin settings + PluginAppsSkipVerifyTLS = Cfg.Section("plugins").Key("app_tls_skip_verify_insecure").MustBool(false) + // PhantomJS rendering ImagesDir = filepath.Join(DataPath, "png") PhantomDir = filepath.Join(HomePath, "vendor/phantomjs") diff --git a/pkg/setting/setting_oauth.go b/pkg/setting/setting_oauth.go index bc52d2336c3..ee2e812415b 100644 --- a/pkg/setting/setting_oauth.go +++ b/pkg/setting/setting_oauth.go @@ -13,6 +13,7 @@ type OAuthInfo struct { TlsClientCert string TlsClientKey string TlsClientCa string + TlsSkipVerify bool } type OAuther struct { diff --git a/pkg/social/social.go b/pkg/social/social.go index 9d2a53946c7..d40c0a0c965 100644 --- a/pkg/social/social.go +++ b/pkg/social/social.go @@ -66,6 +66,7 @@ func NewOAuthService() { TlsClientCert: sec.Key("tls_client_cert").String(), TlsClientKey: sec.Key("tls_client_key").String(), TlsClientCa: sec.Key("tls_client_ca").String(), + TlsSkipVerify: sec.Key("tls_skip_verify_insecure").MustBool(), } if !info.Enabled { From f2f8ca52d96e9bc5120828dba125e476bd5edf49 Mon Sep 17 00:00:00 2001 From: Matt Bostock Date: Fri, 6 Oct 2017 15:03:46 +0100 Subject: [PATCH 2/4] OAuth: Check both TLS client cert and key If either is set, try to use them. This should help avoid a situation where someone has half-configured TLS client authentication and it doesn't work without raising an obvious error. --- pkg/api/login_oauth.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index 426f3f048b4..be54a5af855 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -81,7 +81,7 @@ func OAuthLogin(ctx *middleware.Context) { // initialize oauth2 context oauthCtx := oauth2.NoContext - if setting.OAuthService.OAuthInfos[name].TlsClientCert != "" { + if setting.OAuthService.OAuthInfos[name].TlsClientCert != "" || setting.OAuthService.OAuthInfos[name].TlsClientKey != "" { cert, err := tls.LoadX509KeyPair(setting.OAuthService.OAuthInfos[name].TlsClientCert, setting.OAuthService.OAuthInfos[name].TlsClientKey) if err != nil { log.Fatal(err) From ccf093da8121948dc177437328c80e31b48a5406 Mon Sep 17 00:00:00 2001 From: Matt Bostock Date: Fri, 6 Oct 2017 15:16:26 +0100 Subject: [PATCH 3/4] OAuth: Separate TLS client auth and CA config It should be specify to either use TLS client authentication or use a user-supplied CA; previously you had to enable client authentication to use a custom CA. --- pkg/api/login_oauth.go | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index be54a5af855..a11b591f7d1 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -78,16 +78,25 @@ func OAuthLogin(ctx *middleware.Context) { } // handle call back + tr := &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: setting.OAuthService.OAuthInfos[name].TlsSkipVerify, + }, + } + sslcli := &http.Client{ + Transport: tr, + } - // initialize oauth2 context - oauthCtx := oauth2.NoContext if setting.OAuthService.OAuthInfos[name].TlsClientCert != "" || setting.OAuthService.OAuthInfos[name].TlsClientKey != "" { cert, err := tls.LoadX509KeyPair(setting.OAuthService.OAuthInfos[name].TlsClientCert, setting.OAuthService.OAuthInfos[name].TlsClientKey) if err != nil { log.Fatal(err) } - // Load CA cert + tr.TLSClientConfig.Certificates = append(tr.TLSClientConfig.Certificates, cert) + } + + if setting.OAuthService.OAuthInfos[name].TlsClientCa != "" { caCert, err := ioutil.ReadFile(setting.OAuthService.OAuthInfos[name].TlsClientCa) if err != nil { log.Fatal(err) @@ -95,19 +104,11 @@ func OAuthLogin(ctx *middleware.Context) { caCertPool := x509.NewCertPool() caCertPool.AppendCertsFromPEM(caCert) - tr := &http.Transport{ - TLSClientConfig: &tls.Config{ - InsecureSkipVerify: setting.OAuthService.OAuthInfos[name].TlsSkipVerify, - Certificates: []tls.Certificate{cert}, - RootCAs: caCertPool, - }, - } - sslcli := &http.Client{Transport: tr} - - oauthCtx = context.Background() - oauthCtx = context.WithValue(oauthCtx, oauth2.HTTPClient, sslcli) + tr.TLSClientConfig.RootCAs = caCertPool } + oauthCtx := context.WithValue(context.Background(), oauth2.HTTPClient, sslcli) + // get token from provider token, err := connect.Exchange(oauthCtx, code) if err != nil { From 83f1ae4e3e3478027274c33dc077ac697b23ae55 Mon Sep 17 00:00:00 2001 From: Matt Bostock Date: Fri, 6 Oct 2017 15:23:06 +0100 Subject: [PATCH 4/4] OAuth: Rename sslcli Rename `sslcli` to the more descriptive `oauthClient`. --- pkg/api/login_oauth.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index a11b591f7d1..b1109e61c3d 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -83,7 +83,7 @@ func OAuthLogin(ctx *middleware.Context) { InsecureSkipVerify: setting.OAuthService.OAuthInfos[name].TlsSkipVerify, }, } - sslcli := &http.Client{ + oauthClient := &http.Client{ Transport: tr, } @@ -107,7 +107,7 @@ func OAuthLogin(ctx *middleware.Context) { tr.TLSClientConfig.RootCAs = caCertPool } - oauthCtx := context.WithValue(context.Background(), oauth2.HTTPClient, sslcli) + oauthCtx := context.WithValue(context.Background(), oauth2.HTTPClient, oauthClient) // get token from provider token, err := connect.Exchange(oauthCtx, code)