resolve additional matching issue (#11)

Summary:
Pull Request resolved: #11

Go's Match string function is similar to a substring matcher, as opposed to python's regex `match` function which matches from the beginning of the string. For example
`cat.*` will match the command `bash /etc/some_folder/file_name_contains_cat.sh`
as per go matchers.
An easy way to restrict this is to add boundary characters inside expressions as tacquito reads them.
There are two places we could add these boundary characters
1) config
2) the server code

Within facebook's infra , it would be okay to attach this to either of the places since we control the config. But the stringy module is a part of the OSS modules, and in that case it's better to add the guardrails to the server code so the boundaries are not *optional*

Reviewed By: elizabethjoshi

Differential Revision: D64148191

fbshipit-source-id: 16f45c8a3e21c4d5d0509d9fe20a52fdedcbc22c

Author: Surya Ahuja

authorize_fields.go

@@ -134,7 +134,7 @@ func (t Args) Validate(condition interface{}) error {
 	return nil
 }
 
-// String returns Args as string, ignoring <cr> cmd-arg=<cr>
+// String returns Args as string
 func (t Args) String() string {
 	var b strings.Builder
 	for _, arg := range t {
@@ -194,6 +194,28 @@ func (t Args) CommandArgs() string {
 	return strings.Join(args, " ")
 }
 
+// isLineEnding returns true if a is a valid line ending for tacacs authorization
+// payload
+func isLineEnding(a string) bool {
+	return a == "<cr>" || a == "<CR>"
+}
+
+// CommandArgsNoLE joins all cmd-arg args into a single string
+// and ignores line endings, specifically <cr>
+func (t Args) CommandArgsNoLE() string {
+	args := make([]string, 0, len(t))
+	for idx, arg := range t {
+		a, _, v := arg.ASV()
+		if a == "cmd-arg" {
+			if idx == len(t)-1 && isLineEnding(v) {
+				continue
+			}
+			args = append(args, v)
+		}
+	}
+	return strings.Join(args, " ")
+}
+
 // Args splits the Args into cmd, cmd-arg and other=arg
 // the key is the left side of the delimiter, etc
 func (t Args) Args() []string {

authorize_fields_test.go

@@ -0,0 +1,40 @@
+/*
+ Copyright (c) Facebook, Inc. and its affiliates.
+
+ This source code is licensed under the MIT license found in the
+ LICENSE file in the root directory of this source tree.
+*/
+
+package tacquito
+
+import (
+	"testing"
+)
+
+func TestArgsStripCR(t *testing.T) {
+	args := Args{
+		"cmd=show",
+		"cmd-arg=version",
+		"cmd-arg=<cr>",
+	}
+	expected := "version"
+	if v := args.CommandArgsNoLE(); v != expected {
+		t.Fatalf("failed to get command args, expected %s, got %s", expected, v)
+	}
+}
+
+func TestArgsStripCRInMiddle(t *testing.T) {
+	args := Args{
+		"cmd=show",
+		"cmd-arg=version",
+		"cmd-arg=<cr>",
+		"cmd-arg=actual",
+		"cmd-arg=line",
+		"cmd-arg=ending",
+		"cmd-arg=<cr>",
+	}
+	expected := "version <cr> actual line ending"
+	if v := args.CommandArgsNoLE(); v != expected {
+		t.Fatalf("failed to get command args, expected %s, got %s", expected, v)
+	}
+}

cmds/server/config/authorizers/stringy/command.go

@@ -16,6 +16,15 @@ import (
 	"github.com/facebookincubator/tacquito/cmds/server/config"
 )
 
+const (
+	// default anchors for regex expressions embedded in command match attributes
+	// stored as bytes and strs for matching and concatenation
+	regexStartByte = '^'
+	regexEndByte   = '$'
+	regexStartStr  = "^"
+	regexEndStr    = "$"
+)
+
 // NewCommandBasedAuthorizer will return a CommandBasedAuthorizer authorizer. If initial request params
 // are not suitable for command based, it returns nil
 func NewCommandBasedAuthorizer(ctx context.Context, l loggerProvider, b tq.AuthorRequest, u config.User) *CommandBasedAuthorizer {
@@ -80,7 +89,6 @@ func (a CommandBasedAuthorizer) evaluate() bool {
 			return false
 		}
 	}
-
 	for _, c := range a.user.Commands {
 		c.TrimSpace()
 		if c.Name == "*" {
@@ -94,8 +102,19 @@ func (a CommandBasedAuthorizer) evaluate() bool {
 			// cmd matches, but we have no conditions, so match it
 			return returnBool(c.Action)
 		}
+
 		for _, regexish := range c.Match {
-			if matched, err := regexp.MatchString(regexish, a.body.Args.CommandArgs()); err != nil {
+			if len(regexish) == 0 {
+				continue
+			}
+			// guard against regexes that are not anchored to the start and end of the string
+			if regexish[0] != regexStartByte {
+				regexish = regexStartStr + regexish
+			}
+			if regexish[len(regexish)-1] != regexEndByte {
+				regexish = regexish + regexEndStr
+			}
+			if matched, err := regexp.MatchString(regexish, a.body.Args.CommandArgsNoLE()); err != nil {
 				a.Errorf(a.ctx, "bad regex detected; %v", err)
 				return false
 			} else if matched {

cmds/server/config/authorizers/stringy/test/stringy_test.go

@@ -152,6 +152,44 @@ func TestCommands(t *testing.T) {
 				}
 			},
 		},
+		{
+			name: "cisco; service=shell, cmd=check for regex boundaries",
+			user: config.User{
+				Name: "cisco",
+				Commands: []config.Command{
+					{
+						Name:   "bash",
+						Match:  []string{"cat.*"},
+						Action: config.PERMIT,
+					},
+				},
+			},
+			request: newAuthorRequest("cisco", tq.Args{"service=shell", "cmd=bash", "cmd-arg=/etc/some_folder/file_name_contains_cat.sh"}),
+			validate: func(name string, response *mockedResponse) {
+				if response.got.Status != tq.AuthorStatusFail {
+					assert.Fail(t, fmt.Sprintf("[%v] should have had a status of [%v] but got [%v]", name, tq.AuthorStatusFail, response.got.Status))
+				}
+			},
+		},
+		{
+			name: "cisco; service=shell, cmd=check that boundaries are not added if they are already set",
+			user: config.User{
+				Name: "cisco",
+				Commands: []config.Command{
+					{
+						Name:   "bash",
+						Match:  []string{"^cat.*$"},
+						Action: config.PERMIT,
+					},
+				},
+			},
+			request: newAuthorRequest("cisco", tq.Args{"service=shell", "cmd=bash", "cmd-arg=cat", "cmd-arg=/etc/some_folder/file_name_contains_cat.sh"}),
+			validate: func(name string, response *mockedResponse) {
+				if response.got.Status != tq.AuthorStatusPassAdd {
+					assert.Fail(t, fmt.Sprintf("[%v] should have had a status of [%v] but got [%v]", name, tq.AuthorStatusPassAdd, response.got.Status))
+				}
+			},
+		},
 	}
 	for _, test := range tests {
 		logger.Infof(ctx, "running test [%v]", test.name)