-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Standardize crypto errors #7558
base: master
Are you sure you want to change the base?
Standardize crypto errors #7558
Conversation
@cicoyle I am getting some errors related to MockActors in http_test while running tests locally. Am i missing something |
Please note that with crypto operations, errors are sensitive as they can disclose information to users that can be used for attacks (side channel). Please make sure errors from the crypto components are never returned to callers. They are ok to log, but not returned in the API response - return a generic "failed to perform operation" error instead. |
I believe you might need the tags in your Go Tool Args. When you run the tests in Dapr you will likely need to add these tags: Here is a screenshot of my Goland IDE config I use. Once you get to the integration tests too, you will need a tag: |
pkg/api/errors/cryptography.go
Outdated
Build() | ||
} | ||
|
||
func CryptoNotFound(name string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. There is a generic NotFound
you can use here: https://github.com/dapr/dapr/blob/master/pkg/api/errors/errors.go#L11
pkg/api/errors/cryptography.go
Outdated
Build() | ||
} | ||
|
||
func CryptoProviderGetKey(name string, err string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func CryptoProviderGetKey(name string, err string) error { | |
func CryptoProviderGetKey(name string, msg string) error { |
Then you can get rid of the message := err
line below :)
Or if the intent was to use the string version of the initial error, then it should be of type error
when passed in
This is off to a great start! 🎉 If the tags don't fix your MockActors issues, then please comment and add the error you are seeing. Also, when you commit, if you do |
Signed-off-by: Chaitanya Bhangale <chaitanyabhangale@Chaitanyas-MacBook-Pro-2.local>
Signed-off-by: Chaitanya Bhangale <chaitanyabhangale@Chaitanyas-MacBook-Pro-2.local>
err = fmt.Errorf("failed to encrypt") | ||
err = apierrors.CryptoProviderEncryptOperation(in.ComponentName, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: use errors.New
if there's no format (like, no %s
), which is faster (same below)
err = fmt.Errorf("failed to encrypt") | |
err = apierrors.CryptoProviderEncryptOperation(in.ComponentName, err) | |
err = apierrors.CryptoProviderEncryptOperation(in.ComponentName, errors.New("failed to encrypt")) |
err = fmt.Errorf("failed to decrypt") | ||
err = apierrors.CryptoProviderDecryptOperation(in.ComponentName, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err = fmt.Errorf("failed to decrypt") | |
err = apierrors.CryptoProviderDecryptOperation(in.ComponentName, err) | |
err = apierrors.CryptoProviderDecryptOperation(in.ComponentName, errors.New("failed to decrypt")) |
err = fmt.Errorf("failed to wrap key") | ||
err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err = fmt.Errorf("failed to wrap key") | |
err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err) | |
err = apierrors.CryptoProviderKeyOperation(in.ComponentName, errors.New("failed to wrap key")) |
err = fmt.Errorf("failed to unwrap key") | ||
err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err = fmt.Errorf("failed to unwrap key") | |
err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err) | |
err = apierrors.CryptoProviderKeyOperation(in.ComponentName, errors.New("failed to unwrap key")) |
err = fmt.Errorf("failed to sign") | ||
err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err = fmt.Errorf("failed to sign") | |
err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err) | |
err = apierrors.CryptoProviderKeyOperation(in.ComponentName, errors.New("failed to sign")) |
err = fmt.Errorf("failed to verify signature: ") | ||
err = apierrors.CryptoProviderVerifySignatureOperation(in.ComponentName, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err = fmt.Errorf("failed to verify signature: ") | |
err = apierrors.CryptoProviderVerifySignatureOperation(in.ComponentName, err) | |
err = apierrors.CryptoProviderVerifySignatureOperation(in.ComponentName, errors.New("failed to verify signature")) |
pkg/api/errors/cryptography.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit, I'd name this "crypto.go" since that's how all other files are named
This pull request has been automatically marked as stale because it has not had activity in the last 60 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 67 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
@chaitanyab2311 are you planning to take this PR out of draft? |
ping @chaitanyab2311 |
Description
Enriched errors in cryptography API. This is still a draft as unit tests need to be updated and integration tests need to be added
Issue reference
Please reference the issue this PR will close: #7491
Checklist