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

Entropy pool implementation #1090

Closed
wants to merge 17 commits into from

Conversation

torben-hansen
Copy link
Contributor

@torben-hansen torben-hansen commented Jul 11, 2023

Issues:

CryptoAlg-1952

Description of changes:

Implements the backend for passive entropy. The integration as the actual entropy backend for RAND_bytes (for fips build), will occur in the next PR.

The entropy pool has one main action: RAND_entropy_pool_get. This function can be used to fetch entropy from the pool. If the pool is depleted, the "depleted workflow" is activated, that workflow requests a need for more entropy outside the module. New entropy is loaded into the module through RAND_load_entropy (the entropy sourcing part is not implemented yet).

Call-outs:

As noted in the source code:

	// Out-side module call
	// Could create a soft-lock in the struct here
	RAND_module_entropy_depleted();
	// And then unlock struct here

I could soft-lock the pool during the depleted workflow. This shouldn't really be necessary since the every operation is serialised in the thread.

Testing:

Added a bunch of tests. Can add more if needed.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@torben-hansen torben-hansen marked this pull request as ready for review July 12, 2023 04:29
// entropy_pool_ensure_can_satisfy returns 1 if the entropy pool |entropy_pool|
// contains enough entropy to satisfy a get request of size |get_size|.
// Returns 0 otherwise.
static int entropy_pool_ensure_can_satisfy(struct entropy_pool *entropy_pool,
Copy link
Member

Choose a reason for hiding this comment

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

Should this abort if entropy_pool == NULL?. I know you gate some of the functions already, but not all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to only validate the pointer in "exported" functions, not the static ones.

Also changed such that the entropy pool doesn't abort. Callers can do that if they wish. But not at this layer.

crypto/fipsmodule/rand/entropy_pool.c Show resolved Hide resolved
crypto/fipsmodule/rand/internal.h Show resolved Hide resolved
crypto/fipsmodule/rand/entropy_pool.c Outdated Show resolved Hide resolved
return 1;
}

static void entropy_pool_cannot_satisfy_request(void) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: to me entropy_pool_cannot_satisfy_request seems like it was getting ready to abort, it's not clear what it does.

Suggested change
static void entropy_pool_cannot_satisfy_request(void) {
static void request_external_entropy(void) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if not entropy_pool_ensure_can_satisfy() then entropy_pool_cannot_satisfy_request() seems like the logically named next action.

Just tried to separate logic. Maybe it was too "smart". Just inlined the code instead.

crypto/fipsmodule/rand/entropy_pool.c Show resolved Hide resolved
crypto/fipsmodule/rand/entropy_pool_test.cc Outdated Show resolved Hide resolved
crypto/fipsmodule/rand/entropy_pool_test.cc Show resolved Hide resolved
crypto/fipsmodule/rand/entropy_pool_test.cc Show resolved Hide resolved
Copy link
Contributor

@dkostic dkostic left a comment

Choose a reason for hiding this comment

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

approved accidentally, taking it back for a moment :)

@torben-hansen
Copy link
Contributor Author

Given #1112, this implementation is no longer needed. Closing.

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.

4 participants