Fail Credentials validation if it contains extra data #276
Conversation
| return nil, fmt.Errorf("entry %s: invalid credentials data: %v", id, err) | ||
| } | ||
| var validationErrors error | ||
| credentialsMap["type"] = credentialsType |
There was a problem hiding this comment.
Ça c'est vraiment l'incarnation golang de :sifflercommeunépais: et :ctutoéquipisse-ouainmaisçaapasderapport:
There was a problem hiding this comment.
Ouais je n'avais pas trop le choix de faire une passe de même ou de réécrire une grosse partie du code ou de faire un genre de deepcopy maison 🤷🏻
| if err != nil { | ||
| validationErrors = multierror.Append(validationErrors, fmt.Errorf("entry %s: unable to create a decoder: %v", id, err)) |
There was a problem hiding this comment.
As of go 1.20, they added this to the std lib. You can simply use this. If this project is not at version 1.20, then this is fine unless you want to update the version of go.
There was a problem hiding this comment.
The project is using Go 1.21, so that should be ok.
But, I tried using the package and I don't like the interface, so I don't think it's worth it.
There was a problem hiding this comment.
Okay, thought the interface was kinda the same. In any case multierror is fine.
credentials/credentials.go
Outdated
| if err != nil { | ||
| validationErrors = multierror.Append(validationErrors, fmt.Errorf("entry %s: unable to create a decoder: %v", id, err)) | ||
| } | ||
| if err := decoder.Decode(credentialsMap); err != nil { | ||
| return nil, fmt.Errorf("entry %s: invalid credentials data: %v", id, err) |
There was a problem hiding this comment.
No part of the code you changed, but the error created here should be rather:
fmt.Errorf("entry %s: invalid credentials data: %w", id, err)
%w is used for wrapping an error and should be used instead of %v when the value is an error.
credentials/credentials_aws_test.go
Outdated
| } | ||
|
|
||
| cred, err := ParseSingleCredentials(credMap) | ||
| assert.Nil(t, err) |
There was a problem hiding this comment.
This should be:
assert.NoError(t, err)
There was a problem hiding this comment.
Oh that explains it! assert.Nil(t, err) is used elsewhere in the code and it did not fail when it should have
This is especially useful for "target_tags" for which a malformed value could lead to a credentials leak