-
Notifications
You must be signed in to change notification settings - Fork 216
digest: add implementations of the AssociatedOid trait #1098
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
Conversation
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("...");
} |
@newpavlov I suppose that using newtype for the digests would allow defining |
@lumag Unfortunately, we hit some of Rust limitations (e.g. currently it's impossible to use |
See #1069 for some discussion of newtypes as a potential alternative (probably a better place to continue discussion than this particular PR) |
There was a problem hiding this 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.
.github/workflows/digest.yml
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Note that we still do not have a way to add |
@newpavlov you rock! I've used your implementation for the |
/// Wrapper around [`VariableOutputCore`] which selects output size | ||
/// at compile time. | ||
#[derive(Clone)] | ||
pub struct CtVariableCoreWrapper<T, OutSize> | ||
pub struct CtVariableCoreWrapper<T, OutSize, O = NoOid> |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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
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)