rules html doesn't always generate parsable configs

[adamdecaf]

Bug Report

What did you do?

Recording rules can have : on either the front of end of their name. Without quotes in yaml this leads to a parsing error.

Example:

- record: ':kube_pod_info_node_count:'
  expr: sum(min by(node) (kube_pod_info))

Running promtool (or prometheus) over the following gives an error:

$ cat /tmp/mapping-bad.rules.yaml 
groups:
  - name: "node.rules"
    rules:
      - record: :kube_pod_info_node_count:
        expr: sum(min by(node) (kube_pod_info))

$ promtool check rules /tmp/mapping-bad.rules.yaml 
Checking /tmp/mapping-bad.rules.yaml
  FAILED:
yaml: line 3: mapping values are not allowed in this context

What did you expect to see?

I expected for the HTML rules output to be a valid config.

Environment

• Prometheus version:2.2.1

# promtool 
$ git log -n1 
commit b424eb42e328f13eb32149225f020a4159decb17 (HEAD -> master, origin/master, origin/HEAD, adamdecaf/master)
Author: Daisy T <infoverload@users.noreply.github.com>
Date:   Mon Apr 30 20:08:45 2018 +0200

    document remote write queue parameters (#4126)

[adamdecaf]

A fix for this could be to wrap everything in quotes, or just those with a leading (or trailing) :.

[brian-brazil]

This smells like a bug in the yaml library we use.

[juliusv]

Probably that is because the YAML field value itself doesn't start with a colon, but with an HTML tag, only the browser then shows it as starting with a colon, since it interprets the HTML. Maybe we need to add some quoting ourselves around the actual text in the HTML.

[juliusv]

Relevant code piece:

prometheus/rules/recording.go

Lines 112 to 132 in b3ff5f6

	// HTMLSnippet returns an HTML snippet representing this rule.
	func (rule *RecordingRule) HTMLSnippet(pathPrefix string) template.HTML {
	ruleExpr := rule.vector.String()
	labels := make(map[string]string, len(rule.labels))
	for _, l := range rule.labels {
	labels[l.Name] = template.HTMLEscapeString(l.Value)
	}
	r := rulefmt.Rule{
	Record: fmt.Sprintf(`<a href="%s">%s</a>`, pathPrefix+strutil.TableLinkForExpression(rule.name), rule.name),
	Expr: fmt.Sprintf(`<a href="%s">%s</a>`, pathPrefix+strutil.TableLinkForExpression(ruleExpr), template.HTMLEscapeString(ruleExpr)),
	Labels: labels,
	}
	byt, err := yaml.Marshal(r)
	if err != nil {
	return template.HTML(fmt.Sprintf("error marshalling recording rule: %q", err.Error()))
	}
	return template.HTML(byt)
	}

[brian-brazil]

Ah, we're doing our own escaping rather than relying on Go templates. We should probably switch this to Go templates, as they have protections for this sort of thing.

[juliusv]

The problem here is not an HTML escaping issue though, but a YAML field escaping issue. Basically we want to make the YAML field value a link, so we have to interpret it as HTML, but the YAML library doesn't know this.

[brian-brazil]

In either case, I propose we not pass in a blob to template.HTML and hope it gets it right and instead use template.HTML in the way it was designed.

[juliusv]

The problem is that we have three nesting layers: HTML in YAML in HTML. If we don't handle the inner HTML layer ourselves, then we also need to handle the YAML layer in the Go template. So probably best keep handling the inner HTML ourselves.

[brian-brazil]

I propose that we get rid of two of the layers, and do this all up in web. Layering things in this fashion is fragile.

[juliusv]

It's still going to be layered in this way with the same issues no matter where we handle it though? Unless we get rid of the HTML linking within the YAML completely, which I don't think is the plan.

[brian-brazil]

No, we can render it entirely inside one template rather than spreading the logic around.

[juliusv]

Hm, how would that work? Since Go templates are rather primitive in what they can do directly, don't you still need the same kind of external helper functions that do the initial HTML-in-YAML wrapping?

[brian-brazil]

We only need to make it look like YAML, and the structure is pretty simple and unchanging.

[juliusv]

Heh ok, had that possibility in my mind as well of course, but I thought we wouldn't want to go down that road because that seems too dirty (e.g. when we're going to add rule options or other fields, those need to be added to the custom marshaling there too). Well, let's see who actually sends a fix, I still like the idea of manually escaping the YAML field value more :)

[brian-brazil]

I'd rather risk missing some fields when we make a change than introducing an XSS.