From d4240c4a3e3ff0ab8165dbe2c5c67e2f10fd4188 Mon Sep 17 00:00:00 2001 From: Roman Khavronenko Date: Fri, 16 Aug 2024 11:32:04 +0200 Subject: [PATCH] lib/httputils: parse URL before creating HTTP transport (#6820) https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6740 --------- Signed-off-by: hagen1778 --- app/vmalert/datasource/init.go | 4 ++-- app/vmalert/notifier/alertmanager.go | 3 ++- app/vmalert/remoteread/init.go | 4 ++-- app/vmalert/remotewrite/debug_client.go | 2 +- app/vmalert/remotewrite/init.go | 4 ++-- app/vmctl/main.go | 6 +++--- docs/CHANGELOG.md | 1 + docs/vmalert.md | 6 +++--- lib/httputils/tls.go | 5 +++++ lib/httputils/tls_test.go | 6 ++++++ lib/snapshot/snapshot.go | 5 +++-- 11 files changed, 30 insertions(+), 16 deletions(-) diff --git a/app/vmalert/datasource/init.go b/app/vmalert/datasource/init.go index 74fb779353..a99f130e88 100644 --- a/app/vmalert/datasource/init.go +++ b/app/vmalert/datasource/init.go @@ -17,7 +17,7 @@ import ( var ( addr = flag.String("datasource.url", "", "Datasource compatible with Prometheus HTTP API. It can be single node VictoriaMetrics or vmselect endpoint. Required parameter. "+ - "Supports address in the form of IP address with a port (e.g., 127.0.0.1:8428) or DNS SRV record. "+ + "Supports address in the form of IP address with a port (e.g., http://127.0.0.1:8428) or DNS SRV record. "+ "See also -remoteRead.disablePathAppend and -datasource.showURL") appendTypePrefix = flag.Bool("datasource.appendTypePrefix", false, "Whether to add type prefix to -datasource.url based on the query type. Set to true if sending different query types to the vmselect URL.") showDatasourceURL = flag.Bool("datasource.showURL", false, "Whether to avoid stripping sensitive information such as auth headers or passwords from URLs in log messages or UI and exported metrics. "+ @@ -99,7 +99,7 @@ func Init(extraParams url.Values) (QuerierBuilder, error) { tr, err := httputils.Transport(*addr, *tlsCertFile, *tlsKeyFile, *tlsCAFile, *tlsServerName, *tlsInsecureSkipVerify) if err != nil { - return nil, fmt.Errorf("failed to create transport: %w", err) + return nil, fmt.Errorf("failed to create transport for -datasource.url=%q: %w", *addr, err) } tr.DialContext = netutil.NewStatDialFunc("vmalert_datasource") tr.DisableKeepAlives = *disableKeepAlive diff --git a/app/vmalert/notifier/alertmanager.go b/app/vmalert/notifier/alertmanager.go index ff268dd990..8930256ef8 100644 --- a/app/vmalert/notifier/alertmanager.go +++ b/app/vmalert/notifier/alertmanager.go @@ -132,7 +132,8 @@ func NewAlertManager(alertManagerURL string, fn AlertURLGenerator, authCfg proma } tr, err := httputils.Transport(alertManagerURL, tls.CertFile, tls.KeyFile, tls.CAFile, tls.ServerName, tls.InsecureSkipVerify) if err != nil { - return nil, fmt.Errorf("failed to create transport: %w", err) + return nil, fmt.Errorf("failed to create transport for alertmanager URL=%q: %w", alertManagerURL, err) + } ba := new(promauth.BasicAuthConfig) diff --git a/app/vmalert/remoteread/init.go b/app/vmalert/remoteread/init.go index c7ce0ea1db..612db0fad5 100644 --- a/app/vmalert/remoteread/init.go +++ b/app/vmalert/remoteread/init.go @@ -17,7 +17,7 @@ var ( addr = flag.String("remoteRead.url", "", "Optional URL to datasource compatible with Prometheus HTTP API. It can be single node VictoriaMetrics or vmselect."+ "Remote read is used to restore alerts state."+ "This configuration makes sense only if `vmalert` was configured with `remoteWrite.url` before and has been successfully persisted its state. "+ - "Supports address in the form of IP address with a port (e.g., 127.0.0.1:8428) or DNS SRV record. "+ + "Supports address in the form of IP address with a port (e.g., http://127.0.0.1:8428) or DNS SRV record. "+ "See also '-remoteRead.disablePathAppend', '-remoteRead.showURL'.") showRemoteReadURL = flag.Bool("remoteRead.showURL", false, "Whether to show -remoteRead.url in the exported metrics. "+ @@ -68,7 +68,7 @@ func Init() (datasource.QuerierBuilder, error) { } tr, err := httputils.Transport(*addr, *tlsCertFile, *tlsKeyFile, *tlsCAFile, *tlsServerName, *tlsInsecureSkipVerify) if err != nil { - return nil, fmt.Errorf("failed to create transport: %w", err) + return nil, fmt.Errorf("failed to create transport for -remoteRead.url=%q: %w", *addr, err) } tr.IdleConnTimeout = *idleConnectionTimeout tr.DialContext = netutil.NewStatDialFunc("vmalert_remoteread") diff --git a/app/vmalert/remotewrite/debug_client.go b/app/vmalert/remotewrite/debug_client.go index 17443c7314..7a549907c7 100644 --- a/app/vmalert/remotewrite/debug_client.go +++ b/app/vmalert/remotewrite/debug_client.go @@ -32,7 +32,7 @@ func NewDebugClient() (*DebugClient, error) { t, err := httputils.Transport(*addr, *tlsCertFile, *tlsKeyFile, *tlsCAFile, *tlsServerName, *tlsInsecureSkipVerify) if err != nil { - return nil, fmt.Errorf("failed to create transport: %w", err) + return nil, fmt.Errorf("failed to create transport for -remoteWrite.url=%q: %w", *addr, err) } c := &DebugClient{ c: &http.Client{ diff --git a/app/vmalert/remotewrite/init.go b/app/vmalert/remotewrite/init.go index bd8352d91b..412ce9e432 100644 --- a/app/vmalert/remotewrite/init.go +++ b/app/vmalert/remotewrite/init.go @@ -15,7 +15,7 @@ import ( var ( addr = flag.String("remoteWrite.url", "", "Optional URL to VictoriaMetrics or vminsert where to persist alerts state "+ "and recording rules results in form of timeseries. "+ - "Supports address in the form of IP address with a port (e.g., 127.0.0.1:8428) or DNS SRV record. "+ + "Supports address in the form of IP address with a port (e.g., http://127.0.0.1:8428) or DNS SRV record. "+ "For example, if -remoteWrite.url=http://127.0.0.1:8428 is specified, "+ "then the alerts state will be written to http://127.0.0.1:8428/api/v1/write . See also -remoteWrite.disablePathAppend, '-remoteWrite.showURL'.") showRemoteWriteURL = flag.Bool("remoteWrite.showURL", false, "Whether to show -remoteWrite.url in the exported metrics. "+ @@ -72,7 +72,7 @@ func Init(ctx context.Context) (*Client, error) { t, err := httputils.Transport(*addr, *tlsCertFile, *tlsKeyFile, *tlsCAFile, *tlsServerName, *tlsInsecureSkipVerify) if err != nil { - return nil, fmt.Errorf("failed to create transport: %w", err) + return nil, fmt.Errorf("failed to create transport for -remoteWrite.url=%q: %w", *addr, err) } t.IdleConnTimeout = *idleConnectionTimeout t.DialContext = netutil.NewStatDialFunc("vmalert_remotewrite") diff --git a/app/vmctl/main.go b/app/vmctl/main.go index 3a07a6247b..3664449258 100644 --- a/app/vmctl/main.go +++ b/app/vmctl/main.go @@ -68,7 +68,7 @@ func main() { tr, err := httputils.Transport(addr, certFile, keyFile, caFile, serverName, insecureSkipVerify) if err != nil { - return fmt.Errorf("failed to create Transport: %s", err) + return fmt.Errorf("failed to create transport for -%s=%q: %s", otsdbAddr, addr, err) } oCfg := opentsdb.Config{ Addr: addr, @@ -180,7 +180,7 @@ func main() { tr, err := httputils.Transport(addr, certFile, keyFile, caFile, serverName, insecureSkipVerify) if err != nil { - return fmt.Errorf("failed to create transport: %s", err) + return fmt.Errorf("failed to create transport for -%s=%q: %s", remoteReadSrcAddr, addr, err) } rr, err := remoteread.NewClient(remoteread.Config{ @@ -438,7 +438,7 @@ func initConfigVM(c *cli.Context) (vm.Config, error) { tr, err := httputils.Transport(addr, certFile, keyFile, caFile, serverName, insecureSkipVerify) if err != nil { - return vm.Config{}, fmt.Errorf("failed to create Transport: %s", err) + return vm.Config{}, fmt.Errorf("failed to create transport for -%s=%q: %s", vmAddr, addr, err) } bfRetries := c.Int(vmBackoffRetries) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 7b169e2b01..c986ce360f 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -44,6 +44,7 @@ See also [LTS releases](https://docs.victoriametrics.com/lts-releases/). * BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent/): account for `-usePromCompatibleNaming` cmd-line flag during when pushing data to remote storages. Thanks to @12345XXX for the [pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6776). * BUGFIX: `vminsert` in [VictoriaMetrics cluster](https://docs.victoriametrics.com/cluster-victoriametrics/): reduce CPU usage by limiting the number of concurrently running inserts. The issue was introduced in [this commit](https://github.com/VictoriaMetrics/VictoriaMetrics/commit/498fe1cfa523be5bfecaa372293c3cded85e75ab) starting from v1.101.0. See [this](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6733) issue for details. * BUGFIX: [MetricsQL](https://docs.victoriametrics.com/metricsql/): fix calculation [histogram_quantile](https://docs.victoriametrics.com/metricsql/#histogram_quantile) over Prometheus buckets with inconsistent values. It was producing incorrect results in case lower buckets. The issue was introduced in [v1.102.0](https://docs.victoriametrics.com/changelog/#v11020) release, see [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6714) for the details. +* BUGFIX: [vmalert](https://docs.victoriametrics.com/vmalert/), [vmctl](https://docs.victoriametrics.com/vmctl/) and snapshot API: verify correctness of URLs provided via cmd-line flags before executing HTTP requests. See [this](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6740) issue for details. ## [v1.102.1](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.102.1) diff --git a/docs/vmalert.md b/docs/vmalert.md index 11ded4093a..e2be95c617 100644 --- a/docs/vmalert.md +++ b/docs/vmalert.md @@ -1075,7 +1075,7 @@ The shortlist of configuration flags is the following: -datasource.tlsServerName string Optional TLS server name to use for connections to -datasource.url. By default, the server name from -datasource.url is used -datasource.url string - Datasource compatible with Prometheus HTTP API. It can be single node VictoriaMetrics or vmselect URL. Required parameter. Supports address in the form of IP address with a port (e.g., 127.0.0.1:8428) or DNS SRV record. See also -remoteRead.disablePathAppend and -datasource.showURL + Datasource compatible with Prometheus HTTP API. It can be single node VictoriaMetrics or vmselect URL. Required parameter. Supports address in the form of IP address with a port (e.g., http://127.0.0.1:8428) or DNS SRV record. See also -remoteRead.disablePathAppend and -datasource.showURL -defaultTenant.graphite string Default tenant for Graphite alerting groups. See https://docs.victoriametrics.com/vmalert/#multitenancy .This flag is available only in Enterprise binaries. See https://docs.victoriametrics.com/enterprise/ -defaultTenant.prometheus string @@ -1341,7 +1341,7 @@ The shortlist of configuration flags is the following: -remoteRead.tlsServerName string Optional TLS server name to use for connections to -remoteRead.url. By default, the server name from -remoteRead.url is used -remoteRead.url vmalert - Optional URL to datasource compatible with Prometheus HTTP API. It can be single node VictoriaMetrics or vmselect.Remote read is used to restore alerts state.This configuration makes sense only if vmalert was configured with `remoteWrite.url` before and has been successfully persisted its state. Supports address in the form of IP address with a port (e.g., 127.0.0.1:8428) or DNS SRV record. See also '-remoteRead.disablePathAppend', '-remoteRead.showURL'. + Optional URL to datasource compatible with Prometheus HTTP API. It can be single node VictoriaMetrics or vmselect.Remote read is used to restore alerts state.This configuration makes sense only if vmalert was configured with `remoteWrite.url` before and has been successfully persisted its state. Supports address in the form of IP address with a port (e.g., http://127.0.0.1:8428) or DNS SRV record. See also '-remoteRead.disablePathAppend', '-remoteRead.showURL'. -remoteWrite.basicAuth.password string Optional basic auth password for -remoteWrite.url -remoteWrite.basicAuth.passwordFile string @@ -1397,7 +1397,7 @@ The shortlist of configuration flags is the following: -remoteWrite.tlsServerName string Optional TLS server name to use for connections to -remoteWrite.url. By default, the server name from -remoteWrite.url is used -remoteWrite.url string - Optional URL to VictoriaMetrics or vminsert where to persist alerts state and recording rules results in form of timeseries. Supports address in the form of IP address with a port (e.g., 127.0.0.1:8428) or DNS SRV record. For example, if -remoteWrite.url=http://127.0.0.1:8428 is specified, then the alerts state will be written to http://127.0.0.1:8428/api/v1/write . See also -remoteWrite.disablePathAppend, '-remoteWrite.showURL'. + Optional URL to VictoriaMetrics or vminsert where to persist alerts state and recording rules results in form of timeseries. Supports address in the form of IP address with a port (e.g., http://127.0.0.1:8428) or DNS SRV record. For example, if -remoteWrite.url=http://127.0.0.1:8428 is specified, then the alerts state will be written to http://127.0.0.1:8428/api/v1/write . See also -remoteWrite.disablePathAppend, '-remoteWrite.showURL'. -replay.disableProgressBar Whether to disable rendering progress bars during the replay. Progress bar rendering might be verbose or break the logs parsing, so it is recommended to be disabled when not used in interactive mode. -replay.maxDatapointsPerQuery /query_range diff --git a/lib/httputils/tls.go b/lib/httputils/tls.go index 35f32b89c7..6b7f652a06 100644 --- a/lib/httputils/tls.go +++ b/lib/httputils/tls.go @@ -5,6 +5,7 @@ import ( "crypto/x509" "fmt" "net/http" + "net/url" "os" "strings" ) @@ -12,6 +13,10 @@ import ( // Transport creates http.Transport object based on provided URL. // Returns Transport with TLS configuration if URL contains `https` prefix func Transport(URL, certFile, keyFile, caFile, serverName string, insecureSkipVerify bool) (*http.Transport, error) { + _, err := url.Parse(URL) + if err != nil { + return nil, fmt.Errorf("failed to parse URL: %w", err) + } t := http.DefaultTransport.(*http.Transport).Clone() if !strings.HasPrefix(URL, "https") { return t, nil diff --git a/lib/httputils/tls_test.go b/lib/httputils/tls_test.go index a27669e0bb..96b6e2a5e1 100644 --- a/lib/httputils/tls_test.go +++ b/lib/httputils/tls_test.go @@ -49,4 +49,10 @@ func TestTransport(t *testing.T) { if tr.TLSClientConfig == nil { t.Fatalf("expected TLSClientConfig to be set, got nil") } + + noSchemaURL := "127.0.0.1:8880" + _, err = Transport(noSchemaURL, certFile, keyFile, CAFile, serverName, insecureSkipVerify) + if err == nil { + t.Fatalf("expected to have parse error for URL without specified schema; got nil instead") + } } diff --git a/lib/snapshot/snapshot.go b/lib/snapshot/snapshot.go index 87bf8d09f6..e8925f0429 100644 --- a/lib/snapshot/snapshot.go +++ b/lib/snapshot/snapshot.go @@ -38,7 +38,7 @@ func Create(createSnapshotURL string) (string, error) { // create Transport tr, err := httputils.Transport(createSnapshotURL, *tlsCertFile, *tlsKeyFile, *tlsCAFile, *tlsServerName, *tlsInsecureSkipVerify) if err != nil { - return "", err + return "", fmt.Errorf("failed to create transport for createSnapshotURL=%q: %s", createSnapshotURL, err) } hc := &http.Client{Transport: tr} @@ -83,7 +83,8 @@ func Delete(deleteSnapshotURL string, snapshotName string) error { // create Transport tr, err := httputils.Transport(deleteSnapshotURL, *tlsCertFile, *tlsKeyFile, *tlsCAFile, *tlsServerName, *tlsInsecureSkipVerify) if err != nil { - return err + return fmt.Errorf("failed to create transport for deleteSnapshotURL=%q: %s", deleteSnapshotURL, err) + } hc := &http.Client{Transport: tr} resp, err := hc.PostForm(u.String(), formData)