Repository: argoproj/argo (go)

Issue:
Validate Workflow metric names
Currently we accept all metric names. This can crash the metrics server.

Acceptable metric name is `[a-zA-Z_:][a-zA-Z_:0-9]*`

Pull request:
fix: Enforce metric naming validation
Fixes #2817 

(#2815 and #2818 to be fixed in separate PR)

Patch:
diff --git a/USERS.md b/USERS.md
index 6f6ab7aa1..0b8ede241 100644
--- a/USERS.md
+++ b/USERS.md
@@ -46,6 +46,7 @@ Currently, the following organizations are **officially** using Argo Workflows:
 1. [Localytics](https://www.localytics.com/)
 1. [Maersk](https://www.maersk.com/solutions/digital-solutions)
 1. [Max Kelsen](https://maxkelsen.com/)
+1. [Microba](https://www.microba.com/)
 1. [Mirantis](https://mirantis.com/)
 1. [NVIDIA](https://www.nvidia.com/)
 1. [OVH](https://www.ovh.com/)
diff --git a/cmd/argo/commands/client/conn.go b/cmd/argo/commands/client/conn.go
index d65bdb395..210b1b09f 100644
--- a/cmd/argo/commands/client/conn.go
+++ b/cmd/argo/commands/client/conn.go
@@ -37,7 +37,7 @@ func AddArgoServerFlagsToCmd(cmd *cobra.Command) {
 	// "-e" for encrypted - like zip
 	cmd.PersistentFlags().BoolVarP(&argoServerOpts.Secure, "secure", "e", os.Getenv("ARGO_SECURE") == "true", "Whether or not the server is using TLS with the Argo Server. Defaults to the ARGO_SECURE environment variable.")
 	// "-k" like curl
-	cmd.PersistentFlags().BoolVarP(&argoServerOpts.InsecureSkipVerify, "insecure-skip-verify", "k", os.Getenv("ARGO_INSECURE_SKIP_VERIFY") == "true", "If true, the Argo Server's certificate will not be checked for validity. This will make your HTTPS connections insecure. Defaults to the ARGO_SECURE environment variable.")
+	cmd.PersistentFlags().BoolVarP(&argoServerOpts.InsecureSkipVerify, "insecure-skip-verify", "k", os.Getenv("ARGO_INSECURE_SKIP_VERIFY") == "true", "If true, the Argo Server's certificate will not be checked for validity. This will make your HTTPS connections insecure. Defaults to the ARGO_INSECURE_SKIP_VERIFY environment variable.")
 }
 
 func NewAPIClient() (context.Context, apiclient.Client) {
diff --git a/docs/metrics.md b/docs/metrics.md
index 85157afa8..db7a15475 100644
--- a/docs/metrics.md
+++ b/docs/metrics.md
@@ -92,6 +92,8 @@ When defining a `histogram`, `buckets` must also be provided (see below).
 
 [Argo variables](variables.md) can be included anywhere in the metric spec, such as in `labels`, `name`, `help`, `when`, etc.
 
+Metric names can only contain alphanumeric characters, `_`, and `:`.
+
 ### Metric Spec
  
  In Argo you can define a metric on the `Workflow` level or on the `Template` level. Here is an example of a `Workflow`
@@ -208,4 +210,4 @@ To define a realtime metric simply add `realtime: true` to a gauge metric with a
   gauge:
     realtime: true
     value: "{{duration}}"
-```
\ No newline at end of file
+```
diff --git a/docs/tls.md b/docs/tls.md
index 7a1cd3514..6515bd0e0 100644
--- a/docs/tls.md
+++ b/docs/tls.md
@@ -2,7 +2,7 @@
 
 ![alpha](assets/alpha.svg)
 
-> v2.9 and after
+> v2.8 and after
 
 If you're running Argo Server you have three options with increasing transport security (note - you should also be running [authentication](argo-server.md#auth-mode)):
 
@@ -64,5 +64,5 @@ argo --secure list
 
 ```
 export ARGO_SECURE=true
-argo --secure --insecure-skip-verify list
+argo list
 ```
diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go
index 24836a20e..e24334f16 100644
--- a/workflow/controller/operator.go
+++ b/workflow/controller/operator.go
@@ -2434,6 +2434,15 @@ func (woc *wfOperationCtx) computeMetrics(metricList []*wfv1.Prometheus, localSc
 			continue
 		}
 
+		if !validate.MetricNameRegex.MatchString(metricTmpl.Name) {
+			woc.reportMetricEmissionError(fmt.Sprintf("metric name '%s' is invalid. Metric names must contain alphanumeric characters, '_', or ':'", metricTmpl.Name))
+			continue
+		}
+		if metricTmpl.Help == "" {
+			woc.reportMetricEmissionError(fmt.Sprintf("metric '%s' must contain a help string under 'help: ' field", metricTmpl.Name))
+			continue
+		}
+
 		// Substitute parameters in non-value fields of the template to support variables in places such as labels,
 		// name, and help. We do not substitute value fields here (i.e. gauge, histogram, counter) here because they
 		// might be realtime ({{workflow.duration}} will not be substituted the same way if it's realtime or if it isn't).
diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go
index 15a4522e7..732c6fb0f 100644
--- a/workflow/validate/validate.go
+++ b/workflow/validate/validate.go
@@ -302,6 +302,7 @@ func (ctx *templateValidationCtx) validateTemplate(tmpl *wfv1.Template, tmplCtx
 	if err != nil {
 		return err
 	}
+
 	localParams := make(map[string]string)
 	if tmpl.IsPodType() {
 		localParams[common.LocalVarPodName] = placeholderGenerator.NextPlaceholder()
@@ -362,6 +363,16 @@ func (ctx *templateValidationCtx) validateTemplate(tmpl *wfv1.Template, tmplCtx
 			return err
 		}
 	}
+	if newTmpl.Metrics != nil {
+		for _, metric := range newTmpl.Metrics.Prometheus {
+			if !MetricNameRegex.MatchString(metric.Name) {
+				return errors.Errorf(errors.CodeBadRequest, "templates.%s metric name '%s' is invalid. Metric names must contain alphanumeric characters, '_', or ':'", tmpl.Name, metric.Name)
+			}
+			if metric.Help == "" {
+				return errors.Errorf(errors.CodeBadRequest, "templates.%s metric '%s' must contain a help string under 'help: ' field", tmpl.Name, metric.Name)
+			}
+		}
+	}
 	return nil
 }
 
@@ -1167,6 +1178,8 @@ var (
 	// paramRegex matches a parameter. e.g. {{inputs.parameters.blah}}
 	paramRegex               = regexp.MustCompile(`{{[-a-zA-Z0-9]+(\.[-a-zA-Z0-9_]+)*}}`)
 	paramOrArtifactNameRegex = regexp.MustCompile(`^[-a-zA-Z0-9_]+[-a-zA-Z0-9_]*$`)
+	workflowFieldNameRegex   = regexp.MustCompile("^" + workflowFieldNameFmt + "$")
+	MetricNameRegex          = regexp.MustCompile(`^[a-zA-Z_:][a-zA-Z_:0-9]*$`)
 )
 
 func isParameter(p string) bool {
@@ -1187,15 +1200,13 @@ const (
 	workflowFieldMaxLength  int    = 128
 )
 
-var workflowFieldNameRegexp = regexp.MustCompile("^" + workflowFieldNameFmt + "$")
-
 // isValidWorkflowFieldName : workflow field name must consist of alpha-numeric characters or '-', and must start with an alpha-numeric character
 func isValidWorkflowFieldName(name string) []string {
 	var errs []string
 	if len(name) > workflowFieldMaxLength {
 		errs = append(errs, apivalidation.MaxLenError(workflowFieldMaxLength))
 	}
-	if !workflowFieldNameRegexp.MatchString(name) {
+	if !workflowFieldNameRegex.MatchString(name) {
 		msg := workflowFieldNameErrMsg + " (e.g. My-name1-2, 123-NAME)"
 		errs = append(errs, msg)
 	}


Test patch:
diff --git a/workflow/validate/validate_test.go b/workflow/validate/validate_test.go
index ec5a552de..6c9635674 100644
--- a/workflow/validate/validate_test.go
+++ b/workflow/validate/validate_test.go
@@ -2292,3 +2292,50 @@ func TestStepWithItemParam(t *testing.T) {
 	_, err := validate(stepWithItemParam)
 	assert.NoError(t, err)
 }
+
+var invalidMetricName = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: hello-world-
+spec:
+  entrypoint: whalesay
+  templates:
+  - name: whalesay
+    metrics:
+      prometheus:
+        - name: invalid.metric.name
+          help: "invalid"
+          gauge:
+            value: 1
+    container:
+      image: docker/whalesay:latest
+`
+
+func TestInvalidMetricName(t *testing.T) {
+	_, err := validate(invalidMetricName)
+	assert.EqualError(t, err, "templates.whalesay metric name 'invalid.metric.name' is invalid. Metric names must contain alphanumeric characters, '_', or ':'")
+}
+
+var invalidMetricHelp = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: hello-world-
+spec:
+  entrypoint: whalesay
+  templates:
+  - name: whalesay
+    metrics:
+      prometheus:
+        - name: metric_name
+          gauge:
+            value: 1
+    container:
+      image: docker/whalesay:latest
+`
+
+func TestInvalidMetricHelp(t *testing.T) {
+	_, err := validate(invalidMetricHelp)
+	assert.EqualError(t, err, "templates.whalesay metric 'metric_name' must contain a help string under 'help: ' field")
+}