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

Implement Arbitrary for Certificate #761

Merged
merged 2 commits into from
Dec 22, 2022
Merged

Implement Arbitrary for Certificate #761

merged 2 commits into from
Dec 22, 2022

Conversation

stormshield-guillaumed
Copy link
Contributor

Add an optional dependency on the arbitrary crate to implement Arbitrary for Certificate. This allows to generate arbitrary certificates for fuzzing.

@tarcieri
Copy link
Member

As a gut reaction, this is a surprisingly invasive change

const-oid/src/lib.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

@stormshield-guillaumed you can bump the clippy rustc version, although it might need some code changes

@stormshield-guillaumed
Copy link
Contributor Author

After a quick check, the arbitrary feature doesn't seem to be included with cargo hack test --feature-powerset. It seems to be because of the cargo-hack version used not supporting dep: features. They started supporting it with version 0.5.15. I think more CI checks will be failing because I didn't pay attention, but the MSRV of arbitrary is 1.63.0, so it won't build with 1.60.0.

Also, I saw that the rust-version of the x509-cert crate is 1.56.0 but the CI runs on 1.60.0.

@stormshield-guillaumed
Copy link
Contributor Author

I checked and arbitrary really requires 1.63.0 because they use array::from_fn.

@tarcieri
Copy link
Member

FWIW we're looking at bumping MSRV to 1.65 in #797

@tarcieri
Copy link
Member

This bumps the cargo-hack version to v0.5.24: RustCrypto/actions#22

@@ -23,6 +23,7 @@ use crate::asn1::ObjectIdentifier;
/// Nevertheless, this crate defines an `ANY` type as it remains a familiar
/// and useful concept which is still extensively used in things like
/// PKI-related RFCs.
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
#[cfg_attr(
feature = "arbitrary",
derive(arbitrary::Arbitrary),
allow(clippy::integer_arithmetic)
)]

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 tried this and a few other places for the allow but none suppresses the warning. I don't know how to put attributes on derive generated code and adding the allow to the module seems a bit extreme (suppressing more than wanted).

Copy link
Member

Choose a reason for hiding this comment

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

I guess the issue is it's annotating the struct and not the associated impl blocks.

You can add it at to the module if it's gated on feature = "arbitrary"

@@ -202,6 +203,7 @@ impl<'a> TryFrom<&'a [u8]> for AnyRef<'a> {
/// backing data.
#[cfg(feature = "alloc")]
#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
#[cfg_attr(
feature = "arbitrary",
derive(arbitrary::Arbitrary),
allow(clippy::integer_arithmetic)
)]

@stormshield-guillaumed
Copy link
Contributor Author

The only remaining errors are on the minimal version check, but I already specify arbitrary = "1.2.0", so I don't know what I can do to solve this.

@tarcieri
Copy link
Member

It looks like a bug in derive_arbitrary v1.1.6:

error[E0599]: no method named `len` found for reference `&Fields` in the current scope
   --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/derive_arbitrary-1.1.6/src/lib.rs:219:30
    |
219 |         if idx + 1 == fields.len() {
    |                              ^^^ method not found in `&Fields`

This needs to be fixed on the arbitrary side. A couple of potential fixes:

  • Yank the 1.1.6 release
  • Change arbitrary to depend on derive_arbitrary ^1.2

@tarcieri
Copy link
Member

I opened an issue on arbitrary: rust-fuzz/arbitrary#134

If you'd like to get the build passing now you can temporarily disable the check in the crates that are failing, although I'd ask you do that in a separate PR/commit so it's easy to revert when the issue does get fixed.

@tarcieri
Copy link
Member

@stormshield-guillaumed can you either PR b4ac2a6 separately or squash all of the commits down to two, one which adds the Arbitrary impl, and the other with the contents of b4ac2a6 so it can be easily reverted?

@tarcieri tarcieri merged commit 6082a98 into RustCrypto:master Dec 22, 2022
@tarcieri
Copy link
Member

Thanks!

tarcieri added a commit that referenced this pull request Feb 26, 2023
Backport release which includes changes from #761, which add optional
support for the `arbitrary` crate as a feature
@tarcieri tarcieri mentioned this pull request Feb 26, 2023
@tarcieri tarcieri mentioned this pull request Mar 19, 2023
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.

None yet

2 participants