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.

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

Handle errors #1311

Merged
merged 4 commits into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion cmd/sops/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ func DecryptTree(opts DecryptTreeOpts) (dataKey []byte, err error) {
}
fileMac, err := opts.Cipher.Decrypt(opts.Tree.Metadata.MessageAuthenticationCode, dataKey, opts.Tree.Metadata.LastModified.Format(time.RFC3339))
if !opts.IgnoreMac {
if err != nil {
return nil, NewExitError(fmt.Sprintf("Cannot decrypt MAC: %s", err), codes.MacMismatch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If err != nil, then decryption failed for some reason we should tell the user, instead of just saying that there was a MAC mismatch, with the file having a MAC of nil.

}
if fileMac != computedMac {
// If the file has an empty MAC, display "no MAC" instead of not displaying anything
if fileMac == "" {
Expand Down Expand Up @@ -318,10 +321,10 @@ func FixAWSKMSEncryptionContextBug(opts GenericDecryptOpts, tree *sops.Tree) (*s
}

file, err := os.Create(opts.InputPath)
defer file.Close()
if err != nil {
return nil, NewExitError(fmt.Sprintf("Could not open file for writing: %s", err), codes.CouldNotWriteOutputFile)
}
defer file.Close()
_, err = file.Write(encryptedFile)
if err != nil {
return nil, err
Expand Down
5 changes: 5 additions & 0 deletions cmd/sops/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -1234,6 +1234,11 @@ func extractSetArguments(set string) (path []interface{}, valueToInsert interfac
fullPath := strings.TrimRight(pathValuePair[0], " ")
jsonValue := pathValuePair[1]
valueToInsert, err = jsonValueToTreeInsertableValue(jsonValue)
if err != nil {
// All errors returned by jsonValueToTreeInsertableValue are created by common.NewExitError(),
// so we can simply pass them on
return nil, nil, err
}

path, err = parseTreePath(fullPath)
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions kms/keysource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ func TestMasterKey_createKMSConfig(t *testing.T) {
assert.NoError(t, err)

creds, err := cfg.Credentials.Retrieve(context.TODO())
assert.Nil(t, err)
assert.Equal(t, "id", creds.AccessKeyID)
assert.Equal(t, "secret", creds.SecretAccessKey)
assert.Equal(t, "token", creds.SessionToken)
Expand Down
3 changes: 3 additions & 0 deletions pgp/keysource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ func TestMasterKey_Decrypt(t *testing.T) {
fingerprint,
"--no-encrypt-to",
}, bytes.NewReader(data))
assert.Nil(t, err)
assert.NoErrorf(t, gnuPGHome.ImportFile(mockPrivateKey), stderr.String())

encryptedData := stdout.String()
Expand Down Expand Up @@ -414,6 +415,7 @@ func TestMasterKey_decryptWithOpenPGP(t *testing.T) {
fingerprint,
"--no-encrypt-to",
}, bytes.NewReader(data))
assert.Nil(t, err)
assert.NoErrorf(t, gnuPGHome.ImportFile(mockPrivateKey), stderr.String())

encryptedData := stdout.String()
Expand Down Expand Up @@ -462,6 +464,7 @@ func TestMasterKey_decryptWithGnuPG(t *testing.T) {
fingerprint,
"--no-encrypt-to",
}, bytes.NewReader(data))
assert.Nil(t, err)
assert.NoErrorf(t, gnuPGHome.ImportFile(mockPrivateKey), stderr.String())

encryptedData := stdout.String()
Expand Down
12 changes: 8 additions & 4 deletions stores/yaml/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,10 @@ func (store Store) appendYamlNodeToTreeBranch(node *yaml.Node, branch sops.TreeB
return nil, fmt.Errorf("YAML documents that are values are not supported")
case yaml.AliasNode:
branch, err = store.appendYamlNodeToTreeBranch(node.Alias, branch, false)
if err != nil {
// This should never happen since node.Alias was already successfully decoded before
return nil, err
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'm pretty sure this should never happen, but it definitely won't hurt to still pass on the error if it actually does.

}
}
if !commentsWereHandled {
branch = store.appendCommentToMap(node.FootComment, branch)
Expand Down Expand Up @@ -204,9 +208,9 @@ func (store *Store) appendSequence(in []interface{}, sequence *yaml.Node) {
}
if len(comments) > 0 {
if beginning {
comments = store.addCommentsHead(sequence, comments)
store.addCommentsHead(sequence, comments)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The return value is always nil. It's mainly returned so you can write comments = store.addCommentsHead(xx, comments) and continue using comments later on. But we have no "later" here, so removing the assignment is best (and makes the linter happy).

} else {
comments = store.addCommentsFoot(sequence.Content[len(sequence.Content)-1], comments)
store.addCommentsFoot(sequence.Content[len(sequence.Content)-1], comments)
}
}
}
Expand All @@ -231,9 +235,9 @@ func (store *Store) appendTreeBranch(branch sops.TreeBranch, mapping *yaml.Node)
}
if len(comments) > 0 {
if beginning {
comments = store.addCommentsHead(mapping, comments)
store.addCommentsHead(mapping, comments)
} else {
comments = store.addCommentsFoot(mapping.Content[len(mapping.Content)-2], comments)
store.addCommentsFoot(mapping.Content[len(mapping.Content)-2], comments)
}
}
}
Expand Down
59 changes: 59 additions & 0 deletions stores/yaml/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,59 @@ var BRANCHES = sops.TreeBranches{
},
}

var ALIASES = []byte(`---
key1: &foo
- foo
key2: *foo
key3: &bar
foo: bar
baz: bam
key4: *bar
`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having a test with anchors and aliases surely doesn't hurt.


var ALIASES_BRANCHES = sops.TreeBranches{
sops.TreeBranch{
sops.TreeItem{
Key: "key1",
Value: []interface{}{
"foo",
},
},
sops.TreeItem{
Key: "key2",
Value: []interface{}{
"foo",
},
},
sops.TreeItem{
Key: "key3",
Value: sops.TreeBranch{
sops.TreeItem{
Key: "foo",
Value: "bar",
},
sops.TreeItem{
Key: "baz",
Value: "bam",
},
},
},
sops.TreeItem{
Key: "key4",
Value: sops.TreeBranch{
sops.TreeItem{
Key: "foo",
Value: "bar",
},
sops.TreeItem{
Key: "baz",
Value: "bam",
},
},
},
},
}

var COMMENT_1 = []byte(`# test
a:
b: null
Expand Down Expand Up @@ -170,6 +223,12 @@ func TestLoadPlainFile(t *testing.T) {
assert.Equal(t, BRANCHES, branches)
}

func TestLoadAliasesPlainFile(t *testing.T) {
branches, err := (&Store{}).LoadPlainFile(ALIASES)
assert.Nil(t, err)
assert.Equal(t, ALIASES_BRANCHES, branches)
}

func TestComment1(t *testing.T) {
// First iteration: load and store
branches, err := (&Store{}).LoadPlainFile(COMMENT_1)
Expand Down
Loading