refactor: Replace file based with use of Secret Provider to get Access Tokens #784
Conversation
…s Tokens closes #769 BREAKING CHANGE: All App Services running with the secure Edgex Stack now need to have the SecretStore configured, a Vault token created and run with EDGEX_SECURITY_SECRET_STORE=true. Signed-off-by: lenny <[email protected]>
lenny-goodell
force-pushed
the
access-token-reworked
branch
from
75b460f to
dd31cd7
Compare
internal/app/service.go
Outdated
| @@ -430,7 +429,7 @@ func (svc *Service) Initialize() error { | |||
| secretProvider := bootstrapContainer.SecretProviderFrom(svc.dic.Get) | |||
| credentials, err := secretProvider.GetSecrets(svc.config.Database.Type) | |||
| if err != nil { | |||
| return fmt.Errorf("unable to set RedisStreams password from DB credentials") | |||
| return fmt.Errorf("unable to set RedisStreams password from DB credentials: %s", err.Error()) | |||
There was a problem hiding this comment.
I thought we are supposed to use %w now.
There was a problem hiding this comment.
Nope, not sure what %w is. It is a string returned from err.Error() so %s is most appropriate. Some folks us %v which will work also, but I like to be more explicate.
There was a problem hiding this comment.
Also %v leaves it up to reflection to figure our the best way to convert the value to a string. So when it is a string why not skip the reflection and use %s...
There was a problem hiding this comment.
| return fmt.Errorf("unable to set RedisStreams password from DB credentials: %s", err.Error()) | |
| return fmt.Errorf("unable to set RedisStreams password from DB credentials: %w", err) |
There was a problem hiding this comment.
Ok, just looked up %w. It is specifically for when wrapping errors . Good to know. :-) THX!
Wrapping an error with %w makes it available to errors.Is and errors.As:
err := fmt.Errorf("access denied: %w", ErrPermission)
...
if errors.Is(err, ErrPermission) ...
There was a problem hiding this comment.
Good point. If that's the case, we shouldn't propagate that error at all.
There was a problem hiding this comment.
Nope, it isn't leaking the credential that it couldn't get from the SecretStore. ;-)
The error give details on why if could find the credentials.
There was a problem hiding this comment.
if that is the case, then it is ok; i was just worries the error was wrapped from vault or redis's server's error which might have some sensitive infor there. ok if not
There was a problem hiding this comment.
@bnevis-i , you good with this explanation of why it isn't a creds leak?
Signed-off-by: lenny <[email protected]>
Signed-off-by: lenny <[email protected]>
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently expecting the Registry/Config Access tokens to be retrieved for files using the file location config settings.
Issue Number: #769
What is the new behavior?
Now uses SecretProvider to retrieve the access tokens.
File location config settings have been removed
Does this PR introduce a breaking change?
BREAKING CHANGE: All App Services running with the secure Edgex Stack now need to have the SecretStore configured, a Vault token created and run with EDGEX_SECURITY_SECRET_STORE=true.
Are there any new imports or modules? If so, what are they used for and why?
no
Are there any specific instructions or things that should be known prior to reviewing?
Other information