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

digest: add implementations of the AssociatedOid trait #1098

Merged
merged 12 commits into from Sep 16, 2022

Conversation

newpavlov
Copy link
Member

This PR adds implementation of the const_oid::AssociatedOid trait for the core wrappers. It allows to define OID for a core type and make it accessible for higher-level wrapped types.

Lack of these impls was mentioned in #1069 (comment)

@newpavlov
Copy link
Member Author

newpavlov commented Sep 7, 2022

During creation of this PR I found that, unfortunately, it will not work as-is. SHA-224 and SHA-256 share the same core, but have different OIDs. I don't think we can resolve this issue without introducing breaking changes.

Note that we can not write the following impl ot work around this issue:

impl AssociatedOid for CtVariableCoreWrapper<Sha256VarCore, U28> {
    const OID: ObjectIdentifier = ObjectIdentifier::new_unwrap("...");
}

digest/src/lib.rs Outdated Show resolved Hide resolved
@lumag
Copy link
Contributor

lumag commented Sep 7, 2022

@newpavlov I suppose that using newtype for the digests would allow defining AssociateOids. However this is definitely a bigger change.

@newpavlov
Copy link
Member Author

@lumag
Manual newtypes would mean a HUGE amount of boilerplate. The only practical solution is to introduce a bunch of macros to digest instead of the generic wrappers, but I would like to avoid such solution if possible.

Unfortunately, we hit some of Rust limitations (e.g. currently it's impossible to use ObjectIdentifier as a generic const argument) which prevent straightforward implementation of optional OIDs on non-core types, but I have some ideas about potential workarounds, which I would like to try.

@tarcieri
Copy link
Member

tarcieri commented Sep 7, 2022

See #1069 for some discussion of newtypes as a potential alternative (probably a better place to continue discussion than this particular PR)

Copy link
Member Author

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

I have added optional type parameter to CtVariableCoreWrapper which allows to pass OID for resulting hash function. Unfortunately, it means that we have to generate separate "carrier" types, but otherwise I think the workaround works quite well.

How addition of OIDs looks in implementation crates can be seen in RustCrypto/hashes#405.

@@ -68,4 +68,5 @@ jobs:
- run: cargo test --features dev
- run: cargo test --features alloc
- run: cargo test --features std
- run: cargo test --all-features
# the `oid` feature bumps MSRV to 1.57
# - run: cargo test --all-features
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder how can we handle it better.

Copy link
Member

Choose a reason for hiding this comment

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

You can split out a separate job for it with a comment to re-unify them when MSRV is bumped.

@newpavlov
Copy link
Member Author

Note that we still do not have a way to add AssociatedOid implementation for "compound" algorithms such as HMAC-SHA256, but I think it's better to discuss it in a separate issue.

@lumag
Copy link
Contributor

lumag commented Sep 9, 2022

@newpavlov you rock! I've used your implementation for the rsa crate and it works like a charm: RustCrypto/RSA#183

/// Wrapper around [`VariableOutputCore`] which selects output size
/// at compile time.
#[derive(Clone)]
pub struct CtVariableCoreWrapper<T, OutSize>
pub struct CtVariableCoreWrapper<T, OutSize, O = NoOid>
Copy link
Member

@tarcieri tarcieri Sep 13, 2022

Choose a reason for hiding this comment

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

Instead of using an extra generic type here, could CtVariableCoreWrapper have an impl of AssociatedOid which delegates to inner, and is bounded on inner having an impl of AssociatedOid?

That would avoid complicating the type signature and eliminate the need for NoOid.

Ditto for CoreWrapper.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand you correctly, then no, it will not work. See the SHA-224 vs SHA-256 issue raised in my earlier comment. They have the same core (i.e. the same inner type), so with such solution they can not have different OIDs.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, ok. That's really rather unfortunate.

Copy link
Member Author

@newpavlov newpavlov Sep 13, 2022

Choose a reason for hiding this comment

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

Ideally we would use const OID: Option<ObjectIdentifier> instead of the O parameter with AssociatedOid impl bounded on OID being Some, but, as you know, const generics are not powerful enough for that and will not be in the mid term future.

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Would be good to get this merged/release to make progress on RustCrypto/RSA#183

@newpavlov newpavlov marked this pull request as ready for review September 16, 2022 00:39
@newpavlov newpavlov merged commit f7968a1 into master Sep 16, 2022
@newpavlov newpavlov deleted the digest/const-oid-impl branch September 16, 2022 00:51
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

3 participants