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

feat: Doc field encryption #2817

Merged

Conversation

islamaliev
Copy link
Contributor

@islamaliev islamaliev commented Jul 8, 2024

Relevant issue(s)

Resolves #2809 #2812

Description

Adds field-level encryption which allows separate fields
to be encryption with a dedicated symmectic key.

@islamaliev islamaliev self-assigned this Jul 8, 2024
@islamaliev islamaliev requested a review from a team July 8, 2024 11:31
@islamaliev islamaliev added area/datastore Related to the datastore / storage engine system area/api Related to the external API component security Related to security labels Jul 8, 2024
@islamaliev islamaliev added this to the DefraDB v0.13 milestone Jul 8, 2024
Copy link

codecov bot commented Jul 8, 2024

Codecov Report

Attention: Patch coverage is 97.61905% with 4 lines in your changes missing coverage. Please review.

Project coverage is 79.17%. Comparing base (32092ac) to head (7398c12).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2817      +/-   ##
===========================================
+ Coverage    79.14%   79.17%   +0.03%     
===========================================
  Files          318      319       +1     
  Lines        24162    24256      +94     
===========================================
+ Hits         19122    19203      +81     
- Misses        3660     3667       +7     
- Partials      1380     1386       +6     
Flag Coverage Δ
all-tests 79.17% <97.62%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
cli/utils.go 71.26% <ø> (-0.31%) ⬇️
client/request/mutation.go 100.00% <ø> (ø)
datastore/prefix_query.go 86.67% <100.00%> (-1.21%) ⬇️
http/client_collection.go 44.75% <100.00%> (+0.77%) ⬆️
http/handler_collection.go 71.52% <100.00%> (+0.54%) ⬆️
internal/core/key.go 84.28% <100.00%> (+0.31%) ⬆️
internal/db/collection.go 73.65% <100.00%> (+0.81%) ⬆️
internal/db/errors.go 65.68% <100.00%> (+0.23%) ⬆️
internal/encryption/context.go 100.00% <100.00%> (ø)
internal/merkle/clock/clock.go 68.75% <100.00%> (-1.79%) ⬇️
... and 7 more

... and 11 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

cli/utils.go Outdated Show resolved Hide resolved
@shahzadlone
Copy link
Member

Likely also want to add the Resolve #2812 in your PR description

Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

LGTM Islam, thanks :) Just got a couple of todo's for your RE GQL and testing.

tests/integration/utils2.go Outdated Show resolved Hide resolved
internal/request/graphql/schema/generate.go Outdated Show resolved Hide resolved
tests/integration/test_case.go Show resolved Hide resolved
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

LGTM - thanks Islam :)

tests/integration/utils2.go Outdated Show resolved Hide resolved
internal/request/graphql/schema/generate.go Show resolved Hide resolved
Copy link
Member

@nasdf nasdf left a comment

Choose a reason for hiding this comment

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

LGTM. Great work!

One thought I had after reviewing: it might be beneficial to merge all of the db context related stuff into a single package at some point in the future.

@AndrewSisley
Copy link
Contributor

One thought I had after reviewing: it might be beneficial to merge all of the db context related stuff into a single package at some point in the future.

I like that thought :)

@islamaliev
Copy link
Contributor Author

One thought I had after reviewing: it might be beneficial to merge all of the db context related stuff into a single package at some point in the future.

I'm not a big fan to be honest. I'd much prefer to split and group packages by their domain, not based on their functional types. Just because they have something to do with context doesn't mean they are cohesive enough to put them in one package.

@islamaliev islamaliev merged commit d73b05b into sourcenetwork:develop Jul 9, 2024
37 of 39 checks passed
@AndrewSisley
Copy link
Contributor

AndrewSisley commented Jul 9, 2024

I'd much prefer to split and group packages by their domain, not based on their functional types. Just because they have something to do with context doesn't mean they are cohesive enough to put them in one package.

I understand what you mean, but for me (thinking as a user) I see them all in the 'Defra' domain, just like we lump config, and the client package into singular clumps. defra.config, defra.ctx, defra.client, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Related to the external API component area/datastore Related to the datastore / storage engine system security Related to security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doc Encryption: enable field-level encryption
4 participants