From 78c9ebc621c2f2bd6fc90d351c4e4675b583fe56 Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Mon, 22 Jul 2019 14:35:26 +0200 Subject: [PATCH] cmd/alertmanager: reject invalid external URLs (#1960) * cmd/alertmanager: reject invalid external URLs Signed-off-by: Simon Pasquier * Address Brian's comments Signed-off-by: Simon Pasquier * Simplify the code according to Max's feedback Signed-off-by: Simon Pasquier --- cmd/alertmanager/main.go | 25 +++++++---- cmd/alertmanager/main_test.go | 78 +++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 9 deletions(-) diff --git a/cmd/alertmanager/main.go b/cmd/alertmanager/main.go index 6ec64f1b..88136942 100644 --- a/cmd/alertmanager/main.go +++ b/cmd/alertmanager/main.go @@ -30,6 +30,7 @@ import ( "github.com/go-kit/kit/log" "github.com/go-kit/kit/log/level" + "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" "github.com/prometheus/common/model" @@ -323,15 +324,16 @@ func run() int { }) if err != nil { - level.Error(logger).Log("err", fmt.Errorf("failed to create API: %v", err.Error())) + level.Error(logger).Log("err", errors.Wrap(err, "failed to create API")) return 1 } - amURL, err := extURL(*listenAddress, *externalURL) + amURL, err := extURL(logger, os.Hostname, *listenAddress, *externalURL) if err != nil { - level.Error(logger).Log("err", err) + level.Error(logger).Log("msg", "failed to determine external URL", "err", err) return 1 } + level.Debug(logger).Log("externalURL", amURL.String()) waitFunc := func() time.Duration { return 0 } if peer != nil { @@ -357,7 +359,7 @@ func run() int { configCoordinator.Subscribe(func(conf *config.Config) error { tmpl, err = template.FromGlobs(conf.Templates...) if err != nil { - return fmt.Errorf("failed to parse templates: %v", err.Error()) + return errors.Wrap(err, "failed to parse templates") } tmpl.ExternalURL = amURL @@ -404,14 +406,13 @@ func run() int { } // Make routePrefix default to externalURL path if empty string. - if routePrefix == nil || *routePrefix == "" { + if *routePrefix == "" { *routePrefix = amURL.Path } - *routePrefix = "/" + strings.Trim(*routePrefix, "/") + level.Debug(logger).Log("routePrefix", *routePrefix) router := route.New().WithInstrumentation(instrumentHandler) - if *routePrefix != "/" { router = router.WithPrefix(*routePrefix) } @@ -481,9 +482,9 @@ func clusterWait(p *cluster.Peer, timeout time.Duration) func() time.Duration { } } -func extURL(listen, external string) (*url.URL, error) { +func extURL(logger log.Logger, hostnamef func() (string, error), listen, external string) (*url.URL, error) { if external == "" { - hostname, err := os.Hostname() + hostname, err := hostnamef() if err != nil { return nil, err } @@ -491,6 +492,9 @@ func extURL(listen, external string) (*url.URL, error) { if err != nil { return nil, err } + if port == "" { + level.Warn(logger).Log("msg", "no port found for listen address", "address", listen) + } external = fmt.Sprintf("http://%s:%s/", hostname, port) } @@ -499,6 +503,9 @@ func extURL(listen, external string) (*url.URL, error) { if err != nil { return nil, err } + if u.Scheme != "http" && u.Scheme != "https" { + return nil, errors.Errorf("%q: invalid %q scheme, only 'http' and 'https' are supported", u.String(), u.Scheme) + } ppref := strings.TrimRight(u.Path, "/") if ppref != "" && !strings.HasPrefix(ppref, "/") { diff --git a/cmd/alertmanager/main_test.go b/cmd/alertmanager/main_test.go index 803e3e00..f5110c82 100644 --- a/cmd/alertmanager/main_test.go +++ b/cmd/alertmanager/main_test.go @@ -14,8 +14,10 @@ package main import ( + "fmt" "testing" + "github.com/go-kit/kit/log" commoncfg "github.com/prometheus/common/config" "github.com/stretchr/testify/require" @@ -86,3 +88,79 @@ func TestBuildReceiverIntegrations(t *testing.T) { }) } } + +func TestExternalURL(t *testing.T) { + hostname := "foo" + for _, tc := range []struct { + hostnameResolver func() (string, error) + external string + listen string + + expURL string + err bool + }{ + { + listen: ":9093", + expURL: "http://" + hostname + ":9093", + }, + { + listen: "localhost:9093", + expURL: "http://" + hostname + ":9093", + }, + { + listen: "localhost:", + expURL: "http://" + hostname + ":", + }, + { + external: "https://host.example.com", + expURL: "https://host.example.com", + }, + { + external: "https://host.example.com/", + expURL: "https://host.example.com", + }, + { + external: "http://host.example.com/alertmanager", + expURL: "http://host.example.com/alertmanager", + }, + { + external: "http://host.example.com/alertmanager/", + expURL: "http://host.example.com/alertmanager", + }, + { + external: "http://host.example.com/////alertmanager//", + expURL: "http://host.example.com/////alertmanager", + }, + { + err: true, + }, + { + hostnameResolver: func() (string, error) { return "", fmt.Errorf("some error") }, + err: true, + }, + { + external: "://broken url string", + err: true, + }, + { + external: "host.example.com:8080", + err: true, + }, + } { + tc := tc + if tc.hostnameResolver == nil { + tc.hostnameResolver = func() (string, error) { + return hostname, nil + } + } + t.Run(fmt.Sprintf("external=%q,listen=%q", tc.external, tc.listen), func(t *testing.T) { + u, err := extURL(log.NewNopLogger(), tc.hostnameResolver, tc.listen, tc.external) + if tc.err { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tc.expURL, u.String()) + }) + } +}