-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add RFC 6979 support. #131
Conversation
Also, just FYI, I'm aware that there aren't any test vectors right now. I added some in Armory-specific code that can't be ported over as-is. If there's serious interest in merging this code, I'll be happy to add test vectors and to do whatever else is necessarily to bring the code up to snuff. :) |
Also see Issue 121. |
Also, note that, for now, this code is hard-coded to expect SHA-256. It'll need to be rewritten a bit in order to support other hash functions. |
Please also note, that this PR has a bad license. I have to strongly oppose this PR as long as this license stays to ensure license consistency in the library. |
Hi. Thanks for pointing out the license. To be honest, the licensing situation is a bit up in the air right now. Farhod (Armory's current maintainer) is going to have a talk tomorrow with Alan (previous maintainer) regarding some license issues. I'll see if it's okay to remove the license and move it over to the Boost license. |
It can't just be Boost license. Each file by itself has to be placed into public domain. |
Thanks for the update. I appreciate it. Update on my end: I still haven't heard anything regarding the meeting that happened. My suspicion is that the licensing won't be changed since I developed the patch for Armory. We did attempt to contact Wei at the time to see if we could get some paid advice from him and maybe get the patch upstreamed. We never got a reply. So, the patch went out as-is, with some unit tests. Anyway, I'll report back if I hear anything about changing the licensing. Otherwise, I see two options. 1)I write a "clean room" patch from scratch. (I'm glad I kept good notes that explain parts of the Crypto++ codebase!) If there's genuine interest in keeping the basic outline of the patch I submitted here, I'll do it. If the team wants to go in a different direction, as currently being discussed in issue 121, I'll consider but it may be best to hand the task off to someone with more familiarity with the codebase. 2)Drop the patch and close the PR. |
Yeah, Wei's been kind of busy. Folks like Denis and I stepped up to help move things forward, but its not like having Wei around.
This is probably the way to go. Once you get the first port under your belt, the ones that follow become easier. I'm guessing you can probably cut-in the core deterministic signature stuff in under 2 hours now.
Personally, I don't think this is a good idea at the moment. The PR offers something we don't have, so the PR is like a flag in the sand for similar folks. Plus, the major issue have been documented. JPM and Denis brought up the licensing, but it caught my eye earlier, too. To me, it was just a minor issue that needed to be worked around with mild care to avoid offending folks. Denis made this comment:
That's an odd requirement, and it helps to understand why source files are in public domain and the library as a whole is copyrighted and licensed. The requirements don't exist to complicate matters for contributors. Rather, it helps satisfy export regulations around cryptography in the US. As I understand it, it gives Wei the legal instruments (authority?) to comply with laws and regulations. For example, I believe Wei can use it to deny someone or an organization a license to the library. That person or organization may be a Restricted Party or on one of the US's Banned List (see, for example, Export Controlled or Embargoed Countries, Entities and Persons). |
Hello. I played with the code a bit today in my PR today and settled on a design. It's basically sticking to the pre-existing classes and using more templates. It's not ideal but it does seem to work. As a bonus, I don't have to create new files; I can just stick everything into pubkey.h. I'll submit a new version in a few days. I'm just going to rewrite everything from scratch to ensure that there are no legal issues with the original Armory code. Thanks. |
Very nice! Thank you, Douglas! |
Completely overhauled everything. Will squash the commits later. This code should be more palatable for everyone. I forgot to include DSA support, though. Let me know if that would be desirable. (I assume so.) |
DSA support would indeed be desirable! |
Okay. If it's not too much work (I really doubt it), I'll add it today. |
Added the DSA test vectors. Turns out no extra code was required to add DSA support. 6979 support should now be provably complete at this point. Run cryptest.exe v 71 to confirm this. |
Very nice! Thank you, Douglas! I'm not the best person to merge the pull request, since I have little experience in the platform considerations and steps involved in a Crypto++ release. I imagine the next step might be for someone more experienced, perhaps Jeffrey, to merge it? |
Thanks again for the hard work Douglas. So this caught my eye when running the changes under the
Crypto++ used to be notorious for crashing compilers. I have not seen a compiler crash in quite some time :) Notice its a debug build (
Based on the information in
And the following, which just shows its a fully patched Ubuntu 14 x64 server with GCC 4.8.4:
|
This is somewhat kind of odd. It shows up under
|
@noloader - Thanks for the crash report. I'll see if I can reproduce it under Ubuntu 15.10. If not, I'll spin up my 14.04 VM. As for the extra text you're seeing, that's in validat2.cpp. I just wanted to show the test suite passing for all the test vectors in RFC 6979 (Sects. A.2.3 through A.2.17), along with a few unofficial test vectors for the secp256k1 curve that's used by Bitcoin but not covered in RFC 6979. The test vectors are used by tools like Trezor, which handle real money/coins. If you'd prefer that I get rid of all the "passed" text and such, that's fine. I'll remove all that and anything else you want removed. |
@noloader - Couldn't reproduce the error in 14.04 or 15.10. Here's my fully updated 14.04 setup.
|
This is a bad sign... My second run did not reproduce it either. It sucks to be me.
No, we actually need those. We need lower case "passed" when things go right, and we need a capitol "FAILED" when things go wrong. In the case of testing, there are two kinds of test: _Validation_ ( _Validation_ Validation performs a quick validation of of the algorithms(s) involved. I think we want to see something like the following for
So in the case of _Test Vectors_ The test vectors are invoked with There's more to the test vectors. First, we will probably want them in The test vectors will probably use the Test Vectors also open another minor can of worms, and that's the changes to I'll handle this portion from scratch if that's OK with you. It will relieve you of it, and it will provide some independence which will help with assurances. A good example to study for the pieces that are added at this step is HKDF. It looks like I goofed and it was cut-in in pieces instead of one large commit, but see:
This is actually fine for the Test Vectors. The test vectors have a _Source_ field to indicate provenance, and it will say something like (from TestVectors/hkdf.txt):
And:
In the last case, Uri and I did the same thing as you are doing... We pulled in extra tests, we stated their provenance as generated by the library, and provided a comment that it mirros the official test vectors/test cases. |
Thanks. If you'd like to move the test vectors around, that's totally fine. I wasn't familiar with Crypto++'s test harness and just plopped down what I did where I did. As for not being able to reproduce the crash, did you upgrade your system? I saw that you were on 14.04.2. I'm on 14.04.4. I assumed that was the cause of the crash. If not, I have no immediate solutions offhand. Sorry! |
@@ -327,8 +327,8 @@ CRYPTOPP_DLL_TEMPLATE_CLASS DL_PrivateKeyImpl<DL_GroupParameters_EC<ECP> >; | |||
CRYPTOPP_DLL_TEMPLATE_CLASS DL_PrivateKeyImpl<DL_GroupParameters_EC<EC2N> >; | |||
CRYPTOPP_DLL_TEMPLATE_CLASS DL_PrivateKey_EC<ECP>; | |||
CRYPTOPP_DLL_TEMPLATE_CLASS DL_PrivateKey_EC<EC2N>; | |||
CRYPTOPP_DLL_TEMPLATE_CLASS DL_Algorithm_GDSA<ECP::Point>; | |||
CRYPTOPP_DLL_TEMPLATE_CLASS DL_Algorithm_GDSA<EC2N::Point>; | |||
CRYPTOPP_DLL_TEMPLATE_CLASS DL_Algorithm_GDSA<ECP::Point, SHA256, false>; |
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.
What's the purpose of this code? Had to put something here to get the code to compile.
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.
What's the purpose of this code?
The short of it: its used to manually instantiate a template for the shared object or DLL. For non-shared object and non-DLL, it does nothing.
Also see the discussion of CRYPTOPP_DLL_TEMPLATE_CLASS
at config.h on the Crypto++ wiki.
Also see Explicit Instantiation on MSDN and 7.5 Where's the Template? in the GCC manual.
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.
Thanks.
Refactored the code (and need to rebase, apparently). I temporarily disabled the test vector code. It needs to be refactored to work with the latest commit. |
Squashed the commits and attempted a rebase. Having some issues with pubkey.h. Git keeps wanting to claim that I've rewritten the entire file. Will look into some ways to undo this. May end up having to close this PR and open a new one. EDIT: Fixed pubkey.h. It's back to normal now. |
Pushed a bit of refactoring. Haven't squashed yet so that the changes can be properly viewed. Will squash later. |
@droark, I'm having a heck of a time with Git and merging this pull request due to changes in Could you checkout the latest Crypto++ sources, and then make a pull request based on them? |
@noloader - Sure. It may have to wait until early next week but I'll take care of this. |
Finally rebased! Will now go look into the other changes that were discussed. |
Adds RFC 6979 support for both DSA and ECDSA.
We are working this on DETSIG dev-branch at the moment. I think I forked then pulled the request. I'm going to close this PR since we effectively grabbed it already. |
Hi guys. I see that the ECDSA class has a 3rd template parameter to use a deterministic signature, but I can't find this in my local Crypto++ library. Did this get removed or something? I'm using Debian Buster, which is very new, with Crypto++ 5.6.4-8. This was "released" like 4 years ago. Can someone please explain how this works? Is there a specific version to be able to use deterministic signatures? |
Deterministic DSA signatures first appeared at Crypto++ 6.0. Also see |
Code was used by Armory and added back around Oct. 2014, with an eye towards attempting to get it added upstream. Code is tested on Linux and OS X. Windows files are untested. Feedback is welcome.