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

Transparent encryption #281

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Transparent encryption #281

wants to merge 3 commits into from

Conversation

urykhy
Copy link

@urykhy urykhy commented Nov 10, 2021

Fixes #280

@urykhy urykhy marked this pull request as ready for review November 10, 2021 17:36
@colinmarc
Copy link
Owner

Hi @urykhy, thanks for the huge contribution!

I am interested in supporting this feature, but I lack the context to understand if the implementation is correct. If you're willing to put a bit more work in, I think we could get this merged.

To start with, I would feel better if we could add some more tests. The tests you added are a good start, but they are far from comprehensively testing this behavior. I wonder if it would make sense to run the entire test suite again (as another job on gh actions) with /_test marked as an encryption zone, rather than trying to duplicate tests here and there. We also need tests for the case where we don't have the key available, to make sure our error messages and exit statuses make sense.

Makefile Outdated Show resolved Hide resolved
aes_test.go Outdated Show resolved Hide resolved
@urykhy urykhy force-pushed the encryption branch 9 times, most recently from 29f7c79 to 07e9b80 Compare February 13, 2022 20:27
@urykhy urykhy force-pushed the encryption branch 4 times, most recently from ecd3fb3 to 07b180c Compare February 14, 2022 17:21
aes.go Outdated Show resolved Hide resolved
kms.go Show resolved Hide resolved
kms.go Outdated Show resolved Hide resolved
@urykhy urykhy force-pushed the encryption branch 7 times, most recently from fc86aad to 27b129b Compare February 14, 2022 19:09
kms.go Outdated Show resolved Hide resolved
@urykhy urykhy force-pushed the encryption branch 5 times, most recently from 1328f22 to 8e23a17 Compare February 26, 2022 11:30
@urykhy
Copy link
Author

urykhy commented Feb 26, 2022

We also need tests for the case where we don't have the key available, to make sure our error messages and exit statuses make sense.

test added.

@urykhy urykhy force-pushed the encryption branch 2 times, most recently from 2680dfb to 982b1e4 Compare February 26, 2022 12:14
file_writer.go Outdated Show resolved Hide resolved
aes.go Outdated Show resolved Hide resolved
@urykhy urykhy force-pushed the encryption branch 3 times, most recently from 00a4963 to 00d1c5b Compare March 7, 2022 07:00
file_reader.go Outdated Show resolved Hide resolved
file_writer.go Outdated Show resolved Hide resolved
file_reader.go Outdated Show resolved Hide resolved
file_reader.go Outdated Show resolved Hide resolved
file_reader.go Outdated Show resolved Hide resolved
file_reader.go Outdated Show resolved Hide resolved
file_writer.go Show resolved Hide resolved
file_writer.go Outdated Show resolved Hide resolved
file_writer.go Outdated Show resolved Hide resolved
file_writer.go Outdated Show resolved Hide resolved
file_writer_test.go Show resolved Hide resolved
file_writer_test.go Outdated Show resolved Hide resolved
file_writer.go Outdated Show resolved Hide resolved
@colinmarc
Copy link
Owner

Thanks for sticking with this!

@Yash9060
Copy link

@colinmarc Any plans to merge this pr ?

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.

Transparent encryption
3 participants