Standardize crypto errors #7558

chaitanyab2311 · 2024-02-23T03:42:23Z

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

Code compiles correctly
Created/updated tests
Unit tests passing
End-to-end tests passing

chaitanyab2311 · 2024-02-23T03:43:57Z

@cicoyle I am getting some errors related to MockActors in http_test while running tests locally. Am i missing something

pkg/api/errors/cryptography.go

pkg/api/universal/subtlecrypto_subtlecrypto.go

ItalyPaleAle · 2024-02-23T04:16:07Z

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.

cicoyle · 2024-02-23T14:30:39Z

@cicoyle I am getting some errors related to MockActors in http_test while running tests locally. Am i missing something

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: -tags=stablecomponents,unit

Here is a screenshot of my Goland IDE config I use.

Once you get to the integration tests too, you will need a tag: go test -v -race -tags integration ./tests/integration -focus errors, where errors might change pending what you'd like to 'focus' on

cicoyle · 2024-02-23T14:32:59Z

pkg/api/errors/cryptography.go

+		Build()
+}
+
+func CryptoNotFound(name string) error {


same here. There is a generic NotFound you can use here: https://github.com/dapr/dapr/blob/master/pkg/api/errors/errors.go#L11

cicoyle · 2024-02-23T14:40:07Z

pkg/api/errors/cryptography.go

+		Build()
+}
+
+func CryptoProviderGetKey(name string, err string) error {


Suggested change

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

cicoyle · 2024-02-23T14:43:07Z

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 git commit -s then the DCO step in CI will pass.

Signed-off-by: Chaitanya Bhangale <chaitanyabhangale@Chaitanyas-MacBook-Pro-2.local>

ItalyPaleAle · 2024-02-23T23:40:26Z

pkg/api/universal/subtlecrypto_subtlecrypto.go

+		err = fmt.Errorf("failed to encrypt")
+		err = apierrors.CryptoProviderEncryptOperation(in.ComponentName, err)


Nit: use errors.New if there's no format (like, no %s), which is faster (same below)

Suggested change

err = fmt.Errorf("failed to encrypt")

err = apierrors.CryptoProviderEncryptOperation(in.ComponentName, err)

err = apierrors.CryptoProviderEncryptOperation(in.ComponentName, errors.New("failed to encrypt"))

ItalyPaleAle · 2024-02-23T23:40:47Z

pkg/api/universal/subtlecrypto_subtlecrypto.go

+		err = fmt.Errorf("failed to decrypt")
+		err = apierrors.CryptoProviderDecryptOperation(in.ComponentName, err)


Suggested change

err = fmt.Errorf("failed to decrypt")

err = apierrors.CryptoProviderDecryptOperation(in.ComponentName, err)

err = apierrors.CryptoProviderDecryptOperation(in.ComponentName, errors.New("failed to decrypt"))

ItalyPaleAle · 2024-02-23T23:41:21Z

pkg/api/universal/subtlecrypto_subtlecrypto.go

+		err = fmt.Errorf("failed to wrap key")
+		err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err)


Suggested change

err = fmt.Errorf("failed to wrap key")

err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err)

err = apierrors.CryptoProviderKeyOperation(in.ComponentName, errors.New("failed to wrap key"))

ItalyPaleAle · 2024-02-23T23:41:41Z

pkg/api/universal/subtlecrypto_subtlecrypto.go

+		err = fmt.Errorf("failed to unwrap key")
+		err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err)


Suggested change

err = fmt.Errorf("failed to unwrap key")

err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err)

err = apierrors.CryptoProviderKeyOperation(in.ComponentName, errors.New("failed to unwrap key"))

ItalyPaleAle · 2024-02-23T23:42:08Z

pkg/api/universal/subtlecrypto_subtlecrypto.go

+		err = fmt.Errorf("failed to sign")
+		err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err)


Suggested change

err = fmt.Errorf("failed to sign")

err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err)

err = apierrors.CryptoProviderKeyOperation(in.ComponentName, errors.New("failed to sign"))

ItalyPaleAle · 2024-02-23T23:42:32Z

pkg/api/universal/subtlecrypto_subtlecrypto.go

+		err = fmt.Errorf("failed to verify signature: ")
+		err = apierrors.CryptoProviderVerifySignatureOperation(in.ComponentName, err)


Suggested change

err = fmt.Errorf("failed to verify signature: ")

err = apierrors.CryptoProviderVerifySignatureOperation(in.ComponentName, err)

err = apierrors.CryptoProviderVerifySignatureOperation(in.ComponentName, errors.New("failed to verify signature"))

ItalyPaleAle · 2024-02-23T23:43:17Z

pkg/api/errors/cryptography.go

Just a nit, I'd name this "crypto.go" since that's how all other files are named

dapr-bot · 2024-04-24T16:51:43Z

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!

dapr-bot · 2024-05-01T17:03:20Z

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!

yaron2 · 2024-05-01T17:06:19Z

@chaitanyab2311 are you planning to take this PR out of draft?

daixiang0 · 2024-05-13T00:59:19Z

ping @chaitanyab2311

Standardize crypto errors

3f2803f

chaitanyab2311 requested review from a team as code owners February 23, 2024 03:42

chaitanyab2311 marked this pull request as draft February 23, 2024 03:42

ItalyPaleAle requested changes Feb 23, 2024

View reviewed changes

cicoyle reviewed Feb 23, 2024

View reviewed changes

Chaitanya Bhangale added 2 commits February 23, 2024 15:44

Incorporated review comments

d74bdde

Signed-off-by: Chaitanya Bhangale <chaitanyabhangale@Chaitanyas-MacBook-Pro-2.local>

Incorporated review comments

7f9d7e6

Signed-off-by: Chaitanya Bhangale <chaitanyabhangale@Chaitanyas-MacBook-Pro-2.local>

ItalyPaleAle reviewed Feb 23, 2024

View reviewed changes

Incorporated review comments

ccdfb96

dapr-bot added the stale Issues and PRs without response label Apr 24, 2024

dapr-bot closed this May 1, 2024

yaron2 reopened this May 1, 2024

dapr-bot removed the stale Issues and PRs without response label May 1, 2024

chaitanyab2311 marked this pull request as ready for review May 1, 2024 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize crypto errors #7558

Standardize crypto errors #7558

chaitanyab2311 commented Feb 23, 2024 •

edited

chaitanyab2311 commented Feb 23, 2024

ItalyPaleAle commented Feb 23, 2024

cicoyle commented Feb 23, 2024 •

edited

cicoyle Feb 23, 2024

cicoyle Feb 23, 2024

cicoyle commented Feb 23, 2024

ItalyPaleAle Feb 23, 2024

ItalyPaleAle Feb 23, 2024

ItalyPaleAle Feb 23, 2024

ItalyPaleAle Feb 23, 2024

ItalyPaleAle Feb 23, 2024

ItalyPaleAle Feb 23, 2024

ItalyPaleAle Feb 23, 2024

dapr-bot commented Apr 24, 2024

dapr-bot commented May 1, 2024

yaron2 commented May 1, 2024

daixiang0 commented May 13, 2024

	func CryptoProviderGetKey(name string, err string) error {
	func CryptoProviderGetKey(name string, msg string) error {

		err = fmt.Errorf("failed to encrypt")
		err = apierrors.CryptoProviderEncryptOperation(in.ComponentName, err)

		err = fmt.Errorf("failed to decrypt")
		err = apierrors.CryptoProviderDecryptOperation(in.ComponentName, err)

		err = fmt.Errorf("failed to wrap key")
		err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err)

		err = fmt.Errorf("failed to unwrap key")
		err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err)

		err = fmt.Errorf("failed to sign")
		err = apierrors.CryptoProviderKeyOperation(in.ComponentName, err)

		err = fmt.Errorf("failed to verify signature: ")
		err = apierrors.CryptoProviderVerifySignatureOperation(in.ComponentName, err)

Standardize crypto errors #7558

Are you sure you want to change the base?

Standardize crypto errors #7558

Conversation

chaitanyab2311 commented Feb 23, 2024 • edited

Description

Issue reference

Checklist

chaitanyab2311 commented Feb 23, 2024

ItalyPaleAle commented Feb 23, 2024

cicoyle commented Feb 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cicoyle commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapr-bot commented Apr 24, 2024

dapr-bot commented May 1, 2024

yaron2 commented May 1, 2024

daixiang0 commented May 13, 2024

chaitanyab2311 commented Feb 23, 2024 •

edited

cicoyle commented Feb 23, 2024 •

edited