fishi0x01/vsh

Unnecessary read of secret in `rm` operation

Closed this issue · 8 comments

agaudreault commented

Description

I was looking at the code of the tool, but it seems like a read is performed all the time to know if a secret is a node, a leaf or both in

vsh/client/type.go

Line 36 in a6721cc

s, err = client.Vault.Logical().Read(client.getKVDataPath(path))
.

This is unnecessary and will yield incorrect results in case the user does not have the permission to read the content of the secret.

version

I was using v0.6.2.

Current behaviour

For instance, a user should be able to delete a secret without reading it. For now, the behaviour without read access is:

  • With /secret/foo and /secret/foo/bar:
    • vsh -v -c "rm /secret/foo"
    • [INFO] rm.go:78 Removed /secret/foo/bar <-- This is not what you would expect to happen
  • With /secret/foo and /secret/foo/bar:
    • vsh -v -c "rm /secret/foo/"
    • [INFO] rm.go:78 Removed /secret/foo/bar
  • With /secret/foo only:
    • vsh -v -c "rm /secret/foo"
    • [ERROR] rm.go:67 Invalid path: /secret/foo <-- This is not what you would expect to happen
  • With /secret/foo/bar only:
    • vsh -v -c "rm /secret/foo"
    • [INFO] rm.go:78 Removed /secret/foo/bar <-- This is not what you would expect to happen

Expected behaviour

I would expect the rm operation to only delete the foo secret when /secret/foo is specified, and to delete bar secret when /secret/foo/ is specified.

  • With /secret/foo and /secret/foo/bar:
    • vsh -c "rm /secret/foo"
    • [INFO] Removed /secret/foo
  • With /secret/foo and /secret/foo/bar:
    • vsh -c "rm /secret/foo/"
    • [INFO] Removed /secret/foo/bar
  • With /secret/foo only:
    • vsh -c "rm /secret/foo/"
    • [ERROR] Invalid path: /secret/foo/
  • With /secret/foo/bar only:
    • vsh -c "rm /secret/foo"
    • [ERROR] Invalid secret: /secret/foo

Suggestion

I think checking if the path ends with a / or not is enough to know if the user means a node or a leaf.

Anyhow, if you need to know for other reasons if a secret is a node, leaf or both, looking at the result of the previous folder list output would give the information without reading the secret.

So with /secret/foo/ or /secret/foo specified, you would

  1. Remove trailing slash if present. Result: /secret/foo.
  2. Remove end of string until the slash. Result: /secret/.
  3. Perform a non-recursive list. Result: /secret/foo and /secret/foo/
  4. Check the result contains:
  • /secret/foo: then it's a leaf
  • /secret/foo/: then it's a node

Additional information

Also, few nice things that would be helpful is to:

  • Set the default log level to info so we do not have to specify -v to have feedback.
  • Allow users to specify a log level with -v INFO|WARN|ERROR|...
  • Add the silenced error as debug or trace logs. Very useful to find out these permission problems
  • Document the minimum required vault ACLs to perform the different operations.
fishi0x01 commented

Thank you for the detailed Issue - I appreciate it 🙇

Your observation is absolutely correct, i.e., read is used to determine if a secret is a node or leaf.
The current design of vsh is based on the approach of treating vault's KV storage like a common fs. In a common fs, a file cannot be 2 kinds at the same time, e.g., a special kind of file like a directory and another kind of 'normal' file. However, vault is purely based on full paths to keys being unique, so files can be ambivalent. That case is not covered by the current design choice.

That being said, I absolutely agree with you that the ambivalence of a file being a node and a leaf at the same time can lead to unexpected results and needs to be fixed. I will have a look at your suggestion and see if I can remove the ACL read requirement. Further, I will enhance the documentation about required ACLs as you suggest.

I will try to submit a PR (in the hopefully near future).

  • 👍1
fishi0x01 commented

I created a new release with a fix for the rm bug - the tests looked promising 🤞 https://github.com/fishi0x01/vsh/releases/tag/v0.6.3
The fix is based on your suggestion above.

Concerning logging: The logging situation is a mess right now. I will address those in a new issue which I will open tomorrow. I will refer to this thread.
Again, thank you for the detailed issue report and please let me know if sth is still not working 🙇

  • ❤️1
agaudreault commented

The tests look fine, but somehow I still got the error although it should be covered by case: remove ambigious file without read permissions.

I got

$ vsh -v -c "ls secret/test"
[DEBUG] client.go:68 Cannot auto-discover mount backends: Token does not have list permission on sys/mounts
foo
foo/
$ vsh -v -c "ls secret/test/foo"
[DEBUG] client.go:68 Cannot auto-discover mount backends: Token does not have list permission on sys/mounts
bar
$ vsh -v -c "ls secret/test/foo/"
[DEBUG] client.go:68 Cannot auto-discover mount backends: Token does not have list permission on sys/mounts
bar
$ vsh -v -c "rm secret/test/foo" 
[DEBUG] client.go:68 Cannot auto-discover mount backends: Token does not have list permission on sys/mounts
[INFO] rm.go:78 Removed secret/test/foo/bar
$ vsh -v -c "ls secret/test/foo"
[DEBUG] client.go:68 Cannot auto-discover mount backends: Token does not have list permission on sys/mounts
[ERROR] ls.go:62 %!!(MISSING)!(MISSING)w(*errors.errorString=&{Not a directory: secret/test/foo})
$ vsh -v -c "ls secret/test/"   
[DEBUG] client.go:68 Cannot auto-discover mount backends: Token does not have list permission on sys/mounts
foo

When I executed the ls command with a user with read permission before to delete it, I got a different result for the second operation.

$ vsh -v -c "ls secret/test/"     
[DEBUG] client.go:83 Found KV backend 'secret/' with version '2'
foo
foo/
$ vsh -v -c "ls secret/test/foo" 
[DEBUG] client.go:83 Found KV backend 'secret/' with version '2'
[ERROR] ls.go:62 %!!(MISSING)!(MISSING)w(*errors.errorString=&{Not a directory: secret/test/foo})
$ vsh -v -c "ls secret/test/foo/" 
[DEBUG] client.go:83 Found KV backend 'secret/' with version '2'
bar

The permission for the delete-user is this one. Adding sys/mounts didn't change a thing.

# List all the mounted secrets engines
path "/sys/mounts" {
  capabilities = ["read", "list"]
}

path "secret/*" {
  capabilities = ["list", "delete"]
}

path "secret/metadata/*" {
  capabilities = ["read", "list", "delete"]
}

I am pretty sure list on the metadata path is not a valid operation for K/V v2 🤔 I can get results with vault kv metadata get secret/test/foo.

vsh/client/type.go

Line 37 in 7da8422

s, err := client.Vault.Logical().List(client.getKVMetaDataPath(filepath.Dir(pathTrim)))

fishi0x01 commented

Interesting. I will enhance the tests to properly show this behavior.

fishi0x01 commented

I drafted a new PR #50 which contains a test that should do exactly the same as you mention above for a user without read permissions - however, the test seems to work properly. I am not sure how to reproduce the issue. Do you see something I am missing?

Concerning list() operation on metadata path in KV2 - I think it is compliant with the API doc: https://www.vaultproject.io/api-docs/secret/kv/kv-v2#list-secrets

agaudreault commented

Oh man! 🤦 I realized I had version 0.6.2 locally and that is where the weird error message format came from! Turns out I must have ran go get github.com/fishi0x01/vsh instead of go get -u and it updated the ~/go/bin/vsh executable with the local dependency and not the latest one.

Now I get the new Not a valid path for operation: /secret/test/foo

Thanks a lot for the fix and sorry for that 😞 ! For some reason, vsh -version returns blank.

fishi0x01 commented

Coolio - glad it works 👍

For the sake of completion, I also added a test for vsh -version in the above PR

Thx again for the effort of creating these detailed issue reports. I highly appreciate it 🙇

agaudreault commented

The problem with the version is that I install the module with go get -u so it does not use the binary compiled with the -X main.vshVersion. A brew formula would be really awesome.