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

add automatic state store encryption #3589

Merged
merged 14 commits into from
Aug 30, 2021
Merged

add automatic state store encryption #3589

merged 14 commits into from
Aug 30, 2021

Conversation

yaron2
Copy link
Member

@yaron2 yaron2 commented Aug 25, 2021

This PR brings automatic state encryption with key rotation support to all Dapr state stores.

Closes #1090.

@yaron2 yaron2 requested review from a team as code owners August 25, 2021 23:03
@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #3589 (fd8ff70) into master (fe02831) will decrease coverage by 0.36%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3589      +/-   ##
==========================================
- Coverage   60.64%   60.28%   -0.37%     
==========================================
  Files          97       99       +2     
  Lines        8790     8968     +178     
==========================================
+ Hits         5331     5406      +75     
- Misses       3078     3160      +82     
- Partials      381      402      +21     
Impacted Files Coverage Δ
pkg/config/configuration.go 71.01% <ø> (ø)
pkg/grpc/api.go 58.19% <0.00%> (-3.06%) ⬇️
pkg/http/api.go 79.06% <0.00%> (-3.37%) ⬇️
pkg/encryption/encryption.go 63.51% <63.51%> (ø)
pkg/runtime/runtime.go 54.35% <69.23%> (+0.32%) ⬆️
pkg/encryption/state.go 82.60% <82.60%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe02831...fd8ff70. Read the comment docs.

pkg/encryption/encryption.go Outdated Show resolved Hide resolved
pkg/encryption/encryption.go Outdated Show resolved Hide resolved
pkg/encryption/state.go Outdated Show resolved Hide resolved
@yaron2 yaron2 requested a review from artursouza August 26, 2021 16:30
pkg/encryption/state.go Outdated Show resolved Hide resolved
Copy link
Member

@artursouza artursouza left a comment

Choose a reason for hiding this comment

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

Sorry, I found this thing in how it is serializing into byte[] prior to encrypting. Can we have an E2E test showing that this works? Also, I think we should wrap this as a experimental feature.

pkg/runtime/runtime.go Outdated Show resolved Hide resolved
pkg/runtime/runtime.go Outdated Show resolved Hide resolved
pkg/http/api.go Show resolved Hide resolved
@yaron2
Copy link
Member Author

yaron2 commented Aug 26, 2021

Sorry, I found this thing in how it is serializing into byte[] prior to encrypting. Can we have an E2E test showing that this works? Also, I think we should wrap this as a experimental feature.

I added a test to ensure base64 values get encrypted and decrypted successfully.

Copy link
Contributor

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

I agree with Artur on this, since we are modifying one of the most basic features of Dapr, we need to have an E2E for it for sure as well as wrap it in a feature flag as experimental for the first release and then later release it completely.

pkg/encryption/encryption.go Outdated Show resolved Hide resolved
pkg/encryption/state.go Outdated Show resolved Hide resolved
pkg/encryption/encryption.go Show resolved Hide resolved
secondaryEncryptionKey = "secondaryEncryptionKey"
errPrefix = "failed to extract encryption key"
AES256Algorithm = "AES256"
FeatureName = "encryption"
Copy link
Member

Choose a reason for hiding this comment

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

nit: please, use this enum instead:

PubSubRouting Feature = "PubSub.Routing"

Copy link
Member

Choose a reason for hiding this comment

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

Also, name it State.Encryption.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the first time I see the "centralized" list.. it's mostly an anti-pattern in Go as packages are expected to be self-governed and all encompassing, also test dependencies or 3rd parties wanting to use the feature names now need to pull the dependencies of configuration. I made that change for now but will probably open an issue to disperse these among their respective packages.

@artursouza artursouza merged commit 7afc7c6 into dapr:master Aug 30, 2021
@yaron2 yaron2 deleted the stateencrypt-1 branch August 30, 2021 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable automatic encryption of state stores
4 participants