Skip to content

Commit 376fc16

Browse files
lexbritvinlexbritvin
committed
Fix after review.
1 parent cef49a1 commit 376fc16

File tree

6 files changed

+133
-131
lines changed

6 files changed

+133
-131
lines changed

‎file.go‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package keyring
22

33
import (
4+
"errors"
45
"io"
6+
"io/fs"
57
"os"
68
"path/filepath"
79
"strings"
@@ -15,17 +17,22 @@ type CredentialsFile interface {
1517
io.ReadWriteCloser
1618
// Open opens a file in FS with flag open options and perm for file permissions if the file is new.
1719
// See os.OpenFile for more info about flag and perm arguments.
18-
Open(flag int, perm os.FileMode) error
20+
Open(flag int, perm fs.FileMode) error
1921
// Unlock decrypts a file if supported.
2022
Unlock(askNew bool) error
2123
// Lock makes it to request Unlock again.
2224
Lock()
2325
// Remove deletes a file from FS.
2426
Remove() error
27+
// Stat returns a [FileInfo] describing the named file.
28+
// If there is an error, it will be of type [*PathError].
29+
// See os.Stat().
30+
Stat() (fs.FileInfo, error)
2531
}
2632

2733
type nullFile struct{}
2834

35+
func (nullFile) Stat() (fs.FileInfo, error) { return nil, fs.ErrNotExist }
2936
func (nullFile) Open(_ int, _ os.FileMode) (err error) { return nil }
3037
func (nullFile) Unlock(_ bool) error { return nil }
3138
func (nullFile) Lock() {}
@@ -42,11 +49,11 @@ type plainFile struct {
4249
// NewPlainFile creates a CredentialsFile to open a plain file.
4350
func NewPlainFile(fname string) CredentialsFile {
4451
return &plainFile{
45-
fname: fname + ".age",
52+
fname: fname,
4653
}
4754
}
4855

49-
func (f *plainFile) Open(flag int, perm os.FileMode) (err error) {
56+
func (f *plainFile) Open(flag int, perm fs.FileMode) (err error) {
5057
isCreate := flag&os.O_CREATE == os.O_CREATE
5158
if isCreate {
5259
err = launchr.EnsurePath(filepath.Dir(f.fname))
@@ -63,6 +70,7 @@ func (f *plainFile) Open(flag int, perm os.FileMode) (err error) {
6370
return nil
6471
}
6572

73+
func (f *plainFile) Stat() (fs.FileInfo, error) { return os.Stat(f.fname) }
6674
func (f *plainFile) Unlock(bool) (err error) { return nil }
6775
func (f *plainFile) Lock() {}
6876
func (f *plainFile) Read(p []byte) (n int, err error) { return f.file.Read(p) }
@@ -77,7 +85,7 @@ func (f *plainFile) Remove() (err error) {
7785
}
7886

7987
type ageFile struct {
80-
file *plainFile
88+
*plainFile
8189
askPass AskPass
8290
passphrase string // @todo make sure it's compatible with ACL in the future
8391

@@ -88,16 +96,12 @@ type ageFile struct {
8896
// NewAgeFile creates a CredentialsFile to open a file encrypted with age.
8997
func NewAgeFile(fname string, askPass AskPass) CredentialsFile {
9098
return &ageFile{
91-
file: &plainFile{
92-
fname: fname + ".age",
93-
},
94-
askPass: askPass,
99+
plainFile: NewPlainFile(fname).(*plainFile),
100+
askPass: askPass,
95101
}
96102
}
97103

98-
func (f *ageFile) Open(flag int, perm os.FileMode) (err error) { return f.file.Open(flag, perm) }
99-
func (f *ageFile) Remove() error { return f.file.Remove() }
100-
func (f *ageFile) Lock() { f.passphrase = "" }
104+
func (f *ageFile) Lock() { f.passphrase = "" }
101105

102106
func (f *ageFile) Unlock(askNew bool) (err error) {
103107
if f.passphrase != "" {
@@ -129,6 +133,8 @@ func (f *ageFile) Read(p []byte) (n int, err error) {
129133
// The file is malformed, not age encrypted and can't be read.
130134
if strings.Contains(err.Error(), "parsing age header:") {
131135
return 0, ErrKeyringMalformed
136+
} else if strings.Contains(err.Error(), "no identity matched any of the recipients") {
137+
return 0, ErrIncorrectPass
132138
}
133139
return 0, err
134140
}
@@ -152,8 +158,11 @@ func (f *ageFile) Write(p []byte) (n int, err error) {
152158
}
153159

154160
func (f *ageFile) Close() error {
161+
var err error
155162
if f.w != nil {
156-
_ = f.w.Close()
163+
err = f.w.Close()
157164
}
158-
return f.file.Close()
165+
f.w = nil
166+
f.r = nil
167+
return errors.Join(err, f.file.Close())
159168
}

‎keyring.go‎

Lines changed: 18 additions & 108 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,11 @@ const defaultFileYaml = "keyring.yaml"
1111

1212
// Keyring errors.
1313
var (
14-
ErrNotFound = errors.New("item not found") // ErrNotFound if an item was not found
15-
ErrEmptyFields = errors.New("item can't be empty") // ErrEmptyFields if fields are empty
16-
ErrEmptyPass = errors.New("passphrase can't be empty") // ErrEmptyPass if a passphrase is empty
17-
ErrKeyringMalformed = errors.New("the keyring is malformed") // ErrKeyringMalformed when keyring can't be read.
14+
ErrNotFound = errors.New("item not found") // ErrNotFound if an item was not found
15+
ErrEmptyFields = errors.New("item can't be empty") // ErrEmptyFields if fields are empty
16+
ErrEmptyPass = errors.New("passphrase can't be empty") // ErrEmptyPass if a passphrase is empty
17+
ErrKeyringMalformed = errors.New("the keyring is malformed") // ErrKeyringMalformed when keyring can't be read.
18+
ErrIncorrectPass = errors.New("the given passphrase is incorrect") // ErrIncorrectPass if a passphrase is incorrect
1819
)
1920

2021
// SecretItem is an interface that represents an item saved in a storage.
@@ -102,19 +103,22 @@ type DataStore interface {
102103
Destroy() error
103104
}
104105

106+
// dataStore is a type alias to embed it as a private property.
107+
type dataStore = DataStore
108+
105109
// Keyring is a [launchr.Service] providing password store functionality.
106110
type Keyring = *keyringService
107111

108112
type keyringService struct {
109-
store DataStore
110-
mask *launchr.SensitiveMask
113+
dataStore
114+
mask *launchr.SensitiveMask
111115
}
112116

113117
// NewService creates a new Keyring service.
114118
func NewService(store DataStore, mask *launchr.SensitiveMask) Keyring {
115119
return &keyringService{
116-
store: store,
117-
mask: mask,
120+
dataStore: store,
121+
mask: mask,
118122
}
119123
}
120124

@@ -142,7 +146,7 @@ func (k *keyringService) ServiceCreate(svc *launchr.ServiceManager) launchr.Serv
142146
// TODO: do not encrypt if the passphrase is not provided.
143147
store := NewFileStore(
144148
NewAgeFile(
145-
cfg.Path(defaultFileYaml),
149+
cfg.Path(defaultFileYaml+".age"),
146150
AskPassFirstAvailable{
147151
AskPassConst(passphrase.get),
148152
AskPassWithTerminal{},
@@ -153,42 +157,9 @@ func (k *keyringService) ServiceCreate(svc *launchr.ServiceManager) launchr.Serv
153157
return NewService(store, mask)
154158
}
155159

156-
// ResetStorage cleans store for subsequent reload.
157-
func (k *keyringService) ResetStorage() {
158-
k.store = nil
159-
}
160-
161-
func (k *keyringService) defaultStore() (DataStore, error) {
162-
return k.store, nil
163-
}
164-
165-
// GetUrls implements DataStore interface. Uses service default store.
166-
func (k *keyringService) GetUrls() ([]string, error) {
167-
s, err := k.defaultStore()
168-
if err != nil {
169-
return []string{}, err
170-
}
171-
172-
return s.GetUrls()
173-
}
174-
175-
// GetKeys implements DataStore interface. Uses service default store.
176-
func (k *keyringService) GetKeys() ([]string, error) {
177-
s, err := k.defaultStore()
178-
if err != nil {
179-
return []string{}, err
180-
}
181-
182-
return s.GetKeys()
183-
}
184-
185160
// GetForURL implements DataStore interface. Uses service default store.
186161
func (k *keyringService) GetForURL(url string) (CredentialsItem, error) {
187-
s, err := k.defaultStore()
188-
if err != nil {
189-
return CredentialsItem{}, err
190-
}
191-
item, err := s.GetForURL(url)
162+
item, err := k.dataStore.GetForURL(url)
192163
if err == nil {
193164
k.maskItem(item)
194165
}
@@ -197,11 +168,7 @@ func (k *keyringService) GetForURL(url string) (CredentialsItem, error) {
197168

198169
// GetForKey implements DataStore interface. Uses service default store.
199170
func (k *keyringService) GetForKey(key string) (KeyValueItem, error) {
200-
s, err := k.defaultStore()
201-
if err != nil {
202-
return KeyValueItem{}, err
203-
}
204-
item, err := s.GetForKey(key)
171+
item, err := k.dataStore.GetForKey(key)
205172
if err == nil {
206173
k.maskItem(item)
207174
}
@@ -210,18 +177,15 @@ func (k *keyringService) GetForKey(key string) (KeyValueItem, error) {
210177

211178
// AddItem implements DataStore interface. Uses service default store.
212179
func (k *keyringService) AddItem(item SecretItem) error {
213-
s, err := k.defaultStore()
214-
if err != nil {
215-
return err
216-
}
217-
218180
k.maskItem(item)
219-
return s.AddItem(item)
181+
return k.dataStore.AddItem(item)
220182
}
221183

222184
// MaskItem masks the item values
223185
func (k *keyringService) maskItem(item SecretItem) {
224186
if k.mask == nil {
187+
// Mask may be nil in unit tests for simplicity.
188+
// Mask is checked in e2e tests.
225189
return
226190
}
227191
switch dataItem := item.(type) {
@@ -234,57 +198,3 @@ func (k *keyringService) maskItem(item SecretItem) {
234198
default:
235199
}
236200
}
237-
238-
// RemoveByURL implements DataStore interface. Uses service default store.
239-
func (k *keyringService) RemoveByURL(url string) error {
240-
s, err := k.defaultStore()
241-
if err != nil {
242-
return err
243-
}
244-
return s.RemoveByURL(url)
245-
}
246-
247-
// RemoveByKey implements DataStore interface. Uses service default store.
248-
func (k *keyringService) RemoveByKey(key string) error {
249-
s, err := k.defaultStore()
250-
if err != nil {
251-
return err
252-
}
253-
return s.RemoveByKey(key)
254-
}
255-
256-
// CleanStorage implements DataStore interface. Uses service default store.
257-
func (k *keyringService) CleanStorage(item SecretItem) error {
258-
s, err := k.defaultStore()
259-
if err != nil {
260-
return err
261-
}
262-
return s.CleanStorage(item)
263-
}
264-
265-
// Exists implements DataStore, checks if keyring exists in persistent storage.
266-
func (k *keyringService) Exists() bool {
267-
s, err := k.defaultStore()
268-
if err != nil {
269-
return false
270-
}
271-
return s.Exists()
272-
}
273-
274-
// Save implements DataStore interface. Uses service default store.
275-
func (k *keyringService) Save() error {
276-
s, err := k.defaultStore()
277-
if err != nil {
278-
return err
279-
}
280-
return s.Save()
281-
}
282-
283-
// Destroy implements DataStore interface. Uses service default store.
284-
func (k *keyringService) Destroy() error {
285-
s, err := k.defaultStore()
286-
if err != nil {
287-
return err
288-
}
289-
return s.Destroy()
290-
}

‎plugin.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,6 @@ func processGetByKey(value any, opts GetKeyValueProcessorOptions, ctx action.Val
114114
return value, err
115115
}
116116

117-
// Ensure keyring storage will be accessible after save.
118-
defer k.ResetStorage()
119117
err = k.Save()
120118
if err != nil {
121119
return value, err
@@ -438,7 +436,7 @@ func (p *persistentPassphrase) init() error {
438436

439437
defer func() {
440438
// If the passphrase is set with user input or env variable, hide it.
441-
if p.file == "" && p.pass != "" {
439+
if p.mask != nil && p.file == "" && p.pass != "" {
442440
p.mask.AddString(p.pass)
443441
}
444442
p.initialized = true

‎plugin_test.go‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package keyring
22

33
import (
4+
"path/filepath"
45
"testing"
56

67
"github.com/launchrctl/launchr"
@@ -162,3 +163,64 @@ func Test_KeyringTemplate(t *testing.T) {
162163
})
163164
}
164165
}
166+
167+
func Test_KeyringSave(t *testing.T) {
168+
type testCase struct {
169+
name string
170+
dataStore func(string) DataStore
171+
}
172+
tt := []testCase{
173+
{"plain file", func(dir string) DataStore {
174+
return NewFileStore(
175+
NewPlainFile(filepath.Join(dir, defaultFileYaml)),
176+
)
177+
}},
178+
{"age file", func(dir string) DataStore {
179+
testPass := persistentPassphrase{pass: "pass"}
180+
return NewFileStore(
181+
NewAgeFile(
182+
filepath.Join(dir, defaultFileYaml+".age"),
183+
AskPassConst(testPass.get),
184+
),
185+
)
186+
}},
187+
}
188+
for _, tt := range tt {
189+
tt := tt
190+
t.Run(tt.name, func(t *testing.T) {
191+
t.Parallel()
192+
// Init test keyring service.
193+
dir := t.TempDir()
194+
keyring := NewService(tt.dataStore(dir), nil)
195+
196+
// Add an item and save.
197+
err := keyring.AddItem(KeyValueItem{Key: "foo", Value: "bar"})
198+
require.NoError(t, err)
199+
err = keyring.Save()
200+
require.NoError(t, err)
201+
202+
// Try adding another item after save.
203+
err = keyring.AddItem(KeyValueItem{Key: "bar", Value: "buz"})
204+
require.NoError(t, err)
205+
err = keyring.Save()
206+
require.NoError(t, err)
207+
208+
// Check the content of the keyring.
209+
assertKeyringVals := func() {
210+
val1, err := keyring.GetForKey("foo")
211+
assert.NoError(t, err)
212+
assert.Equal(t, "bar", val1.Value.(string))
213+
214+
val2, err := keyring.GetForKey("bar")
215+
assert.NoError(t, err)
216+
assert.Equal(t, "buz", val2.Value.(string))
217+
}
218+
assertKeyringVals()
219+
220+
// Try to recreate a service and make sure that the content is definitely loaded from the disk.
221+
keyring = NewService(tt.dataStore(dir), nil)
222+
// Check the content of the keyring.
223+
assertKeyringVals()
224+
})
225+
}
226+
}

0 commit comments

Comments
 (0)