Skip to content

Commit 32ec3b3

Browse files
committed
Fixes after security validation
1 parent 210ceb2 commit 32ec3b3

4 files changed

+166
-17
lines changed

os_specific_posix.go

+21
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
//go:build darwin || linux
2+
3+
package gosnowflake
4+
5+
import (
6+
"fmt"
7+
"os"
8+
"syscall"
9+
)
10+
11+
func provideFileOwner(filepath string) (uint32, error) {
12+
info, err := os.Stat(filepath)
13+
if err != nil {
14+
return 0, err
15+
}
16+
nativeStat, ok := info.Sys().(*syscall.Stat_t)
17+
if !ok {
18+
return 0, fmt.Errorf("cannot cast file info for %v to *syscall.Stat_t", filepath)
19+
}
20+
return nativeStat.Uid, nil
21+
}

os_specific_windows.go

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// go:build windows
2+
3+
package gosnowflake
4+
5+
import "errors"
6+
7+
func provideFileOwner(filepath string) (uint32, error) {
8+
return 0, errors.New("provideFileOwner is unsupported on windows")
9+
}

secure_storage_manager.go

+105-16
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,12 @@ import (
1010
"fmt"
1111
"github.com/99designs/keyring"
1212
"os"
13+
"os/user"
1314
"path/filepath"
1415
"runtime"
16+
"strconv"
1517
"strings"
18+
"sync"
1619
"time"
1720
)
1821

@@ -44,7 +47,7 @@ type secureTokenSpec struct {
4447
tokenType tokenType
4548
}
4649

47-
func (t *secureTokenSpec) buildKey() string {
50+
func (t *secureTokenSpec) buildKey() (string, error) {
4851
return buildCredentialsKey(t.host, t.user, t.tokenType)
4952
}
5053

@@ -80,9 +83,9 @@ func newSecureStorageManager() secureStorageManager {
8083
logger.Debugf("failed to create credentials cache dir. %v", err)
8184
return newNoopSecureStorageManager()
8285
}
83-
return ssm
86+
return &threadSafeSecureStorageManager{&sync.Mutex{}, ssm}
8487
case "darwin", "windows":
85-
return newKeyringBasedSecureStorageManager()
88+
return &threadSafeSecureStorageManager{&sync.Mutex{}, newKeyringBasedSecureStorageManager()}
8689
default:
8790
logger.Infof("OS %v does not support credentials cache", runtime.GOOS)
8891
return newNoopSecureStorageManager()
@@ -171,8 +174,12 @@ func (ssm *fileBasedSecureStorageManager) getTokens(data map[string]any) map[str
171174
}
172175

173176
func (ssm *fileBasedSecureStorageManager) setCredential(tokenSpec *secureTokenSpec, value string) {
174-
credentialsKey := tokenSpec.buildKey()
175-
err := ssm.lockFile()
177+
credentialsKey, err := tokenSpec.buildKey()
178+
if err != nil {
179+
logger.Warn(err)
180+
return
181+
}
182+
err = ssm.lockFile()
176183
if err != nil {
177184
logger.Warnf("Set credential failed. Unable to lock cache. %v", err)
178185
return
@@ -241,8 +248,12 @@ func (ssm *fileBasedSecureStorageManager) unlockFile() {
241248
}
242249

243250
func (ssm *fileBasedSecureStorageManager) getCredential(tokenSpec *secureTokenSpec) string {
244-
credentialsKey := tokenSpec.buildKey()
245-
err := ssm.lockFile()
251+
credentialsKey, err := tokenSpec.buildKey()
252+
if err != nil {
253+
logger.Warn(err)
254+
return ""
255+
}
256+
err = ssm.lockFile()
246257
if err != nil {
247258
logger.Warn("Failed to lock credential cache file.")
248259
return ""
@@ -294,12 +305,37 @@ func (ssm *fileBasedSecureStorageManager) ensurePermissions() error {
294305
return nil
295306
}
296307

297-
func (ssm *fileBasedSecureStorageManager) readTemporaryCacheFile() map[string]any {
298-
err := ssm.ensurePermissions()
308+
func (ssm *fileBasedSecureStorageManager) ensureOwner(filePath string) error {
309+
currentUser, err := user.Current()
299310
if err != nil {
311+
return err
312+
}
313+
dirOwnerUid, err := provideFileOwner(filePath)
314+
if err != nil && !errors.Is(err, os.ErrNotExist) {
315+
return err
316+
}
317+
if errors.Is(err, os.ErrNotExist) {
318+
return nil
319+
}
320+
if strconv.Itoa(int(dirOwnerUid)) != currentUser.Uid {
321+
return errors.New("incorrect owner of " + ssm.credDirPath)
322+
}
323+
return nil
324+
}
325+
326+
func (ssm *fileBasedSecureStorageManager) readTemporaryCacheFile() map[string]any {
327+
if err := ssm.ensurePermissions(); err != nil {
300328
logger.Warnf("Failed to ensure permission for temporary cache file. %v.\n", err)
301329
return map[string]any{}
302330
}
331+
if err := ssm.ensureOwner(ssm.credDirPath); err != nil {
332+
logger.Warn("Failed to ensure owner for %v. %v", ssm.credDirPath, err)
333+
return map[string]any{}
334+
}
335+
if err := ssm.ensureOwner(ssm.credFilePath()); err != nil {
336+
logger.Warn("Failed to ensure owner for %v. %v", ssm.credFilePath(), err)
337+
return map[string]any{}
338+
}
303339

304340
jsonData, err := os.ReadFile(ssm.credFilePath())
305341
if err != nil {
@@ -317,8 +353,12 @@ func (ssm *fileBasedSecureStorageManager) readTemporaryCacheFile() map[string]an
317353
}
318354

319355
func (ssm *fileBasedSecureStorageManager) deleteCredential(tokenSpec *secureTokenSpec) {
320-
credentialsKey := tokenSpec.buildKey()
321-
err := ssm.lockFile()
356+
credentialsKey, err := tokenSpec.buildKey()
357+
if err != nil {
358+
logger.Warn(err)
359+
return
360+
}
361+
err = ssm.lockFile()
322362
if err != nil {
323363
logger.Warnf("Set credential failed. Unable to lock cache. %v", err)
324364
return
@@ -335,6 +375,14 @@ func (ssm *fileBasedSecureStorageManager) deleteCredential(tokenSpec *secureToke
335375
}
336376

337377
func (ssm *fileBasedSecureStorageManager) writeTemporaryCacheFile(cache map[string]any) error {
378+
if err := ssm.ensureOwner(ssm.credDirPath); err != nil {
379+
return err
380+
}
381+
if err := ssm.ensureOwner(ssm.credFilePath()); err != nil {
382+
logger.Warn("Failed to ensure owner for %v. %v", ssm.credFilePath(), err)
383+
return err
384+
}
385+
338386
bytes, err := json.Marshal(cache)
339387
if err != nil {
340388
return fmt.Errorf("failed to marshal credential cache map. %w", err)
@@ -370,7 +418,11 @@ func (ssm *keyringSecureStorageManager) setCredential(tokenSpec *secureTokenSpec
370418
if value == "" {
371419
logger.Debug("no token provided")
372420
} else {
373-
credentialsKey := tokenSpec.buildKey()
421+
credentialsKey, err := tokenSpec.buildKey()
422+
if err != nil {
423+
logger.Warn(err)
424+
return
425+
}
374426
if runtime.GOOS == "windows" {
375427
ring, _ := keyring.Open(keyring.Config{
376428
WinCredPrefix: strings.ToUpper(tokenSpec.host),
@@ -401,7 +453,11 @@ func (ssm *keyringSecureStorageManager) setCredential(tokenSpec *secureTokenSpec
401453

402454
func (ssm *keyringSecureStorageManager) getCredential(tokenSpec *secureTokenSpec) string {
403455
cred := ""
404-
credentialsKey := tokenSpec.buildKey()
456+
credentialsKey, err := tokenSpec.buildKey()
457+
if err != nil {
458+
logger.Warn(err)
459+
return ""
460+
}
405461
if runtime.GOOS == "windows" {
406462
ring, _ := keyring.Open(keyring.Config{
407463
WinCredPrefix: strings.ToUpper(tokenSpec.host),
@@ -432,7 +488,11 @@ func (ssm *keyringSecureStorageManager) getCredential(tokenSpec *secureTokenSpec
432488
}
433489

434490
func (ssm *keyringSecureStorageManager) deleteCredential(tokenSpec *secureTokenSpec) {
435-
credentialsKey := tokenSpec.buildKey()
491+
credentialsKey, err := tokenSpec.buildKey()
492+
if err != nil {
493+
logger.Warn(err)
494+
return
495+
}
436496
if runtime.GOOS == "windows" {
437497
ring, _ := keyring.Open(keyring.Config{
438498
WinCredPrefix: strings.ToUpper(tokenSpec.host),
@@ -454,11 +514,17 @@ func (ssm *keyringSecureStorageManager) deleteCredential(tokenSpec *secureTokenS
454514
}
455515
}
456516

457-
func buildCredentialsKey(host, user string, credType tokenType) string {
517+
func buildCredentialsKey(host, user string, credType tokenType) (string, error) {
518+
if host == "" {
519+
return "", errors.New("host is not provided to store in token cache, skipping")
520+
}
521+
if user == "" {
522+
return "", errors.New("user is not provided to store in token cache, skipping")
523+
}
458524
plainCredKey := host + ":" + user + ":" + string(credType)
459525
checksum := sha256.New()
460526
checksum.Write([]byte(plainCredKey))
461-
return hex.EncodeToString(checksum.Sum(nil))
527+
return hex.EncodeToString(checksum.Sum(nil)), nil
462528
}
463529

464530
type noopSecureStorageManager struct {
@@ -477,3 +543,26 @@ func (ssm *noopSecureStorageManager) getCredential(_ *secureTokenSpec) string {
477543

478544
func (ssm *noopSecureStorageManager) deleteCredential(_ *secureTokenSpec) {
479545
}
546+
547+
type threadSafeSecureStorageManager struct {
548+
mu *sync.Mutex
549+
delegate secureStorageManager
550+
}
551+
552+
func (ssm *threadSafeSecureStorageManager) setCredential(tokenSpec *secureTokenSpec, value string) {
553+
ssm.mu.Lock()
554+
defer ssm.mu.Unlock()
555+
ssm.delegate.setCredential(tokenSpec, value)
556+
}
557+
558+
func (ssm *threadSafeSecureStorageManager) getCredential(tokenSpec *secureTokenSpec) string {
559+
ssm.mu.Lock()
560+
defer ssm.mu.Unlock()
561+
return ssm.delegate.getCredential(tokenSpec)
562+
}
563+
564+
func (ssm *threadSafeSecureStorageManager) deleteCredential(tokenSpec *secureTokenSpec) {
565+
ssm.mu.Lock()
566+
defer ssm.mu.Unlock()
567+
ssm.delegate.deleteCredential(tokenSpec)
568+
}

secure_storage_manager_test.go

+31-1
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) {
6767
assertNilF(t, err)
6868

6969
t.Run("store single token", func(t *testing.T) {
70+
logger.SetLogLevel("debug")
7071
tokenSpec := newMfaTokenSpec("host.com", "johndoe")
7172
cred := "token123"
7273
ssm.setCredential(tokenSpec, cred)
@@ -152,6 +153,34 @@ func TestSetAndGetCredentialMfa(t *testing.T) {
152153
}
153154
}
154155

156+
func TestSkipStoringCredentialIfUserIsEmpty(t *testing.T) {
157+
tokenSpecs := []*secureTokenSpec{
158+
newMfaTokenSpec("mfaHost.com", ""),
159+
newIDTokenSpec("idHost.com", ""),
160+
}
161+
162+
for _, tokenSpec := range tokenSpecs {
163+
t.Run(tokenSpec.host, func(t *testing.T) {
164+
credentialsStorage.setCredential(tokenSpec, "non-empty-value")
165+
assertEqualE(t, credentialsStorage.getCredential(tokenSpec), "")
166+
})
167+
}
168+
}
169+
170+
func TestSkipStoringCredentialIfHostIsEmpty(t *testing.T) {
171+
tokenSpecs := []*secureTokenSpec{
172+
newMfaTokenSpec("", "mfaUser"),
173+
newIDTokenSpec("", "idUser"),
174+
}
175+
176+
for _, tokenSpec := range tokenSpecs {
177+
t.Run(tokenSpec.user, func(t *testing.T) {
178+
credentialsStorage.setCredential(tokenSpec, "non-empty-value")
179+
assertEqualE(t, credentialsStorage.getCredential(tokenSpec), "")
180+
})
181+
}
182+
}
183+
155184
func TestStoreTemporaryCredental(t *testing.T) {
156185
if runningOnGithubAction() {
157186
t.Skip("cannot write to github file system")
@@ -189,7 +218,8 @@ func TestBuildCredentialsKey(t *testing.T) {
189218
{"testaccount.snowflakecomputing.com", "testuser", "IdToken", "5014e26489992b6ea56b50e936ba85764dc51338f60441bdd4a69eac7e15bada"}, // pragma: allowlist secret
190219
}
191220
for _, test := range testcases {
192-
target := buildCredentialsKey(test.host, test.user, test.credType)
221+
target, err := buildCredentialsKey(test.host, test.user, test.credType)
222+
assertNilF(t, err)
193223
if target != test.out {
194224
t.Fatalf("failed to convert target. expected: %v, but got: %v", test.out, target)
195225
}

0 commit comments

Comments
 (0)