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

Fixes Namespace Concat Bug for KV2 secrets #1921

Merged
merged 12 commits into from
May 6, 2024
Merged

Fixes Namespace Concat Bug for KV2 secrets #1921

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1ab68af
Modified dependency
divyaac May 3, 2024
17c9ada
Edited comment
divyaac May 3, 2024
4a6cb66
Edited comment
divyaac May 3, 2024
78eebe3
Modified method
divyaac May 6, 2024
886714f
Added unit tests
divyaac May 6, 2024
ba7eb94
Removed logging
divyaac May 6, 2024
6cbdad3
Edited test reversed mount path and path
divyaac May 6, 2024
3f8a619
Edited test to contain final slash
divyaac May 6, 2024
1460751
Added one more test
divyaac May 6, 2024
7cae3c3
Edited test to have final slash
divyaac May 6, 2024
b7cc85d
Edited test
divyaac May 6, 2024
070ef51
Edited test to canonicalize
divyaac May 6, 2024
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
20 changes: 16 additions & 4 deletions dependency/vault_read.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func (d *VaultReadQuery) readSecret(clients *ClientSet) (*api.Secret, error) {
isKVv2 = false
d.secretPath = d.rawPath
} else if isKVv2 {
d.secretPath = shimKVv2Path(d.rawPath, mountPath)
d.secretPath = shimKVv2Path(d.rawPath, mountPath, clients.Vault().Namespace())
} else {
d.secretPath = d.rawPath
}
Expand Down Expand Up @@ -196,18 +196,30 @@ func deletedKVv2(s *api.Secret) bool {

// shimKVv2Path aligns the supported legacy path to KV v2 specs by inserting
// /data/ into the path for reading secrets. Paths for metadata are not modified.
func shimKVv2Path(rawPath, mountPath string) string {
func shimKVv2Path(rawPath, mountPath, clientNamespace string) string {
switch {
case rawPath == mountPath, rawPath == strings.TrimSuffix(mountPath, "/"):
return path.Join(mountPath, "data")
default:
p := strings.TrimPrefix(rawPath, mountPath)

// Canonicalize the client namespace path to always having a '/' suffix
if !strings.HasSuffix(clientNamespace, "/") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope for this PR, but maybe we should expose namespace.Canonicalize in our SDK for this kind of thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can probably canonicalize it in Agent when we set the template config value

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, though I like keeping it in consul-template too for anything else that happens to be using it. Like I said, out of scope for this PR, but it seems like we have a need for external repos to be able to use the Canonicalize logic without repeating it. Food for thought!

clientNamespace += "/"
}
// Extract client namespace from mount path if it exists
divyaac marked this conversation as resolved.
Show resolved Hide resolved
rawPathNsAndMountPath := strings.TrimPrefix(mountPath, clientNamespace)
divyaac marked this conversation as resolved.
Show resolved Hide resolved

// Trim (mount path - client namespace) from the raw path
p := strings.TrimPrefix(rawPath, rawPathNsAndMountPath)

log.Printf("[ERR] divya modified raw path: %s", p)
divyaac marked this conversation as resolved.
Show resolved Hide resolved

// Only add /data/ prefix to the path if neither /data/ or /metadata/ are
// present.
if strings.HasPrefix(p, "data/") || strings.HasPrefix(p, "metadata/") {
return rawPath
}
return path.Join(mountPath, "data", p)

return path.Join(rawPathNsAndMountPath, "data", p)
}
}
2 changes: 1 addition & 1 deletion dependency/vault_read_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,7 @@ func TestShimKVv2Path(t *testing.T) {
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
actual := shimKVv2Path(tc.path, tc.mountPath)
actual := shimKVv2Path(tc.path, tc.mountPath, "")
assert.Equal(t, tc.expected, actual)
})
}
Expand Down
2 changes: 1 addition & 1 deletion dependency/vault_write.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ func (d *VaultWriteQuery) writeSecret(clients *ClientSet, opts *QueryOptions) (*
data := d.data
mountPath, isv2, _ := isKVv2(clients.Vault(), path)
if isv2 {
path = shimKVv2Path(path, mountPath)
path = shimKVv2Path(path, mountPath, clients.Vault().Namespace())
data = map[string]interface{}{"data": d.data}
}

Expand Down
Loading