From f8edfd851783d0c95fb6089f4ad29052081bd867 Mon Sep 17 00:00:00 2001 From: Karl Fischer Date: Tue, 22 Sep 2020 17:31:08 +0200 Subject: [PATCH] Fix ambiguous file issue --- CHANGELOG.md | 8 +++- README.md | 10 ++++- cli/append.go | 2 +- cli/cat.go | 3 +- cli/cd.go | 10 ++--- cli/command.go | 1 - cli/cp.go | 2 +- cli/grep.go | 3 +- cli/ls.go | 2 +- cli/mv.go | 3 +- cli/rm.go | 3 +- client/type.go | 74 ++++++++++++++++++++++------------ client/util.go | 5 +-- log/log.go | 13 ++++++ main.go | 10 +++-- test/command-tests/append.bats | 8 ++++ test/command-tests/cat.bats | 12 +++++- test/command-tests/cd.bats | 25 ++++++++++++ test/command-tests/cp.bats | 8 ++++ test/command-tests/grep.bats | 8 ++++ test/command-tests/ls.bats | 8 ++++ test/command-tests/mv.bats | 8 ++++ test/command-tests/rm.bats | 8 ++++ test/run.sh | 6 +-- test/util/util.bash | 4 +- 25 files changed, 189 insertions(+), 55 deletions(-) create mode 100644 test/command-tests/cd.bats mode change 100644 => 100755 test/util/util.bash diff --git a/CHANGELOG.md b/CHANGELOG.md index 711b7848..ca637da8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## v0.6.3 (September 22, 2020) + +BUG FIXES: + +* Properly handle ambiguous files without read permission [#46](https://github.com/fishi0x01/vsh/pull/46) - Thank you for detailed report [agaudreault-jive](https://github.com/agaudreault-jive) + ## v0.6.2 (June 18, 2020) ENHANCEMENTS: @@ -7,7 +13,7 @@ ENHANCEMENTS: BUG FIXES: -* Properly handle ambigious files ([#37](https://github.com/fishi0x01/vsh/pull/37)) +* Properly handle ambiguous files ([#37](https://github.com/fishi0x01/vsh/pull/37)) ## v0.6.1 (June 15, 2020) diff --git a/README.md b/README.md index 808f19ba..ae61f30c 100644 --- a/README.md +++ b/README.md @@ -148,10 +148,18 @@ export VAULT_TOKEN= ./vsh -c "rm secret/dir/to/remove/" ``` +## Permission requirements + +`vsh` requires `List` permission on the operated paths. +This is necessary to determine if a path points to a node or leaf in the path tree. +Further, it is needed to gather auto-completion data. + +For operations like `cp` or `mv`, `vsh` additionally requires `Read` and `Write` permissions on the operated paths. + ## Stability Every command has integration tests against KV1 and KV2. -Every test is run against vault `1.0.0` and `1.5.0`, i.e., versions in between should also be compatible. +Every test is run against vault `1.0.0` and `1.5.3`, i.e., versions in between should also be compatible. ## Local Development diff --git a/cli/append.go b/cli/append.go index 715d848c..77fd2f98 100644 --- a/cli/append.go +++ b/cli/append.go @@ -124,7 +124,7 @@ func (cmd *AppendCommand) Run() { src := cmd.client.GetType(newSrcPwd) if src != client.LEAF { - log.Error("Invalid source - must be a leaf: " + newSrcPwd) + log.NotAValidPath(newSrcPwd) return } diff --git a/cli/cat.go b/cli/cat.go index f7dda897..11cd6036 100644 --- a/cli/cat.go +++ b/cli/cat.go @@ -72,8 +72,7 @@ func (cmd *CatCommand) Run() { } } } else { - log.Error("%s%s is not a file", cmd.client.Pwd, cmd.Path) + log.NotAValidPath(absPath) } - return } diff --git a/cli/cd.go b/cli/cd.go index b5a580ea..8b4f0d81 100644 --- a/cli/cd.go +++ b/cli/cd.go @@ -3,6 +3,7 @@ package cli import ( "fmt" "io" + "strings" "github.com/fishi0x01/vsh/client" "github.com/fishi0x01/vsh/log" @@ -56,18 +57,17 @@ func (cmd *CdCommand) Run() { t := cmd.client.GetType(newPwd) if t == client.NONE { - log.Error("Invalid directory: %s", newPwd) + log.NotAValidPath(newPwd) return } if t == client.LEAF { - log.Error("Invalid directory: %s is a file", newPwd) + log.NotAValidPath(newPwd) return } - newPwd = newPwd + "/" - if newPwd == "//" { - newPwd = "/" + if !strings.HasSuffix(newPwd, "/") { + newPwd = newPwd + "/" } cmd.client.Pwd = newPwd return diff --git a/cli/command.go b/cli/command.go index 04814d59..16c46365 100644 --- a/cli/command.go +++ b/cli/command.go @@ -16,7 +16,6 @@ type Command interface { } func cmdPath(pwd string, arg string) (result string) { - strings.HasSuffix(arg, "/") result = filepath.Clean(pwd + arg) if strings.HasSuffix(arg, "/") { diff --git a/cli/cp.go b/cli/cp.go index 678dbe7f..91a3831e 100644 --- a/cli/cp.go +++ b/cli/cp.go @@ -63,7 +63,7 @@ func (cmd *CopyCommand) Run() { case client.NODE: runCommandWithTraverseTwoPaths(cmd.client, newSrcPwd, newTargetPwd, cmd.copySecret) default: - log.Error("Invalid source path: %s", newSrcPwd) + log.NotAValidPath(newSrcPwd) } } diff --git a/cli/grep.go b/cli/grep.go index 9999a452..6d674573 100644 --- a/cli/grep.go +++ b/cli/grep.go @@ -79,7 +79,7 @@ func (cmd *GrepCommand) Run() { filePaths = append(filePaths, traversedPath) } default: - log.Error("Invalid path: %s", path) + log.NotAValidPath(path) return } @@ -92,6 +92,7 @@ func (cmd *GrepCommand) Run() { match.print(cmd.stdout) } } + return } func (cmd *GrepCommand) grepFile(search string, path string) (matches []*Match, err error) { diff --git a/cli/ls.go b/cli/ls.go index ce5136c6..07cef0c4 100644 --- a/cli/ls.go +++ b/cli/ls.go @@ -59,7 +59,7 @@ func (cmd *ListCommand) Run() { result, err := cmd.client.List(newPwd) if err != nil { - log.Error("%w", err) + log.NotAValidPath(newPwd) } else { fmt.Fprintln(cmd.stdout, strings.Join(result, "\n")) } diff --git a/cli/mv.go b/cli/mv.go index 42b8ccff..2f89cbe0 100644 --- a/cli/mv.go +++ b/cli/mv.go @@ -63,8 +63,9 @@ func (cmd *MoveCommand) Run() { case client.NODE: runCommandWithTraverseTwoPaths(cmd.client, newSrcPwd, newTargetPwd, cmd.moveSecret) default: - log.Error("Invalid source path: %s", newSrcPwd) + log.NotAValidPath(newSrcPwd) } + return } func (cmd *MoveCommand) moveSecret(source string, target string) error { diff --git a/cli/rm.go b/cli/rm.go index 7fdd001a..8770eb7a 100644 --- a/cli/rm.go +++ b/cli/rm.go @@ -64,8 +64,9 @@ func (cmd *RemoveCommand) Run() { } } default: - log.Error("Invalid path: %s", newPwd) + log.NotAValidPath(newPwd) } + return } func (cmd *RemoveCommand) removeSecret(path string) error { diff --git a/client/type.go b/client/type.go index 9652e565..ccc41898 100644 --- a/client/type.go +++ b/client/type.go @@ -1,6 +1,9 @@ package client -import "strings" +import ( + "path/filepath" + "strings" +) // PathKind describes the type of a path type PathKind int @@ -23,39 +26,60 @@ func (client *Client) topLevelType(path string) PathKind { } } -func (client *Client) lowLevelType(path string) (result PathKind) { - result = NONE - isNode := false - isLeaf := false - - s, err := client.Vault.Logical().List(client.getKVMetaDataPath(path)) - if err == nil && s != nil { - isNode = true - } - - s, err = client.Vault.Logical().Read(client.getKVDataPath(path)) - if err == nil && s != nil { - isLeaf = true - } +var cachedPath = "" +var cachedDirFiles = make(map[string]int) - if isLeaf && !isNode { - result = LEAF +func (client *Client) isAmbiguous(path string) (result bool) { + // get current directory content + if cachedPath != path { + pathTrim := strings.TrimSuffix(path, "/") + cachedDirFiles = make(map[string]int) + s, err := client.Vault.Logical().List(client.getKVMetaDataPath(filepath.Dir(pathTrim))) + if err == nil && s != nil { + if keysInterface, ok := s.Data["keys"]; ok { + for _, valInterface := range keysInterface.([]interface{}) { + val := valInterface.(string) + cachedDirFiles[val] = 1 + } + } + } + cachedPath = path } - if isNode && !isLeaf { - result = NODE + // check if path exists as file and directory + result = false + if _, ok := cachedDirFiles[filepath.Base(path)]; ok { + if _, ok := cachedDirFiles[filepath.Base(path)+"/"]; ok { + result = true + } } + return result +} - if isLeaf && isNode { - // vault namespace path can overlap with a key, e.g., - // secret/a and secret/a/b - // --> in that case, we have a leaf and a node when checking secret/a +func (client *Client) lowLevelType(path string) (result PathKind) { + if client.isAmbiguous(path) { if strings.HasSuffix(path, "/") { result = NODE } else { result = LEAF } + } else { + hasNode := false + s, err := client.Vault.Logical().List(client.getKVMetaDataPath(path + "/")) + if err == nil && s != nil { + if _, ok := s.Data["keys"]; ok { + hasNode = true + } + } + if hasNode { + result = NODE + } else { + if _, ok := cachedDirFiles[filepath.Base(path)]; ok { + result = LEAF + } else { + result = NONE + } + } } - - return + return result } diff --git a/client/util.go b/client/util.go index f6ec660b..3c8b5023 100644 --- a/client/util.go +++ b/client/util.go @@ -1,7 +1,6 @@ package client import ( - "fmt" "github.com/hashicorp/vault/api" "strings" ) @@ -56,8 +55,8 @@ func (client *Client) kvPath(path string, prefix string) string { case 2: // https://www.vaultproject.io/docs/secrets/kv/kv-v2.html#acl-rules s := strings.SplitN(path, "/", 2) - if len(s) != 2 { - panic(fmt.Errorf("Could not properly split path '%s'", path)) + if len(s) == 1 { + return s[0] + prefix } return s[0] + prefix + s[1] default: diff --git a/log/log.go b/log/log.go index f2d9d7c8..d9eb28f7 100644 --- a/log/log.go +++ b/log/log.go @@ -1,6 +1,7 @@ package log import ( + "fmt" au "github.com/logrusorgru/aurora" "log" "path/filepath" @@ -77,3 +78,15 @@ func Warn(f string, args ...interface{}) { func Error(f string, args ...interface{}) { logger(errorLvl, f, args...) } + +// NotAValidPath prints not a valid path message +func NotAValidPath(f string) { + message := au.Red(au.Sprintf("Not a valid path for operation: %s\n", f)) + fmt.Printf(au.Sprintf(message)) +} + +// NotAValidCommand prints not a valid command +func NotAValidCommand(c string) { + message := au.Red(au.Sprintf("Not a valid command: %s\n", c)) + fmt.Printf(au.Sprintf(message)) +} diff --git a/main.go b/main.go index 4b6a8e2d..85c3a0c2 100644 --- a/main.go +++ b/main.go @@ -66,6 +66,11 @@ func executor(in string) { var cmd cli.Command var run bool + if len(args) == 0 { + fmt.Fprint(os.Stdout, "") + return + } + // Check for built-in commands. switch args[0] { case commands.ls.GetName(): @@ -94,11 +99,8 @@ func executor(in string) { cmd = commands.grep case "exit": os.Exit(0) - case "": - fmt.Fprint(os.Stdout, "") - return default: - log.Error("Unknown command '%s'", args[0]) + log.NotAValidCommand(args[0]) return } diff --git a/test/command-tests/append.bats b/test/command-tests/append.bats index 1313e087..dcd6180e 100644 --- a/test/command-tests/append.bats +++ b/test/command-tests/append.bats @@ -24,6 +24,14 @@ load ../bin/plugins/bats-assert/load assert_success assert_output "test" + ####################################### + echo "==== case: append to non-existing file ====" + run ${APP_BIN} -c "append ${KV_BACKEND}/src/does/not/exist ${KV_BACKEND}/src/aa" + assert_success + + echo "ensure proper error message" + assert_line --partial "Not a valid path: /${KV_BACKEND}/src/does/not/exist" + ####################################### echo "==== case: append value to existing destination with conflicting keys (default merge strategy) ====" run ${APP_BIN} -c "append ${KV_BACKEND}/src/dev/1 ${KV_BACKEND}/dest/prod/all" diff --git a/test/command-tests/cat.bats b/test/command-tests/cat.bats index 3f1b6db9..75bdd6db 100644 --- a/test/command-tests/cat.bats +++ b/test/command-tests/cat.bats @@ -9,11 +9,19 @@ load ../bin/plugins/bats-assert/load assert_success assert_line "value = 1" + ####################################### + echo "==== case: cat non-existing file ====" + run ${APP_BIN} -c "cat ${KV_BACKEND}/src/does/not/exist" + assert_success + + echo "ensure proper error message" + assert_line --partial "Not a valid path: /${KV_BACKEND}/src/does/not/exist" + ####################################### echo "==== case: cat directory ====" run ${APP_BIN} -c "cat ${KV_BACKEND}/src/dev" assert_success - assert_output --partial "is not a file" + assert_line --partial "Not a valid path: /${KV_BACKEND}/src/dev" ####################################### echo "==== case: cat ambigious file ====" @@ -27,5 +35,5 @@ load ../bin/plugins/bats-assert/load echo "==== case: cat ambigious directory ====" run ${APP_BIN} -c "cat ${KV_BACKEND}/src/tooling/" assert_success - assert_output --partial "is not a file" + assert_line --partial "Not a valid path: /${KV_BACKEND}/src/tooling/" } diff --git a/test/command-tests/cd.bats b/test/command-tests/cd.bats new file mode 100644 index 00000000..a4894329 --- /dev/null +++ b/test/command-tests/cd.bats @@ -0,0 +1,25 @@ +load ../util/util +load ../bin/plugins/bats-support/load +load ../bin/plugins/bats-assert/load + +@test "vault-${VAULT_VERSION} ${KV_BACKEND} 'cd'" { + ####################################### + echo "==== case: cd to sub-sub-dir ====" + run ${APP_BIN} -c "cd ${KV_BACKEND}/src/dev" + assert_success + + ####################################### + echo "==== case: cd to KV ====" + run ${APP_BIN} -c "cd ${KV_BACKEND}/" + assert_success + + ####################################### + echo "==== case: cd to KV first level sub-dir ====" + run ${APP_BIN} -c "cd ${KV_BACKEND}/src/" + assert_success + + ####################################### + echo "==== case: cd to non-existing dir ====" + run ${APP_BIN} -c "cd ${KV_BACKEND}/src/does/not/exist" + assert_success +} diff --git a/test/command-tests/cp.bats b/test/command-tests/cp.bats index 54c7346a..da6c6518 100644 --- a/test/command-tests/cp.bats +++ b/test/command-tests/cp.bats @@ -24,6 +24,14 @@ load ../bin/plugins/bats-assert/load assert_success assert_output "test" + ####################################### + echo "==== case: copy non-existing file ====" + run ${APP_BIN} -c "cp ${KV_BACKEND}/src/does/not/exist ${KV_BACKEND}/dest/a" + assert_success + + echo "ensure proper error message" + assert_line --partial "Not a valid path: /${KV_BACKEND}/src/does/not/exist" + ####################################### echo "==== case: copy single directory without trailing '/' ====" run ${APP_BIN} -c "cp ${KV_BACKEND}/src/dev ${KV_BACKEND}/dest/dev" diff --git a/test/command-tests/grep.bats b/test/command-tests/grep.bats index 72504689..1b7238ec 100644 --- a/test/command-tests/grep.bats +++ b/test/command-tests/grep.bats @@ -12,6 +12,14 @@ load ../bin/plugins/bats-assert/load assert_line --partial "/${KV_BACKEND}/src/dev/3" assert_line --partial "/${KV_BACKEND}/src/prod/all" + ####################################### + echo "==== case: grep non-existing file ====" + run ${APP_BIN} -c "grep test ${KV_BACKEND}/src/does/not/exist" + assert_success + + echo "ensure proper error message" + assert_line --partial "Not a valid path: /${KV_BACKEND}/src/does/not/exist" + ####################################### echo "==== case: grep term on ambigious directory ====" run ${APP_BIN} -c "grep juice ${KV_BACKEND}/src/tooling/" diff --git a/test/command-tests/ls.bats b/test/command-tests/ls.bats index 5a143602..b22eee2e 100644 --- a/test/command-tests/ls.bats +++ b/test/command-tests/ls.bats @@ -11,6 +11,14 @@ load ../bin/plugins/bats-assert/load assert_line --index 1 "2" assert_line --index 2 "3" + ####################################### + echo "==== case: ls non-existing dir ====" + run ${APP_BIN} -c "ls ${KV_BACKEND}/src/does/not/exist" + assert_success + + echo "ensure proper error message" + assert_line --partial "Not a valid path: /${KV_BACKEND}/src/does/not/exist" + ####################################### echo "==== case: list backends ====" run ${APP_BIN} -c "ls /" diff --git a/test/command-tests/mv.bats b/test/command-tests/mv.bats index 0605eae4..c6aaba2f 100644 --- a/test/command-tests/mv.bats +++ b/test/command-tests/mv.bats @@ -21,6 +21,14 @@ load ../bin/plugins/bats-assert/load assert_success assert_output --partial "${NO_VALUE_FOUND}" + ####################################### + echo "==== case: move non-existing file ====" + run ${APP_BIN} -c "mv ${KV_BACKEND}/src/does/not/exist ${KV_BACKEND}/src/aa" + assert_success + + echo "ensure proper error message" + assert_line --partial "Not a valid path: /${KV_BACKEND}/src/does/not/exist" + ####################################### echo "==== case: move single directory without trailing '/' ====" run ${APP_BIN} -c "mv ${KV_BACKEND}/src/dev ${KV_BACKEND}/dest/dev" diff --git a/test/command-tests/rm.bats b/test/command-tests/rm.bats index 05056576..38fee778 100644 --- a/test/command-tests/rm.bats +++ b/test/command-tests/rm.bats @@ -17,6 +17,14 @@ load ../bin/plugins/bats-assert/load assert_success assert_output --partial "${NO_VALUE_FOUND}" + ####################################### + echo "==== case: remove non-existing file ====" + run ${APP_BIN} -c "rm ${KV_BACKEND}/src/does/not/exist" + assert_success + + echo "ensure proper error message" + assert_line --partial "Not a valid path: /${KV_BACKEND}/src/does/not/exist" + ####################################### echo "==== case: remove single directory ====" run get_vault_value "value" "${KV_BACKEND}/src/dev/1" diff --git a/test/run.sh b/test/run.sh index bb3c42aa..cd39a18d 100755 --- a/test/run.sh +++ b/test/run.sh @@ -1,7 +1,7 @@ #!/bin/bash set -e # required to fail test suite when a single test fails -VAULT_VERSIONS=("1.5.0" "1.0.0") +VAULT_VERSIONS=("1.5.3" "1.0.0") KV_BACKENDS=("KV1" "KV2") DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" @@ -10,10 +10,10 @@ BATS="${DIR}/bin/core/bin/bats" for vault_version in "${VAULT_VERSIONS[@]}" do - VAULT_VERSION=${vault_version} ${BATS} "${DIR}/special-tests" + VAULT_VERSION=${vault_version} ${BATS} "${DIR}/special-tests/" for kv_backend in "${KV_BACKENDS[@]}" do - VAULT_VERSION=${vault_version} KV_BACKEND="${kv_backend}" ${BATS} "${DIR}/command-tests" + VAULT_VERSION=${vault_version} KV_BACKEND="${kv_backend}" ${BATS} "${DIR}/command-tests/" done done diff --git a/test/util/util.bash b/test/util/util.bash old mode 100644 new mode 100755 index a5c52559..9c555173 --- a/test/util/util.bash +++ b/test/util/util.bash @@ -1,7 +1,7 @@ #!/bin/bash -export VAULT_VERSION=${VAULT_VERSION:-"1.3.4"} -export KV_BACKEND=${KV_BACKEND:-"KV2"} +export VAULT_VERSION=${VAULT_VERSION} +export KV_BACKEND=${KV_BACKEND} export VAULT_CONTAINER_NAME="vsh-integration-test-vault" export VAULT_HOST_PORT=${VAULT_HOST_PORT:-"8888"}