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

Do not destroy linked public keys #335

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Conversation

SalusaSecondus
Copy link
Contributor

ACCP private keys currently support destruction to make it easier to clean up sensitive data from memory. Currently the logic has three issues which need to be fixed:

  1. When both a private and a public key share a single native resource (as happens when ACCP generates them), destroying the private key also destroys the public key.
  2. Destroying a private key does not result in clearing the PKCS#8 encoded key.
  3. Attempting to use a destroyed key results in an IllegalStateException with a message references “Use after free.” This is misleading and might cause users to worry.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@SalusaSecondus SalusaSecondus requested a review from a team as a code owner September 27, 2023 16:31
geedo0
geedo0 previously approved these changes Oct 6, 2023
Comment on lines 231 to 233
if (isDestroyed()) {
throw new IllegalStateException("Already destroyed");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: You can reuse your assertNotDestroyed method here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Missed one. Thank you.

@amirhosv amirhosv merged commit c480e83 into corretto:main Oct 30, 2023
8 checks passed
@SalusaSecondus SalusaSecondus deleted the destroy branch October 31, 2023 17:12
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.

3 participants