From a952d324fef32994cfd7dfa15d020a2100566347 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 22 Dec 2016 02:46:23 -0500 Subject: [PATCH 1/7] physical: file backend to have key base64 URL encoded --- physical/file.go | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/physical/file.go b/physical/file.go index 25feee054..49314eb14 100644 --- a/physical/file.go +++ b/physical/file.go @@ -1,6 +1,7 @@ package physical import ( + "encoding/base64" "encoding/json" "fmt" "io" @@ -49,9 +50,19 @@ func (b *FileBackend) Delete(path string) error { defer b.l.Unlock() basePath, key := b.path(path) - fullPath := filepath.Join(basePath, key) + fullPath := filepath.Join(basePath, "_"+key) err := os.Remove(fullPath) + + // if the file does not exist and if the key is base64 URL encoded, try to delete the file with decoded key + if err != nil && os.IsNotExist(err) { + keyDecodedBytes, err := base64.URLEncoding.DecodeString(key) + if err == nil { + fullPath = filepath.Join(basePath, "_"+string(keyDecodedBytes)) + err = os.Remove(fullPath) + } + } + if err != nil && !os.IsNotExist(err) { return fmt.Errorf("Failed to remove %q: %v", fullPath, err) } @@ -100,14 +111,19 @@ func (b *FileBackend) Get(k string) (*Entry, error) { defer b.l.Unlock() path, key := b.path(k) - path = filepath.Join(path, key) + fullPath := filepath.Join(path, "_"+key) + + f, err := os.Open(fullPath) + + if err != nil && os.IsNotExist(err) { + fullPath = filepath.Join(path, "_"+base64.URLEncoding.EncodeToString([]byte(key))) + f, err = os.Open(fullPath) + } - f, err := os.Open(path) if err != nil { if os.IsNotExist(err) { return nil, nil } - return nil, err } defer f.Close() @@ -123,6 +139,8 @@ func (b *FileBackend) Get(k string) (*Entry, error) { func (b *FileBackend) Put(entry *Entry) error { path, key := b.path(entry.Key) + key = base64.URLEncoding.EncodeToString([]byte(key)) + b.l.Lock() defer b.l.Unlock() @@ -131,9 +149,11 @@ func (b *FileBackend) Put(entry *Entry) error { return err } + fullPath := filepath.Join(path, "_"+key) + // JSON encode the entry and write it f, err := os.OpenFile( - filepath.Join(path, key), + fullPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0600) if err != nil { @@ -172,6 +192,7 @@ func (b *FileBackend) List(prefix string) ([]string, error) { for i, name := range names { if name[0] == '_' { names[i] = name[1:] + // TODO: Decode the name } else { names[i] = name + "/" } @@ -184,5 +205,5 @@ func (b *FileBackend) path(k string) (string, string) { path := filepath.Join(b.Path, k) key := filepath.Base(path) path = filepath.Dir(path) - return path, "_" + key + return path, key } From 17652b486d857752e613b9a4d4a6140bd3e61b8a Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 22 Dec 2016 12:03:28 -0500 Subject: [PATCH 2/7] physical/file: Fix the deletion flow --- physical/file.go | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/physical/file.go b/physical/file.go index 49314eb14..cabb953c1 100644 --- a/physical/file.go +++ b/physical/file.go @@ -49,18 +49,14 @@ func (b *FileBackend) Delete(path string) error { b.l.Lock() defer b.l.Unlock() - basePath, key := b.path(path) - fullPath := filepath.Join(basePath, "_"+key) + basePath, fileName := b.path(path) + fullPath := filepath.Join(basePath, "_"+fileName) err := os.Remove(fullPath) - // if the file does not exist and if the key is base64 URL encoded, try to delete the file with decoded key if err != nil && os.IsNotExist(err) { - keyDecodedBytes, err := base64.URLEncoding.DecodeString(key) - if err == nil { - fullPath = filepath.Join(basePath, "_"+string(keyDecodedBytes)) - err = os.Remove(fullPath) - } + fullPath = filepath.Join(basePath, "_"+base64.URLEncoding.EncodeToString([]byte(fileName))) + err = os.Remove(fullPath) } if err != nil && !os.IsNotExist(err) { @@ -110,13 +106,13 @@ func (b *FileBackend) Get(k string) (*Entry, error) { b.l.Lock() defer b.l.Unlock() - path, key := b.path(k) - fullPath := filepath.Join(path, "_"+key) + basePath, fileName := b.path(k) + fullPath := filepath.Join(basePath, "_"+fileName) f, err := os.Open(fullPath) if err != nil && os.IsNotExist(err) { - fullPath = filepath.Join(path, "_"+base64.URLEncoding.EncodeToString([]byte(key))) + fullPath = filepath.Join(basePath, "_"+base64.URLEncoding.EncodeToString([]byte(fileName))) f, err = os.Open(fullPath) } @@ -137,19 +133,19 @@ func (b *FileBackend) Get(k string) (*Entry, error) { } func (b *FileBackend) Put(entry *Entry) error { - path, key := b.path(entry.Key) + basePath, fileName := b.path(entry.Key) - key = base64.URLEncoding.EncodeToString([]byte(key)) + fileName = base64.URLEncoding.EncodeToString([]byte(fileName)) b.l.Lock() defer b.l.Unlock() // Make the parent tree - if err := os.MkdirAll(path, 0755); err != nil { + if err := os.MkdirAll(basePath, 0755); err != nil { return err } - fullPath := filepath.Join(path, "_"+key) + fullPath := filepath.Join(basePath, "_"+fileName) // JSON encode the entry and write it f, err := os.OpenFile( @@ -192,7 +188,10 @@ func (b *FileBackend) List(prefix string) ([]string, error) { for i, name := range names { if name[0] == '_' { names[i] = name[1:] - // TODO: Decode the name + nameDecodedBytes, err := base64.URLEncoding.DecodeString(names[i]) + if err == nil { + names[i] = string(nameDecodedBytes) + } } else { names[i] = name + "/" } @@ -202,8 +201,6 @@ func (b *FileBackend) List(prefix string) ([]string, error) { } func (b *FileBackend) path(k string) (string, string) { - path := filepath.Join(b.Path, k) - key := filepath.Base(path) - path = filepath.Dir(path) - return path, key + fullPath := filepath.Join(b.Path, k) + return filepath.Dir(fullPath), filepath.Base(fullPath) } From cbccf9869dd0aa42f009d8940f791f56c970c637 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 22 Dec 2016 14:10:40 -0500 Subject: [PATCH 3/7] physical/file: Handle file duplication case while updating --- physical/file.go | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/physical/file.go b/physical/file.go index cabb953c1..262d8b49f 100644 --- a/physical/file.go +++ b/physical/file.go @@ -52,8 +52,11 @@ func (b *FileBackend) Delete(path string) error { basePath, fileName := b.path(path) fullPath := filepath.Join(basePath, "_"+fileName) + // For backwards compatibility, try to delete the file without base64 URL + // encoding the file name. If such a file does not exist, it could mean + // that the file name is encoded. Try deleting the file with the encoded + // file name. err := os.Remove(fullPath) - if err != nil && os.IsNotExist(err) { fullPath = filepath.Join(basePath, "_"+base64.URLEncoding.EncodeToString([]byte(fileName))) err = os.Remove(fullPath) @@ -109,8 +112,9 @@ func (b *FileBackend) Get(k string) (*Entry, error) { basePath, fileName := b.path(k) fullPath := filepath.Join(basePath, "_"+fileName) + // If non-encoded file name is a valid storage entry, read it out. + // Otherwise, encode the file name and look for the storage entry. f, err := os.Open(fullPath) - if err != nil && os.IsNotExist(err) { fullPath = filepath.Join(basePath, "_"+base64.URLEncoding.EncodeToString([]byte(fileName))) f, err = os.Open(fullPath) @@ -133,19 +137,39 @@ func (b *FileBackend) Get(k string) (*Entry, error) { } func (b *FileBackend) Put(entry *Entry) error { + if entry == nil { + return fmt.Errorf("nil entry") + } + + var err error basePath, fileName := b.path(entry.Key) - fileName = base64.URLEncoding.EncodeToString([]byte(fileName)) + // New storage entries will have their file names base64 URL encoded. If a + // file with a non-encoded file name exists, it indicates that this is an + // update operation. To avoid duplication of storage entries, delete the + // old entry first. + fullPath := filepath.Join(basePath, "_"+fileName) + if _, err = os.Stat(fullPath); !os.IsNotExist(err) { + err = b.Delete(entry.Key) + if err != nil { + return fmt.Errorf("failed to remove old entry: %v", err) + } + } b.l.Lock() defer b.l.Unlock() // Make the parent tree - if err := os.MkdirAll(basePath, 0755); err != nil { + if err = os.MkdirAll(basePath, 0755); err != nil { return err } - fullPath := filepath.Join(basePath, "_"+fileName) + // base64 URL encode the file name to make all the characters compatible by + // the host OS (specially Windows). However, the basePath can contain + // disallowed characters. Encoding all the directory names and the file + // name is an over kill, and encoding all at once will flatten the file + // system, which *may* not be desire. + fullPath = filepath.Join(basePath, "_"+base64.URLEncoding.EncodeToString([]byte(fileName))) // JSON encode the entry and write it f, err := os.OpenFile( @@ -188,6 +212,8 @@ func (b *FileBackend) List(prefix string) ([]string, error) { for i, name := range names { if name[0] == '_' { names[i] = name[1:] + // If the file name is encoded, decode it to retain the list output + // meaningful. nameDecodedBytes, err := base64.URLEncoding.DecodeString(names[i]) if err == nil { names[i] = string(nameDecodedBytes) From d2026364c785b86c491605c70fc4b638878c7460 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Fri, 23 Dec 2016 17:09:44 -0500 Subject: [PATCH 4/7] physical/file: added test for base64 encoding the storage file names --- physical/file.go | 4 +- physical/file_test.go | 122 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 2 deletions(-) diff --git a/physical/file.go b/physical/file.go index 262d8b49f..f18fffe2b 100644 --- a/physical/file.go +++ b/physical/file.go @@ -167,8 +167,8 @@ func (b *FileBackend) Put(entry *Entry) error { // base64 URL encode the file name to make all the characters compatible by // the host OS (specially Windows). However, the basePath can contain // disallowed characters. Encoding all the directory names and the file - // name is an over kill, and encoding all at once will flatten the file - // system, which *may* not be desire. + // name is an over kill, and encoding the fullPath will flatten the + // storage, which *may* not be desired. fullPath = filepath.Join(basePath, "_"+base64.URLEncoding.EncodeToString([]byte(fileName))) // JSON encode the entry and write it diff --git a/physical/file_test.go b/physical/file_test.go index 50be4011c..9810f4beb 100644 --- a/physical/file_test.go +++ b/physical/file_test.go @@ -1,14 +1,136 @@ package physical import ( + "encoding/json" "io/ioutil" "os" + "path/filepath" + "reflect" "testing" "github.com/hashicorp/vault/helper/logformat" log "github.com/mgutz/logxi/v1" ) +func TestFileBackend_Base64URLEncoding(t *testing.T) { + backendPath, err := ioutil.TempDir("", "vault") + if err != nil { + t.Fatalf("err: %s", err) + } + defer os.RemoveAll(backendPath) + + logger := logformat.NewVaultLogger(log.LevelTrace) + + b, err := NewBackend("file", logger, map[string]string{ + "path": backendPath, + }) + if err != nil { + t.Fatalf("err: %s", err) + } + + // List the entries. Length should be zero. + keys, err := b.List("") + if err != nil { + t.Fatalf("err: %v", err) + } + if len(keys) != 0 { + t.Fatalf("bad: len(keys): expected: 0, actual: %d", len(keys)) + } + + // Create a storage entry without base64 encoding the file name + rawFullPath := filepath.Join(backendPath, "_foo") + e := &Entry{Key: "foo", Value: []byte("test")} + f, err := os.OpenFile( + rawFullPath, + os.O_CREATE|os.O_TRUNC|os.O_WRONLY, + 0600) + if err != nil { + t.Fatal(err) + } + json.NewEncoder(f).Encode(e) + f.Close() + + // Get should work + out, err := b.Get("foo") + if err != nil { + t.Fatalf("err: %v", err) + } + if !reflect.DeepEqual(out, e) { + t.Fatalf("bad: %v expected: %v", out, e) + } + + // List the entries. There should be one entry. + keys, err = b.List("") + if err != nil { + t.Fatalf("err: %v", err) + } + if len(keys) != 1 { + t.Fatalf("bad: len(keys): expected: 1, actual: %d", len(keys)) + } + + err = b.Put(e) + if err != nil { + t.Fatalf("err: %v", err) + } + + // List the entries again. There should still be one entry. + keys, err = b.List("") + if err != nil { + t.Fatalf("err: %v", err) + } + if len(keys) != 1 { + t.Fatalf("bad: len(keys): expected: 1, actual: %d", len(keys)) + } + + // Get should work + out, err = b.Get("foo") + if err != nil { + t.Fatalf("err: %v", err) + } + if !reflect.DeepEqual(out, e) { + t.Fatalf("bad: %v expected: %v", out, e) + } + + err = b.Delete("foo") + if err != nil { + t.Fatalf("err: %v", err) + } + + out, err = b.Get("foo") + if err != nil { + t.Fatalf("err: %v", err) + } + if out != nil { + t.Fatalf("bad: entry: expected: nil, actual: %#v", e) + } + + keys, err = b.List("") + if err != nil { + t.Fatalf("err: %v", err) + } + if len(keys) != 0 { + t.Fatalf("bad: len(keys): expected: 0, actual: %d", len(keys)) + } + + f, err = os.OpenFile( + rawFullPath, + os.O_CREATE|os.O_TRUNC|os.O_WRONLY, + 0600) + if err != nil { + t.Fatal(err) + } + json.NewEncoder(f).Encode(e) + f.Close() + + keys, err = b.List("") + if err != nil { + t.Fatalf("err: %v", err) + } + if len(keys) != 1 { + t.Fatalf("bad: len(keys): expected: 1, actual: %d", len(keys)) + } +} + func TestFileBackend(t *testing.T) { dir, err := ioutil.TempDir("", "vault") if err != nil { From 8b579d47a95e659ee5d4eb822a135577f6192baf Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Fri, 13 Jan 2017 03:39:33 -0500 Subject: [PATCH 5/7] address review feedback --- physical/file.go | 116 +++++++++++++++++++++++++++-------------------- 1 file changed, 66 insertions(+), 50 deletions(-) diff --git a/physical/file.go b/physical/file.go index f18fffe2b..6031b3d16 100644 --- a/physical/file.go +++ b/physical/file.go @@ -12,6 +12,7 @@ import ( log "github.com/mgutz/logxi/v1" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/vault/helper/jsonutil" ) @@ -49,21 +50,16 @@ func (b *FileBackend) Delete(path string) error { b.l.Lock() defer b.l.Unlock() - basePath, fileName := b.path(path) - fullPath := filepath.Join(basePath, "_"+fileName) - - // For backwards compatibility, try to delete the file without base64 URL - // encoding the file name. If such a file does not exist, it could mean - // that the file name is encoded. Try deleting the file with the encoded - // file name. - err := os.Remove(fullPath) + _, fullPathPrefixedFileName, fullPathPrefixedEncodedFileName := b.path(path) + err := os.Remove(fullPathPrefixedEncodedFileName) if err != nil && os.IsNotExist(err) { - fullPath = filepath.Join(basePath, "_"+base64.URLEncoding.EncodeToString([]byte(fileName))) - err = os.Remove(fullPath) + // For backwards compatibility, try to delete the file without base64 + // URL encoding the file name. + err = os.Remove(fullPathPrefixedFileName) } if err != nil && !os.IsNotExist(err) { - return fmt.Errorf("Failed to remove %q: %v", fullPath, err) + return fmt.Errorf("Failed to remove %q: %v", fullPathPrefixedFileName, err) } err = b.cleanupLogicalPath(path) @@ -105,19 +101,16 @@ func (b *FileBackend) cleanupLogicalPath(path string) error { return nil } -func (b *FileBackend) Get(k string) (*Entry, error) { +func (b *FileBackend) Get(path string) (*Entry, error) { b.l.Lock() defer b.l.Unlock() - basePath, fileName := b.path(k) - fullPath := filepath.Join(basePath, "_"+fileName) - - // If non-encoded file name is a valid storage entry, read it out. - // Otherwise, encode the file name and look for the storage entry. - f, err := os.Open(fullPath) + _, fullPathPrefixedFileName, fullPathPrefixedEncodedFileName := b.path(path) + f, err := os.Open(fullPathPrefixedEncodedFileName) if err != nil && os.IsNotExist(err) { - fullPath = filepath.Join(basePath, "_"+base64.URLEncoding.EncodeToString([]byte(fileName))) - f, err = os.Open(fullPath) + // For backwards compatibility, if non-encoded file name is a valid + // storage entry, read it out. + f, err = os.Open(fullPathPrefixedFileName) } if err != nil { @@ -137,51 +130,60 @@ func (b *FileBackend) Get(k string) (*Entry, error) { } func (b *FileBackend) Put(entry *Entry) error { + var retErr error if entry == nil { - return fmt.Errorf("nil entry") + retErr = multierror.Append(retErr, fmt.Errorf("nil entry")) + return retErr } - var err error - basePath, fileName := b.path(entry.Key) - - // New storage entries will have their file names base64 URL encoded. If a - // file with a non-encoded file name exists, it indicates that this is an - // update operation. To avoid duplication of storage entries, delete the - // old entry first. - fullPath := filepath.Join(basePath, "_"+fileName) - if _, err = os.Stat(fullPath); !os.IsNotExist(err) { - err = b.Delete(entry.Key) - if err != nil { - return fmt.Errorf("failed to remove old entry: %v", err) - } - } + basePath, fullPathPrefixedFileName, fullPathPrefixedEncodedFileName := b.path(entry.Key) b.l.Lock() defer b.l.Unlock() - // Make the parent tree - if err = os.MkdirAll(basePath, 0755); err != nil { - return err + // New storage entries will have their file names base64 URL encoded. If a + // file with a non-encoded file name exists, it indicates that this is an + // update operation. To avoid duplication of storage entries, delete the + // old entry in the defer function. + if _, err := os.Stat(fullPathPrefixedFileName); !os.IsNotExist(err) { + defer func() { + err := os.Remove(fullPathPrefixedFileName) + if err != nil && !os.IsNotExist(err) { + retErr = multierror.Append(retErr, fmt.Errorf("failed to remove old entry: %v", err)) + return + } + err = b.cleanupLogicalPath(entry.Key) + if err != nil { + retErr = multierror.Append(retErr, fmt.Errorf("failed to cleanup the after removing old entry: %v", err)) + return + } + }() } - // base64 URL encode the file name to make all the characters compatible by - // the host OS (specially Windows). However, the basePath can contain - // disallowed characters. Encoding all the directory names and the file - // name is an over kill, and encoding the fullPath will flatten the - // storage, which *may* not be desired. - fullPath = filepath.Join(basePath, "_"+base64.URLEncoding.EncodeToString([]byte(fileName))) + // Make the parent tree + if err := os.MkdirAll(basePath, 0755); err != nil { + retErr = multierror.Append(retErr, err) + return retErr + } // JSON encode the entry and write it f, err := os.OpenFile( - fullPath, + fullPathPrefixedEncodedFileName, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0600) if err != nil { - return err + retErr = multierror.Append(retErr, err) + return retErr } defer f.Close() enc := json.NewEncoder(f) - return enc.Encode(entry) + + err = enc.Encode(entry) + if err != nil { + retErr = multierror.Append(retErr, err) + return retErr + } + return nil } func (b *FileBackend) List(prefix string) ([]string, error) { @@ -226,7 +228,21 @@ func (b *FileBackend) List(prefix string) ([]string, error) { return names, nil } -func (b *FileBackend) path(k string) (string, string) { - fullPath := filepath.Join(b.Path, k) - return filepath.Dir(fullPath), filepath.Base(fullPath) +func (b *FileBackend) path(path string) (string, string, string) { + fullPath := filepath.Join(b.Path, path) + + basePath := filepath.Dir(fullPath) + + fileName := filepath.Base(fullPath) + + fullPathPrefixedFileName := filepath.Join(basePath, "_"+fileName) + + // base64 URL encode the file name to make all the characters compatible by + // the host OS (specially Windows). However, the basePath can contain + // disallowed characters. Encoding all the directory names and the file + // name is an over kill, and encoding the fullPath will flatten the + // storage, which *may* not be desired. + fullPathPrefixedEncodedFileName := filepath.Join(basePath, "_"+base64.URLEncoding.EncodeToString([]byte(fileName))) + + return basePath, fullPathPrefixedFileName, fullPathPrefixedEncodedFileName } From 76a456cc97e5526a18663880533e61378c9790eb Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Fri, 13 Jan 2017 03:51:09 -0500 Subject: [PATCH 6/7] file: correct the old entry check --- physical/file.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/physical/file.go b/physical/file.go index 6031b3d16..70c33d935 100644 --- a/physical/file.go +++ b/physical/file.go @@ -145,7 +145,8 @@ func (b *FileBackend) Put(entry *Entry) error { // file with a non-encoded file name exists, it indicates that this is an // update operation. To avoid duplication of storage entries, delete the // old entry in the defer function. - if _, err := os.Stat(fullPathPrefixedFileName); !os.IsNotExist(err) { + info, err := os.Stat(fullPathPrefixedFileName) + if err == nil && info != nil { defer func() { err := os.Remove(fullPathPrefixedFileName) if err != nil && !os.IsNotExist(err) { From 5aeb276018959308a43b36857f252fd146248040 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Fri, 13 Jan 2017 03:58:46 -0500 Subject: [PATCH 7/7] correcting the error statement --- physical/file.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/physical/file.go b/physical/file.go index 70c33d935..b12c34ef8 100644 --- a/physical/file.go +++ b/physical/file.go @@ -59,7 +59,7 @@ func (b *FileBackend) Delete(path string) error { } if err != nil && !os.IsNotExist(err) { - return fmt.Errorf("Failed to remove %q: %v", fullPathPrefixedFileName, err) + return fmt.Errorf("Failed to remove %q: %v", path, err) } err = b.cleanupLogicalPath(path)