From f7a3e5cbbcb8d4653b2307fca3982f29f9838772 Mon Sep 17 00:00:00 2001
From: Andrew Lytvynov <andrew@gravitational.com>
Date: Thu, 1 Oct 2020 11:00:25 -0700
Subject: [PATCH] Validate node_labels syntax at role creation time

Also improve error output and user validation.
Based on missed feedback in https://github.com/gravitational/teleport/pull/4253#pullrequestreview-499061448
---
 lib/services/role.go     | 12 ++++++++++++
 lib/services/user.go     |  5 ++++-
 lib/utils/parse/parse.go | 14 +++++++++-----
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/lib/services/role.go b/lib/services/role.go
index 790a19ba6c426..2eebded1b083c 100644
--- a/lib/services/role.go
+++ b/lib/services/role.go
@@ -710,6 +710,18 @@ func (r *RoleV3) CheckAndSetDefaults() error {
 		if key == Wildcard && !(len(val) == 1 && val[0] == Wildcard) {
 			return trace.BadParameter("selector *:<val> is not supported")
 		}
+		for _, l := range val {
+			if _, err := parse.NewMatcher(l); err != nil {
+				return trace.Wrap(err)
+			}
+		}
+	}
+	for _, val := range r.Spec.Deny.NodeLabels {
+		for _, l := range val {
+			if _, err := parse.NewMatcher(l); err != nil {
+				return trace.Wrap(err)
+			}
+		}
 	}
 	for i := range r.Spec.Allow.Rules {
 		err := r.Spec.Allow.Rules[i].CheckAndSetDefaults()
diff --git a/lib/services/user.go b/lib/services/user.go
index 99ede69238a5d..911c3261fcce7 100644
--- a/lib/services/user.go
+++ b/lib/services/user.go
@@ -492,7 +492,10 @@ func (u *UserV1) Check() error {
 	}
 	for _, login := range u.AllowedLogins {
 		e, err := parse.NewExpression(login)
-		if err == nil && e.Namespace() != parse.LiteralNamespace {
+		if err != nil {
+			return trace.Wrap(err)
+		}
+		if e.Namespace() != parse.LiteralNamespace {
 			return trace.BadParameter("role variables not allowed in allowed logins")
 		}
 	}
diff --git a/lib/utils/parse/parse.go b/lib/utils/parse/parse.go
index fd1a0cfcf967c..6a48655397e1f 100644
--- a/lib/utils/parse/parse.go
+++ b/lib/utils/parse/parse.go
@@ -175,7 +175,12 @@ type Matcher interface {
 //
 // These expressions do not support variable interpolation (e.g.
 // `{{internal.logins}}`), like Expression does.
-func NewMatcher(value string) (Matcher, error) {
+func NewMatcher(value string) (m Matcher, err error) {
+	defer func() {
+		if err != nil {
+			err = trace.WrapWithMessage(err, "see supported syntax at https://gravitational.com/teleport/docs/enterprise/ssh-rbac/#rbac-for-hosts")
+		}
+	}()
 	match := reVariable.FindStringSubmatch(value)
 	if len(match) == 0 {
 		if strings.Contains(value, "{{") || strings.Contains(value, "}}") {
@@ -191,7 +196,7 @@ func NewMatcher(value string) (Matcher, error) {
 	// parse and get the ast of the expression
 	expr, err := parser.ParseExpr(variable)
 	if err != nil {
-		return nil, trace.BadParameter("failed to parse %q: %v", variable, err)
+		return nil, trace.BadParameter("failed to parse %q: %v", value, err)
 	}
 
 	// walk the ast tree and gather the variable parts
@@ -204,7 +209,7 @@ func NewMatcher(value string) (Matcher, error) {
 	// the matching logic. For example
 	// `{{regexp.match(external.allowed_env_trait)}}`.
 	if result.transform != nil || len(result.parts) > 0 {
-		return nil, trace.BadParameter("%q is not a valid matcher expression - no variables and transformations are allowed", variable)
+		return nil, trace.BadParameter("%q is not a valid matcher expression - no variables and transformations are allowed", value)
 	}
 	return newPrefixSuffixMatcher(prefix, suffix, result.match), nil
 }
@@ -345,8 +350,7 @@ func walk(node ast.Node) (*walkResult, error) {
 					if err != nil {
 						return nil, trace.Wrap(err)
 					}
-					// If this is not_match, wrap the regexpMatcher to invert
-					// it.
+					// If this is not_match, wrap the regexpMatcher to invert it.
 					if fn == RegexpNotMatchFnName {
 						result.match = notMatcher{result.match}
 					}