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

Change HmacDrbg to support variable output size #243

Merged
merged 1 commit into from
Feb 12, 2021

Conversation

rvolgers
Copy link
Contributor

This is simply a matter of iterating the hashing of self.v until
sufficient output bytes have been produced.

This is necessary for the upcoming DSA implementation.

@rvolgers rvolgers force-pushed the drbg_variable_output branch from cd62770 to 4b22b57 Compare February 12, 2021 04:48
@codecov-io
Copy link

codecov-io commented Feb 12, 2021

Codecov Report

Merging #243 (8ab54d8) into master (e24ccec) will increase coverage by 0.63%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
+ Coverage   54.51%   55.14%   +0.63%     
==========================================
  Files           8        8              
  Lines         299      301       +2     
==========================================
+ Hits          163      166       +3     
+ Misses        136      135       -1     
Impacted Files Coverage Δ
ecdsa/src/rfc6979.rs 97.50% <100.00%> (+2.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e24ccec...8ab54d8. Read the comment docs.

ecdsa/src/rfc6979.rs Outdated Show resolved Hide resolved
This is simply a matter of iterating the hashing of self.v until
sufficient output bytes have been produced.

This is necessary for the upcoming DSA implementation.
@rvolgers rvolgers force-pushed the drbg_variable_output branch from 4b22b57 to 8ab54d8 Compare February 12, 2021 15:49
@rvolgers
Copy link
Contributor Author

@tarcieri By the way, the next step is splitting this to a separate crate. Would it be acceptable to keep it in the same repository though? It is not a signature algorithm obviously, but it's most easily tested as part of ecdsa / dsa.

@tarcieri
Copy link
Member

@rvolgers yeah that's fine. I registered the rfc6979 crate

@tarcieri tarcieri merged commit 71c94ee into RustCrypto:master Feb 12, 2021
@rvolgers
Copy link
Contributor Author

RFC6979 is about DSA / ECDSA. Do you propose to move the k generation into this crate as well, then? That probably means adding separate features for DSA and ECDSA.

@rvolgers
Copy link
Contributor Author

Actually, that seems fine. It sidesteps a number of other complicating factors.

@tarcieri
Copy link
Member

I think everything in the current ecdsa::rfc6979 module can be extracted, including the rejection sampling. However, the ecdsa crate should depend on it, which means elliptic curve-specific types like NonZeroScalar and FieldBytes should remain in the ecdsa crate.

@rvolgers
Copy link
Contributor Author

@tarcieri I'm running into some trouble with the FromDigest trait and stuff from dev.rs. I don't fully understand why this exists at all, it seems like a lot of ad-hoc code for what should be a generic "from bytes" operation. But I guess I'm missing something. Should I just move all that stuff to the new crate?

@tarcieri
Copy link
Member

I'm running into some trouble with the FromDigest trait and stuff from dev.rs. I don't fully understand why this exists at all, it seems like a lot of ad-hoc code for what should be a generic "from bytes" operation.

Can you be more specific? Are you talking about this line?

    let h1 = Scalar::<C>::from_digest(msg_digest).to_repr();

If so, it corresponds directly to this pseudocode from RFC6979 Section 2.4:

       H(m) is transformed into an integer modulo q using the bits2int
       transform and an extra modular reduction:

          h = bits2int(H(m)) mod q

       As was noted in the description of bits2octets, the extra modular
       reduction is no more than a conditional subtraction.

The modular reduction should happen in the ecdsa and dsa crates, IMO. The rfc6979 crate can accept a mod-reduced input, and document that as part of the contract of the function.

This needs to happen anywhere you see anything specific to DSA or ECDSA: the rfc6979 crate can only implement the functionality which is non-specific to a particular algorithm.

@rvolgers
Copy link
Contributor Author

I'm talking about refactoring trouble, as in, I move stuff and the compiler yells at me. This is boring work and I'm trying to limit the amount of time I'm spending to figure out random implementation details I don't care about, of which there are a lot (cargo features, large amount of generics etc).

In particular, the testcase has a dependency on the FromDigest impl for MockCurve, and I can't import that as it creates a circular dependency. So either I have to move that to the new crate also, or change the test. I didn't fully understand what it's for, especially since it seems to only be implemented for MockCurve, unless other implementations live elsewhere, in which case, the trait should probably also live there and not in ecdsa or the new crate?

@rvolgers
Copy link
Contributor Author

I'll put up a draft PR soon, it's probably no use talking about it like this.

@tarcieri
Copy link
Member

Yeah, pushing up some actual code would help.

In particular, the testcase has a dependency on the FromDigest impl for MockCurve, and I can't import that as it creates a circular dependency. So either I have to move that to the new crate also, or change the test. I didn't fully understand what it's for, especially since it seems to only be implemented for MockCurve, unless other implementations live elsewhere, in which case, the trait should probably also live there and not in ecdsa or the new crate?

Unlike DSA, ECDSA needs to support multiple elliptic curves, hence the generics. MockCurve is used for testing algorithms which are implemented generically over elliptic curves. The other impls of FromDigest live within the respective elliptic curve crates which provide concrete implementations.

The FromDigest trait describes an ECDSA-specific algorithm as defined in ANSI X9.62-2005 and absolutely should remain in the ecdsa crate. It could perhaps be better documented in this regard.

In porting over the existing tests, you should operate on the raw output of bits2int(H(m)) mod q, as everything before that is an ECDSA-specific concern.

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.

3 participants