Skip to content

Clear encryption data when no longer needed #2616

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.

Sign up for GitHub

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

Jump to bottom
Closed
dlmarion wants to merge 2 commits into from
Closed

Clear encryption data when no longer needed #2616

dlmarion wants to merge 2 commits into from

Conversation

@dlmarion
Copy link
Contributor

@dlmarion dlmarion commented Apr 7, 2022

No description provided.

@dlmarion dlmarion requested a review from milleruntime April 7, 2022 19:14
@dlmarion
Copy link
Contributor Author

dlmarion commented Apr 7, 2022

There are still some issues with this PR, just looking to get feedback on the approach.

@dlmarion dlmarion requested a review from ctubbsii April 7, 2022 19:15
milleruntime
milleruntime reviewed Apr 8, 2022
View reviewed changes
core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java
kekId = null;
Arrays.fill(encFek, (byte) 0);
}

Copy link
Contributor

@milleruntime milleruntime Apr 8, 2022

Choose a reason for hiding this comment

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

These changes look good so far. I like using AutoCloseable, that is a good idea.

milleruntime
milleruntime reviewed Apr 8, 2022
View reviewed changes
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java
}
} else {
// read crypto parameters and get decrypter
this.in.seek(offsetCryptoParameters);
Copy link
Contributor

@milleruntime milleruntime Apr 8, 2022

Choose a reason for hiding this comment

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

We might want to put some checks in to ensure that the CryptoService is open so we don't write null values to the file.

ctubbsii
ctubbsii reviewed Apr 12, 2022
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

I have not had a chance to review this yet. However, it seems quite large, and some of it probably conflicts with the design changes in #2197. I would prefer to focus efforts on reviewing #2197. I think this should be held back until that one is merged. Any problem with that, @dlmarion ?

@dlmarion
Copy link
Contributor Author

dlmarion commented Apr 12, 2022

I think this should be held back until that one is merged.

I have no issue with that

However, it seems quite large, and some of it probably conflicts with the design changes in #2197.

IIRC most of the changes here have to do with the fact that CryptoService is now AutoClosable

@dlmarion dlmarion self-assigned this Apr 12, 2022
@milleruntime
Copy link
Contributor

milleruntime commented Apr 12, 2022

However, it seems quite large, and some of it probably conflicts with the design changes in #2197.

IIRC most of the changes here have to do with the fact that CryptoService is now AutoClosable

The changes in #2197 are also incomplete so that would be another reason to wait and could be why this PR looks so big.

@milleruntime milleruntime mentioned this pull request Apr 12, 2022
Merged
@ctubbsii ctubbsii mentioned this pull request May 18, 2022
Merged
@dlmarion
Copy link
Contributor Author

dlmarion commented Sep 16, 2022

Closing this for now as I'm not sure that adding AutoCloseable to the CryptoService is the right thing to do.

@dlmarion dlmarion closed this Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ctubbsii ctubbsii ctubbsii left review comments

@milleruntime milleruntime milleruntime left review comments

Assignees

@dlmarion dlmarion

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dlmarion @milleruntime @ctubbsii