Skip to content

Commit 627c60e

Browse files
digivavahashiblaum
digivava
authored and
hashiblaum
committed
Get AppRole secret ID from file on disk (#1153)
* Read secret ID from a validated file path instead of directly * Check if secret ID was set * clean up * Generate yaml for secret ID as file path * Less convoluted help text * more clear error message * Make SecretIDPath and SecretRef mutually exclusive * Test for validatePath * Add some extra path validation for Windows * Update manifests * Ability for user to specify their own volumes in VSO * Remove secretidpath-specific integration test in favor of unit test
1 parent 3380993 commit 627c60e

15 files changed

+630
-101
lines changed

‎api/v1beta1/common.go‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@
44
package v1beta1
55

66
import (
7+
"fmt"
8+
"os"
9+
"path/filepath"
10+
"strings"
11+
712
v1 "k8s.io/api/core/v1"
813
)
914

@@ -124,3 +129,46 @@ type VaultClientMeta struct {
124129
// any sensitive information.
125130
ID string `json:"id,omitempty"`
126131
}
132+
133+
// validatePath validates a file path for security. It checks for path traversal
134+
// attempts, ensures the path is absolute, and validates the file exists and is
135+
// not too large. Returns the cleaned path if valid.
136+
func validatePath(path string) (string, error) {
137+
// Prevent path traversal attacks in the original input
138+
if strings.Contains(path, "..") {
139+
return "", fmt.Errorf("invalid path: path traversal detected")
140+
}
141+
142+
cleanPath := filepath.Clean(path)
143+
144+
// Double-check the final normalized path is also safe from path traversal,
145+
// since filepath.Clean uses OS-specific separators and could potentially
146+
// normalize paths differently
147+
if strings.Contains(cleanPath, "..") {
148+
return "", fmt.Errorf("invalid path: path traversal detected")
149+
}
150+
151+
// Ensure the path is absolute to prevent relative path issues
152+
if !filepath.IsAbs(cleanPath) {
153+
return "", fmt.Errorf("invalid path: must be an absolute path")
154+
}
155+
156+
// Validate the file exists and get its info
157+
fileInfo, err := os.Stat(cleanPath)
158+
if err != nil {
159+
return "", fmt.Errorf("failed to access file %s: %w", cleanPath, err)
160+
}
161+
162+
// Ensure it's a regular file, not a directory or special file
163+
if !fileInfo.Mode().IsRegular() {
164+
return "", fmt.Errorf("path must be a regular file, not a directory or special file")
165+
}
166+
167+
// Limit file size to 1MB to prevent resource exhaustion attacks
168+
const maxFileSize = 1024 * 1024
169+
if fileInfo.Size() > maxFileSize {
170+
return "", fmt.Errorf("file too large: %d bytes (max: %d)", fileInfo.Size(), maxFileSize)
171+
}
172+
173+
return cleanPath, nil
174+
}

‎api/v1beta1/common_test.go‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
// Copyright (c) HashiCorp, Inc.
2+
// SPDX-License-Identifier: BUSL-1.1
3+
4+
package v1beta1
5+
6+
import (
7+
"os"
8+
"path/filepath"
9+
"testing"
10+
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
)
14+
15+
func Test_validatePath(t *testing.T) {
16+
tmpDir := t.TempDir()
17+
validFile := filepath.Join(tmpDir, "valid-file")
18+
require.NoError(t, os.WriteFile(validFile, []byte("content"), 0o600))
19+
20+
// Create a file that's too large (> 1MB)
21+
largeFile := filepath.Join(tmpDir, "large-file")
22+
largeContent := make([]byte, 1024*1024+1) // 1MB + 1 byte
23+
require.NoError(t, os.WriteFile(largeFile, largeContent, 0o600))
24+
25+
tests := []struct {
26+
name string
27+
path string
28+
want string
29+
wantError bool
30+
errorMsg string
31+
}{
32+
{
33+
name: "valid-absolute-path",
34+
path: validFile,
35+
want: validFile,
36+
wantError: false,
37+
},
38+
{
39+
name: "valid-path-with-redundant-separators",
40+
path: validFile + "//",
41+
want: validFile,
42+
wantError: false,
43+
},
44+
{
45+
name: "invalid-relative-path",
46+
path: "relative/path",
47+
wantError: true,
48+
errorMsg: "must be an absolute path",
49+
},
50+
{
51+
name: "invalid-path-with-dotdot-in-middle",
52+
path: tmpDir + "/subdir/../../etc/passwd",
53+
wantError: true,
54+
errorMsg: "path traversal detected",
55+
},
56+
{
57+
name: "invalid-literal-dotdot-in-path",
58+
path: "/tmp/test/../sensitive",
59+
wantError: true,
60+
errorMsg: "path traversal detected",
61+
},
62+
{
63+
name: "invalid-file-does-not-exist",
64+
path: "/nonexistent/file",
65+
wantError: true,
66+
errorMsg: "failed to access file",
67+
},
68+
{
69+
name: "invalid-file-too-large",
70+
path: largeFile,
71+
wantError: true,
72+
errorMsg: "file too large",
73+
},
74+
{
75+
name: "invalid-path-is-directory",
76+
path: tmpDir,
77+
wantError: true,
78+
errorMsg: "must be a regular file",
79+
},
80+
}
81+
82+
for _, tt := range tests {
83+
t.Run(tt.name, func(t *testing.T) {
84+
got, err := validatePath(tt.path)
85+
if tt.wantError {
86+
assert.Error(t, err)
87+
if tt.errorMsg != "" {
88+
assert.Contains(t, err.Error(), tt.errorMsg)
89+
}
90+
} else {
91+
require.NoError(t, err)
92+
assert.Equal(t, tt.want, got)
93+
}
94+
})
95+
}
96+
}

‎api/v1beta1/vaultauth_types.go‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -127,13 +127,15 @@ type VaultAuthConfigAppRole struct {
127127
// RoleID of the AppRole Role to use for authenticating to Vault.
128128
RoleID string `json:"roleId,omitempty"`
129129

130-
// SecretID of the AppRole Role to use for authenticating to Vault.
131-
// If both SecretID and SecretRef are specified, SecretID takes precedence.
132-
SecretID string `json:"secretID,omitempty"`
130+
// SecretIDPath is a file system path pointing to a file containing the plaintext Secret ID for the
131+
// AppRole Role to use for authenticating to Vault.
132+
// SecretIDPath and SecretRef are mutually exclusive, and only one should be specified.
133+
SecretIDPath string `json:"secretIDPath,omitempty"`
133134

134135
// SecretRef is the name of a Kubernetes secret in the consumer's (VDS/VSS/PKI) namespace which
135136
// provides the AppRole Role's SecretID. The secret must have a key named `id` which holds the
136137
// AppRole Role's secretID.
138+
// SecretIDPath and SecretRef are mutually exclusive, and only one should be specified.
137139
SecretRef string `json:"secretRef,omitempty"`
138140
}
139141

@@ -145,6 +147,9 @@ func (a *VaultAuthConfigAppRole) Merge(other *VaultAuthConfigAppRole) (*VaultAut
145147
if c.RoleID == "" {
146148
c.RoleID = other.RoleID
147149
}
150+
if c.SecretIDPath == "" {
151+
c.SecretIDPath = other.SecretIDPath
152+
}
148153
if c.SecretRef == "" {
149154
c.SecretRef = other.SecretRef
150155
}
@@ -163,8 +168,20 @@ func (a *VaultAuthConfigAppRole) Validate() error {
163168
errs = errors.Join(fmt.Errorf("empty roleID"))
164169
}
165170

166-
if a.SecretRef == "" && a.SecretID == "" {
167-
errs = errors.Join(fmt.Errorf("empty secretRef and seecretID"))
171+
if a.SecretRef == "" && a.SecretIDPath == "" {
172+
errs = errors.Join(errs, fmt.Errorf("either secretRef or secretIDPath must be specified"))
173+
}
174+
175+
if a.SecretRef != "" && a.SecretIDPath != "" {
176+
errs = errors.Join(errs, fmt.Errorf("secretRef and secretIDPath are mutually exclusive, only one should be specified"))
177+
}
178+
179+
if a.SecretIDPath != "" {
180+
safePath, err := validatePath(a.SecretIDPath)
181+
if err != nil {
182+
return fmt.Errorf("invalid SecretIDPath: %w", err)
183+
}
184+
a.SecretIDPath = safePath
168185
}
169186

170187
return errs

‎api/v1beta1/vaultauth_types_test.go‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
// Copyright (c) HashiCorp, Inc.
2+
// SPDX-License-Identifier: BUSL-1.1
3+
4+
package v1beta1
5+
6+
import (
7+
"os"
8+
"path/filepath"
9+
"testing"
10+
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
)
14+
15+
func TestVaultAuthConfigAppRole_Validate(t *testing.T) {
16+
tmpDir := t.TempDir()
17+
validFilePath := filepath.Join(tmpDir, "test-secretid")
18+
require.NoError(t, os.WriteFile(validFilePath, []byte("test-secret-id"), 0o600))
19+
20+
tests := []struct {
21+
name string
22+
appRole *VaultAuthConfigAppRole
23+
wantError bool
24+
errorMsg string
25+
}{
26+
{
27+
name: "valid-with-secretref",
28+
appRole: &VaultAuthConfigAppRole{
29+
RoleID: "test-role",
30+
SecretRef: "test-secret",
31+
},
32+
wantError: false,
33+
},
34+
{
35+
name: "valid-with-secretidpath",
36+
appRole: &VaultAuthConfigAppRole{
37+
RoleID: "test-role",
38+
SecretIDPath: validFilePath,
39+
},
40+
wantError: false,
41+
},
42+
{
43+
name: "invalid-both-secretref-and-secretidpath",
44+
appRole: &VaultAuthConfigAppRole{
45+
RoleID: "test-role",
46+
SecretRef: "test-secret",
47+
SecretIDPath: validFilePath,
48+
},
49+
wantError: true,
50+
errorMsg: "mutually exclusive",
51+
},
52+
{
53+
name: "invalid-missing-both",
54+
appRole: &VaultAuthConfigAppRole{
55+
RoleID: "test-role",
56+
},
57+
wantError: true,
58+
errorMsg: "either secretRef or secretIDPath must be specified",
59+
},
60+
{
61+
name: "invalid-missing-roleid",
62+
appRole: &VaultAuthConfigAppRole{
63+
SecretRef: "test-secret",
64+
},
65+
wantError: true,
66+
errorMsg: "empty roleID",
67+
},
68+
}
69+
70+
for _, tt := range tests {
71+
t.Run(tt.name, func(t *testing.T) {
72+
err := tt.appRole.Validate()
73+
if tt.wantError {
74+
assert.Error(t, err)
75+
if tt.errorMsg != "" {
76+
assert.Contains(t, err.Error(), tt.errorMsg)
77+
}
78+
} else {
79+
assert.NoError(t, err)
80+
}
81+
})
82+
}
83+
}
84+
85+
func TestVaultAuthConfigAppRole_Merge(t *testing.T) {
86+
tests := []struct {
87+
name string
88+
base *VaultAuthConfigAppRole
89+
other *VaultAuthConfigAppRole
90+
want *VaultAuthConfigAppRole
91+
wantError bool
92+
}{
93+
{
94+
name: "merge-empty-fields",
95+
base: &VaultAuthConfigAppRole{
96+
RoleID: "base-role",
97+
},
98+
other: &VaultAuthConfigAppRole{
99+
RoleID: "other-role",
100+
SecretRef: "other-secret",
101+
},
102+
want: &VaultAuthConfigAppRole{
103+
RoleID: "base-role",
104+
SecretRef: "other-secret",
105+
},
106+
wantError: false,
107+
},
108+
{
109+
name: "merge-preserves-base-values",
110+
base: &VaultAuthConfigAppRole{
111+
RoleID: "base-role",
112+
SecretRef: "base-secret",
113+
},
114+
other: &VaultAuthConfigAppRole{
115+
RoleID: "other-role",
116+
SecretRef: "other-secret",
117+
},
118+
want: &VaultAuthConfigAppRole{
119+
RoleID: "base-role",
120+
SecretRef: "base-secret",
121+
},
122+
wantError: false,
123+
},
124+
{
125+
name: "merge-creates-invalid-config",
126+
base: &VaultAuthConfigAppRole{
127+
RoleID: "base-role",
128+
},
129+
other: &VaultAuthConfigAppRole{
130+
RoleID: "other-role",
131+
},
132+
wantError: true,
133+
},
134+
}
135+
136+
for _, tt := range tests {
137+
t.Run(tt.name, func(t *testing.T) {
138+
got, err := tt.base.Merge(tt.other)
139+
if tt.wantError {
140+
assert.Error(t, err)
141+
} else {
142+
require.NoError(t, err)
143+
assert.Equal(t, tt.want.RoleID, got.RoleID)
144+
assert.Equal(t, tt.want.SecretRef, got.SecretRef)
145+
assert.Equal(t, tt.want.SecretIDPath, got.SecretIDPath)
146+
}
147+
})
148+
}
149+
}

‎chart/crds/secrets.hashicorp.com_vaultauthglobals.yaml‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,16 +79,18 @@ spec:
7979
description: RoleID of the AppRole Role to use for authenticating
8080
to Vault.
8181
type: string
82-
secretID:
82+
secretIDPath:
8383
description: |-
84-
SecretID of the AppRole Role to use for authenticating to Vault.
85-
If both SecretID and SecretRef are specified, SecretID takes precedence.
84+
SecretIDPath is a file system path pointing to a file containing the plaintext Secret ID for the
85+
AppRole Role to use for authenticating to Vault.
86+
SecretIDPath and SecretRef are mutually exclusive, and only one should be specified.
8687
type: string
8788
secretRef:
8889
description: |-
8990
SecretRef is the name of a Kubernetes secret in the consumer's (VDS/VSS/PKI) namespace which
9091
provides the AppRole Role's SecretID. The secret must have a key named `id` which holds the
9192
AppRole Role's secretID.
93+
SecretIDPath and SecretRef are mutually exclusive, and only one should be specified.
9294
type: string
9395
type: object
9496
aws:

0 commit comments

Comments
 (0)