Implement secure_random() #727

lovvik · 2024-06-05T04:12:31Z

as asked by issue #683

Implemented secure_random()
Added 2 build directories
Changed calls of fast_random() in src, tests and benchmarks
Added -lcrypto linking

adrianopol · 2024-06-06T21:50:58Z

3rdparty/SConscript


-    if not conf.CheckLibWithHeaderExt('ssl', 'openssl/rand.h', 'C', run=not is_crosscompiling):
+    if not conf.CheckLibWithHeaderExt('ssl', 'openssl/rand.h', 'C', run=not is_crosscompiling) or not conf.CheckLibWithHeaderExt('crypto', 'openssl/crypto.h', 'C', run=not is_crosscompiling):


hello!
it's enough to validate the presence of openssl/rand.h here; validating every single file's presence we use in the project is too redundant

i don't really get why, but this how i could get -lcrypto to be linked.
This, on the other hand results in linkage errors :

conf.env.AddManualDependency(libs=['ssl','crypto'], prefix=ossl_prefix) if not conf.CheckLibWithHeaderExt('ssl', 'openssl/rand.h', 'C', run=not is_crosscompiling): #or not conf.CheckLibWithHeaderExt('crypto', 'openssl/crypto.h', 'C', run=not is_crosscompiling): env.Die("OpenSSL not found (see 'config.log' for details)") ```

i don't really get why, but this how i could get -lcrypto to be linked.

Ah, sorry, you are checking a different library here. Then it's OK.

You can also make shorten the condition:

if not conf.CheckLibWithHeaderExt(['ssl', 'crypto'], ['openssl/rand.h', 'openssl/crypto.h'], 'C', run=not is_crosscompiling):

Seems we have AddPkgConfigDependency('libssl'), but don't have AddPkgConfigDependency('libcrypto'), it should be added, because they're different packages in pkg-config:

pkg-config --cflags --libs libssl -lssl pkg-config --cflags --libs libcrypto -lcrypto

And in this case it indeed makes sense to add a second check. openssl/rand.h checks that libssl worked, and openssl/crypto.h checks that lubcrypto worked.

i don't really get why, but this how i could get -lcrypto to be linked.

Likely CheckLibWithHeaderExt adds it, but we should no rely on it. Actually it's probably a bug that should be fixed (not in this PR of course).

Also please avoid long lines.

Hello, i now have this. The AddPkgConfigDependency('libcrypto') does it (even without CheckLib...) . is it ok ?

elif 'openssl' in system_dependencies: ossl_prefix = None # macOS: fallback to use a brew-installed openssl if meta.platform == 'darwin' and not is_crosscompiling: ossl_prefix = env.FindBrewPackage('openssl') conf = Configure(env, custom_tests=env.CustomTests) if not conf.AddPkgConfigDependency('libssl', '--cflags --libs', add_prefix=ossl_prefix): conf.env.AddManualDependency(libs=['ssl'], prefix=ossl_prefix) if not conf.AddPkgConfigDependency('libcrypto', '--cflags --libs', add_prefix=ossl_prefix): conf.env.AddManualDependency(libs=['crypto'], prefix=ossl_prefix) if not conf.CheckLibWithHeaderExt('ssl', 'openssl/rand.h', 'C', run=not is_crosscompiling) or \ not conf.CheckLibWithHeaderExt('crypto', 'openssl/crypto.h', 'C', run=not is_crosscompiling): env.Die("OpenSSL not found (see 'config.log' for details)") env = conf.Finish()

adrianopol · 2024-06-06T21:52:15Z

src/internal_modules/roc_core/target_nocsprng/roc_core/secure_random.cpp

it was not intended to remove the original fast_random version, so please just keep it

adrianopol · 2024-06-06T21:53:09Z

src/internal_modules/roc_core/target_nocsprng/roc_core/secure_random.cpp

@@ -17,7 +17,6 @@ namespace core {
 namespace {

 uint32_t rng_state;
-


adrianopol · 2024-06-06T22:05:05Z

src/internal_modules/roc_core/target_nocsprng/roc_core/secure_random.cpp

+// Wrappers
+// To use for external calls
+
+uint32_t secure_random() {
+    return fast_random();
+};
+
+uint32_t secure_random_range(uint32_t from, uint32_t to) {
+    return fast_random_range(from, to);
+};
+
+double secure_random_gaussian() {
+    return fast_random_gaussian();
+};
+


this should be implemented in target_nocsprng (as written in #683)

Hello, i don't understand. That is target_nocsprng.

I mean, the fast_random implementation should be the only in this file, and secure_random() function implemented through calls to fast_random*() is the only one to go to the target_nocsprng directory.

adrianopol · 2024-06-06T22:10:31Z

src/internal_modules/roc_core/target_nocsprng/roc_core/secure_random.h

-//! @file roc_core/fast_random.h
+//! @file roc_core/target_nocsprng/roc_core/secure_random.h


adrianopol · 2024-06-06T22:45:24Z

src/tests/roc_core/test_secure_random.cpp

the original file should be kept

this was asked by gavv : #683 (comment)

adrianopol · 2024-06-06T22:46:30Z

src/tests/roc_ctl/bench_task_queue_contention.cpp

this should not be changed

adrianopol · 2024-06-06T22:47:06Z

src/tests/roc_fec/test_encoder_decoder.cpp

this should not be changed

adrianopol · 2024-06-06T22:47:56Z

src/tests/roc_netio/test_helpers/conn_reader.h

this should not be changed

adrianopol · 2024-06-06T22:48:21Z

src/tests/roc_netio/test_helpers/conn_writer.h

this should not be changed; checkout the task description and revert other places please

gavv · 2024-06-07T09:43:52Z

Thanks for PR!

Regarding my comment from matrix that you linked above: #683 (comment)

I guess I was not very clear.

The idea is:

fast_random is always available and has a single cross-platform implementation; it's not affected by this task at all
secure_random is always available
but secure_random can have two different implementations: using OpenSSL, and fallback using fast_random (which as you remember is always available)
some parts of the code use fast_random, some use secure_random, depending on requirements; CSPRNG is needed when RTP RFC mandates it; but it's slower and can fail, so it's not used everywhere
regarding tests:
- we need tests for fast_random and for secure_random; they're different tests at least because these functions have different signatures
- tests for fast_random should be be affected by this task at all
- tests for secure_random can have only one implementation, no matter if secure_random is implemented via OpenSSL or fast_random

That's what I tried to say in that comment, we don't need two separate implementations of tests for secure_random, however we still need separate tests for fast_random and secure_random.

lovvik · 2024-06-07T10:37:01Z

oh...i really misinterpreted it all. It will be fixed.

github-actions · 2024-06-17T10:28:22Z

🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts.

adrianopol requested changes Jun 6, 2024

View reviewed changes

gavv added needs revision Pull request should be revised by its author contribution A pull-request by someone else except maintainers labels Jun 7, 2024

gavv changed the title ~~Solving issue #683~~ Implement secure_random() Jun 7, 2024

github-actions bot added the needs rebase Pull request has conflicts and should be rebased label Jun 17, 2024

lovvik closed this Jun 25, 2024

lovvik force-pushed the develop branch from 1e7e956 to a518843 Compare June 25, 2024 22:42

github-actions bot removed needs revision Pull request should be revised by its author needs rebase Pull request has conflicts and should be rebased labels Jun 25, 2024

gavv added the postponed We decided to postpone the issue for an indefinite period label Jul 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement secure_random() #727

Implement secure_random() #727

lovvik commented Jun 5, 2024 •

edited

Loading

adrianopol Jun 6, 2024

lovvik Jun 7, 2024 •

edited

Loading

adrianopol Jun 7, 2024

gavv Jun 7, 2024

lovvik Jun 7, 2024 •

edited

Loading

gavv Jun 20, 2024

adrianopol Jun 6, 2024

adrianopol Jun 6, 2024

adrianopol Jun 6, 2024

lovvik Jun 7, 2024

adrianopol Jun 7, 2024

adrianopol Jun 6, 2024

adrianopol Jun 6, 2024

lovvik Jun 7, 2024 •

edited

Loading

adrianopol Jun 6, 2024

adrianopol Jun 6, 2024

adrianopol Jun 6, 2024

adrianopol Jun 6, 2024

gavv commented Jun 7, 2024

lovvik commented Jun 7, 2024

github-actions bot commented Jun 17, 2024


		if not conf.CheckLibWithHeaderExt('ssl', 'openssl/rand.h', 'C', run=not is_crosscompiling):
		if not conf.CheckLibWithHeaderExt('ssl', 'openssl/rand.h', 'C', run=not is_crosscompiling) or not conf.CheckLibWithHeaderExt('crypto', 'openssl/crypto.h', 'C', run=not is_crosscompiling):

		@@ -17,7 +17,6 @@ namespace core {
		namespace {

		uint32_t rng_state;

		//! @file roc_core/fast_random.h
		//! @file roc_core/target_nocsprng/roc_core/secure_random.h

Implement secure_random() #727

Implement secure_random() #727

Conversation

lovvik commented Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

lovvik Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovvik Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovvik Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavv commented Jun 7, 2024

lovvik commented Jun 7, 2024

github-actions bot commented Jun 17, 2024

lovvik commented Jun 5, 2024 •

edited

Loading

lovvik Jun 7, 2024 •

edited

Loading

lovvik Jun 7, 2024 •

edited

Loading

lovvik Jun 7, 2024 •

edited

Loading