feature-gate tests that use generate

[vlmutolo]

Description
Tests currently fail with cargo test --no-default-features.

Additional information
It seems like this is happening because lots of the tests generate a random key. We could probably just feature-gate all those tests behind getrandom or maybe safe_api. Thoughts?

• Orion features selected: default-features = false
• Orion version: 723d9ef11c2b4e5f2f7746ffa9392c7cea5d8dbf
• Rust version: $ rustc -V : rustc 1.56.0 (09c42c458 2021-10-18)

We should probably also make sure the CI is testing no "no enabled features" case.

[brycx]

All the tests calling safe_api-enabled APIs should already be gated behind safe_api.

I think the issue here is, calling cargo test --no-default-features, also runs the doctest, which implicitly require safe_api.

cargo test --tests --no-default-features should work fine IIRC. There should also be CI test for this such as:

orion/.github/workflows/test.yml

Line 80 in cfa2c0c

- name: Test release-mode, no default features

Or did you mean other tests?

[brycx]

If we can get doctests working without default features, by gating those that e.g. generate keys, that would be very useful. But I suspect most doctests make use of safe_api functionality, since the examples focus on simplicity and security.

Some, like hashing, could be in scope though.

[brycx]

@vlmutolo I'll remove the bug-label and add the improvement-label instead. Let me know if I misunderstood something here.

[vlmutolo]

I think the issue here is, calling cargo test --no-default-features, also runs the doctest, which implicitly require safe_api.

Yes, good point. I hadn't realized that it was only the doctests failing.

I'll remove the bug-label and add the improvement-label instead

So I guess it just depends on what "bug" means. It's certainly not as severe as a runtime bug or security issue. But I'd probably consider doctests failing under esoteric-but-valid combinations of features to be a (minor) bug.

[brycx]

So I guess it just depends on what "bug" means. It's certainly not as severe as a runtime bug or security issue. But I'd probably consider doctests failing under esoteric-but-valid combinations of features to be a (minor) bug.

I understand where you're coming from. I don't think it's the best state-of-affairs either. My initial thoughts on how this could be approached, is simply feature-gating the actual doc-tests that use safe_api-functionality, behind that flag (example below for XChaCha20-Poly1305):

//! # #[cfg(feature = "safe_api")] { // ADDED HERE
//! use orion::hazardous::aead;
//!
//! let secret_key = aead::xchacha20poly1305::SecretKey::generate();
//! let nonce = aead::xchacha20poly1305::Nonce::generate();
//! let ad = "Additional data".as_bytes();
//! let message = "Data to protect".as_bytes();
//!
//! // Length of the above message is 15 and then we accommodate 16 for the Poly1305
//! // tag.
//!
//! let mut dst_out_ct = [0u8; 15 + 16];
//! let mut dst_out_pt = [0u8; 15];
//! // Encrypt and place ciphertext + tag in dst_out_ct
//! aead::xchacha20poly1305::seal(&secret_key, &nonce, message, Some(&ad), &mut dst_out_ct)?;
//! // Verify tag, if correct then decrypt and place message in dst_out_pt
//! aead::xchacha20poly1305::open(&secret_key, &nonce, &dst_out_ct, Some(&ad), &mut dst_out_pt)?;
//!
//! assert_eq!(dst_out_pt.as_ref(), message.as_ref());
//! # Ok::<(), orion::errors::UnknownCryptoError>(()) } // ADDED HERE

With this, we still get the documentation generated even when generating with cargo doc --no-deps --no-default-features, but the tests also pass with cargo test --no-default-features, because then the doc-tests for XChaCha20-Poly1305 aren't run at all.

However, if a user compiles docs with the above example, then runs tests, they might expect to be able to use e.g. generate() in no_std even though they can't, since the tests pass. OTOH, this seems like a frail counter-argument: Our documentation gives the doc-compilation commands for all features, the docs themselves explain that generate() will not be available in no_std and we even have the newer feature-flag annotations on docs.rs to communicate the same.

So what do you think about feature-gating those tests failing for --no-default-features @vlmutolo? We could add a test to the current CI doc-test workflow to ensure this is upheld in the future as well. We should still be careful to only gate those that need it though. We still want as many tests as possible to run even in no_std.

[vlmutolo]

This is very interesting. I don't think I've seen a conditional compilation macro applied this way before. I think this is a good solution.

However, if a user compiles docs with the above example, then runs tests, they might expect to be able to use e.g. generate() in no_std even though they can't, since the tests pass.

I'm not too worried about that scenario. It's still a compile-time error to try to use generate when it's not available, and the docs are effective at communicating when you need it, like you said.

---

I'll just throw out one other solution for consideration. We could expose the feature-gating in the doctest itself. Maybe something like the following:

#[cfg(feature = "safe_api")]
let secret_key = aead::xchacha20poly1305::SecretKey::generate();

#[cfg(not(feature = "safe_api"))]
let secret_key = aead::xchacha20poly1305::SecretKey::from(b"do_it_yourself");

// Etc

On one hand, this is pretty clear about what features need to be used and it also should pass tests without default features.

On the other hand, it kind of clutters the doctest and distracts from the main example for each of the tests.

[brycx]

I'm not too worried about that scenario. It's still a compile-time error to try to use generate when it's not available, and the docs are effective at communicating when you need it, like you said.

Makes sense.

On the other hand, it kind of clutters the doctest and distracts from the main example for each of the tests.

I thought about this approach as well, but opted for the other one because I thought plastering the examples would make them unreadable. I tihnk it's worth prioritizing as simple as possible and secure examples. Because of that, I'm leaning more towards my initial suggestion, unless you have other concerns.

[vlmutolo]

That sounds like a good plan to me. Keeping the documentation easy to understand is critical.

[brycx]

A tentative list of modules that use safe-api functionality in the doc examples, where the modules themselves are not already feature-gated by safe-api:

• orion::hazardous::aead::chacha20poly1305
• orion::hazardous::aead::streaming
• orion::hazardous::aead::xchacha20poly1305
• orion::hazardous::ecc::x25519
• orion::hazardous::ecc::x25519::PrivateKey
• orion::hazardous::ecc::x25519::SharedKey
• orion::hazardous::kdf::argon2i
• orion::hazardous::kdf::hkdf
• orion::hazardous::kdf::pbkdf2
• orion::hazardous::kdf::pbkdf2::sha256::Password
• orion::hazardous::kdf::pbkdf2::sha384::Password
• orion::hazardous::kdf::pbkdf2::sha512::Password
• orion::hazardous::mac::blake2b
• orion::mac::blake2b::SecretKey
• orion::hazardous::mac::hmac
• orion::hazardous::mac::hmac::sha256::SecretKey
• orion::hazardous::mac::hmac::sha384::SecretKey
• orion::hazardous::mac::hmac::sha512::SecretKey
• orion::hazardous::mac::poly1305
• orion::hazardous::mac::poly1305::OneTimeKey
• orion::hazardous::stream::chacha20
• orion::hazardous::stream::chacha20::SecretKey
• orion::hazardous::stream::xchacha20
• orion::util::secure_cmp

Added to scope of next minor release.