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

XMSS RFC 8391 Update #1858

Merged
merged 8 commits into from
May 24, 2019
Merged

XMSS RFC 8391 Update #1858

merged 8 commits into from
May 24, 2019

Conversation

mgierlings
Copy link
Collaborator

Updates XMSS (draft status) to conform to RFC 8391

XMSS_SHAKE256_W16_H16 = 0x0b00000b,
XMSS_SHAKE256_W16_H20 = 0x0c00000c
XMSS_SHA2_10_256 = 0x00000001,
XMSS_SHA2_16_256 = 0x00000002,
Copy link
Owner

@randombit randombit Mar 15, 2019

Choose a reason for hiding this comment

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

So the format changed between draft-06 and RFC? :/

Is there any reasonable way we can continue supporting older keys/sigs? It seems this change would invalidate all existing uses.

At the very least we'll also want to use a different OID for keys so it is not possible to mix them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, unfortunately the OID format did change. There exists another, new draft status document of interest regarding XMSS Algorithm Identifiers for HSS and XMSS for Use in the Internet X.509 Public Key Infrastructure. Also somewhere down the road XMSS will likely be assigned an "official" OID. So this may not be the last incompatible change to XMSS we see. There is also a disclaimer in RFC 8391:

This document is not an Internet Standards Track specification; it is
published for informational purposes.

This document is a product of the Internet Research Task Force
(IRTF). The IRTF publishes the results of Internet-related research
and development activities. These results might not be suitable for
deployment. This RFC represents the consensus of the Crypto Forum
Research Group of the Internet Research Task Force (IRTF). Documents
approved for publication by the IRSG are not candidates for any level
of Internet Standard; see Section 2 of RFC 7841.

Information about the current status of this document, any errata,
and how to provide feedback on it may be obtained at
https://www.rfc-editor.org/info/rfc8391.

A possibility to support older formats would be to integrate a key conversion utility into the botan cli.

randombit added a commit that referenced this pull request Mar 15, 2019
@randombit
Copy link
Owner

I sent out a warning letting anyone who is actively using current draft-06 support to let us know https://lists.randombit.net/pipermail/botan-devel/2019-March/002278.html and added a warning to the 2.10 release notes. If we don't hear from anyone by say May 1st, I say go ahead and merge this.

In the mean time we should still work to minimize disturbance:

  • Increase XMSS datestamp in info.txt
  • Change to a new OID arc (update also doc/oids.txt)
  • Maybe even change BOTAN_HAS_XMSS to BOTAN_HAS_XMSS_RFC8391?

@randombit
Copy link
Owner

Haven't heard from anyone about this so likely this is good to merge, but lets wait until end of the month just to be sure.

@randombit randombit mentioned this pull request Apr 7, 2019
18 tasks
@randombit
Copy link
Owner

@mgierlings Can you rebase to address the merge conflict?

@mgierlings
Copy link
Collaborator Author

@randombit Will do. I also have another update to this PR in the pipeline, which I'll push soon.

@randombit
Copy link
Owner

It looks like you merged master into your branch rather than rebasing, this makes history very hard to read. Please rebase instead.

mgierlings added 6 commits May 6, 2019 10:00
Changes XMSS and XMSS WOTS algorithm names and OIDs to correspond
to RFC 8391.
- Replaces XMSS test vectors with new vectors that were
  generated using Bouncy Castle's XMSS implementation.
- Adjusts the XMSS test bench to recognize the new XMSS
  algorithm naming scheme.
Internally XMSS uses a 64 Bit type for the leaf index.
This patch removes the four leading zero bytes from the
XMSS leaf index and serializes it as a four byte value
as described in RFC 8391.

Test cases are adjusted accordingly.

The 64 Bit type is kept internally which potentially
allows for code reuse when implementing XMSS^MT on top
of the current XMSS code.
@mgierlings
Copy link
Collaborator Author

@randombit The broken commit should be fixed. I'm not sure though what causes the softhsm error in some of the Travis CI builds.

@randombit
Copy link
Owner

@mgierlings Thanks. I think the problem is bench_xmss in speed.cpp needs to be updated for the new param names.

@randombit
Copy link
Owner

I don't think any change in performance was expected either way, but ftr I did check master vs this branch, they had only insignificant differences in sign and verify time.

@codecov-io
Copy link

codecov-io commented May 10, 2019

Codecov Report

Merging #1858 into master will decrease coverage by <.01%.
The diff coverage is 69.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1858      +/-   ##
==========================================
- Coverage   91.38%   91.37%   -0.01%     
==========================================
  Files         531      532       +1     
  Lines       56462    56556      +94     
  Branches     5877     5882       +5     
==========================================
+ Hits        51596    51677      +81     
- Misses       4866     4879      +13
Impacted Files Coverage Δ
src/lib/pubkey/xmss/xmss_publickey.cpp 94.11% <ø> (ø) ⬆️
src/lib/pubkey/xmss/xmss_signature_operation.cpp 100% <ø> (ø) ⬆️
src/lib/pubkey/xmss/xmss_wots_publickey.cpp 100% <ø> (ø) ⬆️
src/cli/speed.cpp 90.75% <ø> (ø) ⬆️
src/lib/pubkey/xmss/xmss_signature.cpp 100% <100%> (ø) ⬆️
src/lib/pubkey/pk_algs.cpp 94.81% <100%> (ø) ⬆️
src/lib/pubkey/xmss/xmss_privatekey.cpp 95.74% <100%> (ø) ⬆️
src/lib/pubkey/xmss/xmss_wots_parameters.cpp 86.11% <50%> (ø) ⬆️
src/tests/test_xmss.cpp 93.02% <50%> (ø) ⬆️
src/lib/pubkey/xmss/xmss_parameters.cpp 90% <69.44%> (ø) ⬆️
... and 16 more

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 2c9caac...e390adc. Read the comment docs.

@randombit
Copy link
Owner

@mgierlings I fixed the test issue in 10630cc. Is this PR ready to merge?

@mgierlings
Copy link
Collaborator Author

@randombit Looks good to me! One remark though on the XMSS parameter name changes introduced with 10630cc: The RFC actually uses "-" between XMSS and the hash function name, see RFC 8391 Section 5.3 XMSS Parameters. By replacing the dash with an underscore we technically use non standard compliant algorithm names.

@securitykernel
Copy link
Collaborator

@mgierlings BouncyCastle uses the same naming scheme, maybe this is a thing? Anyway, whatever we decide here, we should probably make it consistent in both libraries.

@mgierlings
Copy link
Collaborator Author

@securitykernel Agreed, we should be consistent. If others start implementing XMSS, they will likely use names from the standard document. The cleanest solution would probably be to adhere to the RFC's naming scheme (in both BouncyCastle and Botan). If we come up with our own now we might need to revisit this and change naming again in the future.

@randombit
Copy link
Owner

I renamed the params because that's what the patched handbook uses for the param names. I find combining hyphen and underscore really awkward. But if that is the standard naming scheme, probably best to follow it.

@mgierlings
Copy link
Collaborator Author

mgierlings commented May 12, 2019

@randombit The algorithm names in the handbook are supposed to correspond to the xmss_algorithm_t enum items, since this is what is passed as argument in the code snippet below. However I did omit the bulky prefix Botan::XMSS_Parameters::xmss_algorithm_t, as a result the algorithm list in the handbook is somewhat ambiguous. To be more clear, we could change the handbook to list the available algorithms as follows:

Botan::XMSS_Parameters::xmss_algorithm_t::XMSS_SHA2_10_256 ("XMSS-SHA2_10_256")
Botan::XMSS_Parameters::xmss_algorithm_t::XMSS_SHA2_16_256 ("XMSS-SHA2_16_256")
Botan::XMSS_Parameters::xmss_algorithm_t::XMSS_SHA2_20_256 ("XMSS-SHA2_20_256")
...

or

"XMSS-SHA2_10_256" (Botan::XMSS_Parameters::xmss_algorithm_t::XMSS_SHA2_10_256)
"XMSS-SHA2_16_256" (Botan::XMSS_Parameters::xmss_algorithm_t::XMSS_SHA2_16_256)
"XMSS-SHA2_20_256" (Botan::XMSS_Parameters::xmss_algorithm_t::XMSS_SHA2_20_256)
...

@randombit
Copy link
Owner

I switched param names back. So I think this is now good to merge? It's been almost two months since the notification went out warning about the change and it looks like nobody sees an issue with it.

@mgierlings
Copy link
Collaborator Author

@randombit Yes, I'd say we're good to merge.

@randombit randombit merged commit e390adc into randombit:master May 24, 2019
randombit added a commit that referenced this pull request May 24, 2019
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.

4 participants