Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(secrets): support for csi directories #565

Merged
merged 9 commits into from
Oct 20, 2022
Merged

feat(secrets): support for csi directories #565

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
00a21df
feat(secrets): support for csi directories
Sep 14, 2022
2a54c5b
change file reader logic
Sep 14, 2022
5088f34
nit
Sep 15, 2022
5917497
open logic
Sep 15, 2022
8fa3237
test
Sep 15, 2022
67779a2
fix test
Sep 16, 2022
2b3c346
fix test
Sep 16, 2022
62c8730
Apply suggestions from code review
Tediferous Sep 16, 2022
12f2c35
review tweaks
Sep 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 56 additions & 1 deletion secrets/secrets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
"fmt"
"io"
"io/fs"

"github.com/reddit/baseplate.go/errorsbp"
)
Expand All @@ -28,6 +29,11 @@ func (s Secret) IsEmpty() bool {
return len(s) == 0
}

// CSIFile represent the raw parsed object of a file made by the Vault CSI provider
Tediferous marked this conversation as resolved.
Show resolved Hide resolved
type CSIFile struct {
Secret GenericSecret `json:"data"`
}

// Secrets allows to access secrets based on their different type.
type Secrets struct {
simpleSecrets map[string]SimpleSecret
Expand Down Expand Up @@ -258,16 +264,33 @@ func NewSecrets(r io.Reader) (*Secrets, error) {
return nil, err
}

err = secretsDocument.Validate()
return secretsValidate(secretsDocument)
}

// NewDirSecrets parses a directory and returns its secrets
func NewDirSecrets(dir fs.FS) (*Secrets, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stutters – secrets.NewDirSecrets. Perhaps secrets.FromDir?

secretsDocument := Document{
Secrets: make(map[string]GenericSecret),
}
secretsDocument, err := csiPathParser(dir)
if err != nil {
return nil, err
}
return secretsValidate(secretsDocument)

}

func secretsValidate(secretsDocument Document) (*Secrets, error) {
secrets := &Secrets{
simpleSecrets: make(map[string]SimpleSecret),
versionedSecrets: make(map[string]VersionedSecret),
credentialSecrets: make(map[string]CredentialSecret),
vault: secretsDocument.Vault,
}
err := secretsDocument.Validate()
if err != nil {
Tediferous marked this conversation as resolved.
Show resolved Hide resolved
return nil, err
}
for key, secret := range secretsDocument.Secrets {
switch secret.Type {
case "simple":
Expand Down Expand Up @@ -298,3 +321,35 @@ func NewSecrets(r io.Reader) (*Secrets, error) {
}
return secrets, nil
}

func csiPathParser(dir fs.FS) (Document, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔕

Suggested change
func csiPathParser(dir fs.FS) (Document, error) {
// walkCSIDirectory ...
func walkCSIDirectory(dir fs.FS) (*Document, error) {

secretsDocument := Document{
Secrets: make(map[string]GenericSecret),
}
err := fs.WalkDir(
dir,
".",
func(path string, d fs.DirEntry, err error) error {
if err != nil {
return err
}
if d.IsDir() {
return nil
}
file, err := dir.Open(path)
if err != nil {
return err
}
defer file.Close()

var secretFile CSIFile
err = json.NewDecoder(file).Decode(&secretFile)
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔕 bare returns like this can make errors difficult to trace back if they don't contain sufficient context. I'm not going to audit everything in here, but something to keep in mind for the future. Instead:

a) if the error message contains sufficient context, add a comment to let code reviewers and future readers know that you thought about this:

return err // contains context

or, b) wrap the error with a helpful message:

return fmt.Errorf("decoding %q: %s", path, err)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed but we should use %w for err.

}
secretsDocument.Secrets[path] = secretFile.Secret
return nil
},
)
return secretsDocument, err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you explicitly need the return value in the error case, it's idiomatic to return nil, err in the multi-return error case. The error and the return value are generally considered to be mutually exclusive and this pattern makes the intended use explicit and lowers overall cognitive overhead. (In this case you'd also have to adjust the signature to return a *Document)

}
26 changes: 24 additions & 2 deletions secrets/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package secrets
import (
"context"
"io"
"io/fs"
"os"

"github.com/reddit/baseplate.go/filewatcher"
"github.com/reddit/baseplate.go/log"
Expand Down Expand Up @@ -42,12 +44,21 @@ func NewStore(ctx context.Context, path string, logger log.Wrapper, middlewares
secretHandlerFunc: nopSecretHandlerFunc,
}
store.secretHandler(middlewares...)
fileInfo, err := os.Stat(path)
if err != nil {
return nil, err
}

parser := store.parser
if fileInfo.IsDir() {
parser = filewatcher.WrapDirParser(store.dirParser)
}

result, err := filewatcher.New(
ctx,
filewatcher.Config{
Path: path,
Parser: store.parser,
Parser: parser,
Logger: logger,
},
)
Expand All @@ -59,7 +70,7 @@ func NewStore(ctx context.Context, path string, logger log.Wrapper, middlewares
return store, nil
}

func (s *Store) parser(r io.Reader) (interface{}, error) {
func (s *Store) parser(r io.Reader) (any, error) {
secrets, err := NewSecrets(r)
if err != nil {
return nil, err
Expand All @@ -70,6 +81,17 @@ func (s *Store) parser(r io.Reader) (interface{}, error) {
return secrets, nil
}

func (s *Store) dirParser(dir fs.FS) (any, error) {
secrets, err := NewDirSecrets(dir)
if err != nil {
return nil, err
}

s.secretHandlerFunc(secrets)

return secrets, nil
}

// secretHandler creates the middleware chain.
func (s *Store) secretHandler(middlewares ...SecretMiddleware) {
for _, m := range middlewares {
Expand Down
83 changes: 83 additions & 0 deletions secrets/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,72 @@ const specificationExample = `
}
}`

var externalAccountKey = `
{
"request_id": "1afc3036-2282-d483-c2d4-6d483efdf16c",
"lease_id": "",
"lease_duration": 2764800,
"renewable": false,
"data": {
"type": "versioned",
"current": "YWJjZGVmZ2hpamtsbW5vcHFyc3R1dnd4eXowMTIzNDU=",
"previous": "aHVudGVyMg=="
},
"warnings": null
}
`

var someAPIKey = `
{
"request_id": "1afc3036-2282-d483-c2d4-6d483efdf16c",
"lease_id": "",
"lease_duration": 2764800,
"renewable": false,
"data": {
"type": "simple",
"value": "Y2RvVXhNMVdsTXJma3BDaHRGZ0dPYkVGSg==",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanity check: this isn't a real secret, right? Looks more random than the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont know, I copied this from the other example secret that this test was already using

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a valid secret that's only used in unit tests. @pacejackson might know more of the origin of it 😅

I'm 99% sure it's not something we used in prod.

"encoding": "base64"
},
"warnings": null
}
`
var someDatabaseCredentials = `
{
"request_id": "1afc3036-2282-d483-c2d4-6d483efdf16c",
"lease_id": "",
"lease_duration": 2764800,
"renewable": false,
"data": {
"type": "credential",
"username": "spez",
"password": "hunter2"
},
"warnings": null
}
`

func TestGetSimpleSecret(t *testing.T) {
dir := t.TempDir()
dirCSI := t.TempDir()
tmpFile, err := os.CreateTemp(dir, "secrets.json")
if err != nil {
t.Fatal(err)
}
if err := os.Mkdir(dirCSI+"/secret", 0777); err != nil {
t.Fatal(err)
}
if err := os.Mkdir(dirCSI+"/secret/myservice", 0777); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(dirCSI+"/secret/myservice/external-account-key", []byte(externalAccountKey), 0777); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(dirCSI+"/secret/myservice/some-api-key", []byte(someAPIKey), 0777); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(dirCSI+"/secret/myservice/some-database-credentials", []byte(someDatabaseCredentials), 0777); err != nil {
t.Fatal(err)
}
tmpPath := tmpFile.Name()
tmpFile.Write([]byte(specificationExample))
if err := tmpFile.Close(); err != nil {
Expand Down Expand Up @@ -77,6 +137,29 @@ func TestGetSimpleSecret(t *testing.T) {
}
defer store.Close()

secret, err := store.GetSimpleSecret(tt.key)
if tt.expectedError == nil && err != nil {
t.Fatal(err)
}
if tt.expectedError != nil && err.Error() != tt.expectedError.Error() {
t.Fatalf("expected error %v, actual: %v", tt.expectedError, err)
}
if !reflect.DeepEqual(secret, tt.expected) {
t.Fatalf("expected %+v, actual: %+v", tt.expected, secret)
}
},
)
}
for _, tt := range tests {
t.Run(
tt.name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will run the subtests with the same name as from line 132. which will make the output very confusing.

either construct the name to add -file/-dir suffix (e.g. make line 132 tt.name + "-file" and here tt.name + "-dir", or run another level of subtests with names file/dir.

I would strongly prefer to add another level of subtests. you can also make it another level of for loop to reuse the majority of the code:

	for _, tt := range tests {
		t.Run(
			tt.name,
			func(t *testing.T) {
				for label, path := range map[string]string{
					"file": tmpPath,
					"dir": dirCSI,
				} {
					t.Run(label, func(t *testing.T) {
						store, err := secrets.NewStore(context.Background(), path, log.TestWrapper(t))
						if err != nil {
							t.Fatal(err)
						}
						t.Cleanup(func() { store.Close() })

						secret, err := store.GetSimpleSecret(tt.key)
						if tt.expectedError == nil && err != nil {
							t.Fatal(err)
						}
						if tt.expectedError != nil && err.Error() != tt.expectedError.Error() {
							t.Fatalf("expected error %v, actual: %v", tt.expectedError, err)
						}
						if !reflect.DeepEqual(secret, tt.expected) {
							t.Fatalf("expected %+v, actual: %+v", tt.expected, secret)
						}
					})
				}
			},
		)
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are correct

func(t *testing.T) {
store, err := secrets.NewStore(context.Background(), dirCSI, log.TestWrapper(t))
if err != nil {
t.Fatal(err)
}
defer store.Close()

secret, err := store.GetSimpleSecret(tt.key)
if tt.expectedError == nil && err != nil {
t.Fatal(err)
Expand Down