Prepare 240 for merge

[trigpolynom]

This PR is a fix for #239. It builds on #240 in order to make hash-n-sign available to digital signature implementation.

See the list of OIDs for reference. In this PR I only implemented dilithium2 hash-n-sign algs given this comment by @baentsch.

[trigpolynom]

@baentsch after a lot of figuring stuff out, it seems this PR has passed all CI tests and ready for review,

[trigpolynom]

fixing the other failed checks

[baentsch]

Thanks for the contribution and taking this on!

fixing the other failed checks

Hmm -- there's still quite some CI failing... Also --knowing this is a Draft not ready for review-- please allow me anyway to already comment that the changes visible should not survive a code generation run (generate.py) as and when necessary again. Please consider updating the template files in oqs-template instead/too.

[trigpolynom]

@baentsch So I made the changes here to generate.py. I've tested that running generate should produce same code with passing tests. Are there other changes I need to make?

[baentsch]

@baentsch So I made the changes here to generate.py. I've tested that running generate should produce same code with passing tests. Are there other changes I need to make?

Nope. My bad: I was expecting to see changes to the template files themselves. But you avoided that (need I assumed) by manually creating loads of new entries in the generate.yml file: I kind of was expecting that an orthogonal method to "mix_with" would be added, e.g., "add_hashes": That would then make easier (automate) adding hashes to all other sig algs.

So what are your plans to move this to "Ready for Review"? Iron out CI issues? Align the code points with "main" (see the comment in generate.yml as to which numbers should be allocated)? Anything else? More sig algs than just Dilithium2?

[trigpolynom]

@baentsch So right now I wanted to iron out the CI issues. I just fixed the asan Security Check test failure so that check should pass now. What do you think I should add? Should I do more sigalgs other than dilithium? Also I tried to change the YAML such that the hash-n-sign sigalgs would just be mixed with but I wasn't exactly sure how that should have been formatted.

[baentsch]

Should I do more sigalgs other than dilithium?

Yes, that would have been my suggestion (also was with the original PR): By adding (support for) more algs, the mechanisms to be added to automatically "mix in" the various digests to the sigalgs need to be created and exercized.

Also I tried to change the YAML such that the hash-n-sign sigalgs would just be mixed with but I wasn't exactly sure how that should have been formatted

This would be defined by YAML -- should be the same syntax as with the current hybrids being "mixed in". What might be more convoluted are the changes to the generator to properly process such list of hashes-with-OIDs-and-codepoints.

But honestly, I'd leave that to you: Either one manually writes YML statements for each new alg combination to be added (you decide how many/which ones are needed) or creates code to automate this for many algorithms. I typically prefer the latter as it makes oqsprovider more independent from upstream (e.g., name) changes -- but it's a lot of work to get right for a rare event. The second downside to full automation/matrix of digests-and-sigalg is that you need lots of OIDs and code points...

[trigpolynom]

Yes, that would have been my suggestion (also was with the original PR): By adding (support for) more algs, the mechanisms to be added to automatically "mix in" the various digests to the sigalgs need to be created and exercized.

Okay cool I'll do this!

But honestly, I'd leave that to you: Either one manually writes YML statements for each new alg combination to be added (you decide how many/which ones are needed) or creates code to automate this for many algorithms. I typically prefer the latter as it makes oqsprovider more independent from upstream (e.g., name) changes -- but it's a lot of work to get right for a rare event. The second downside to full automation/matrix of digests-and-sigalg is that you need lots of OIDs and code points...

And yeah this would be a lot of work. I'll first try the manual approach and see where that gets us.

[trigpolynom]

@baentsch , quick update. Added all the hash-n-sign algs listed in this list. Every test is passing except for tlssig and its only failing on a handful of algs (See below). Figuring this out now.

SSL_accept() failed -1, 1SSL_connect() failed -1, 1  TLS-SIG handshake test failed: falcon512WithShake128, return code: -5
SSL_accept() failed -1, 1SSL_connect() failed -1, 1  TLS-SIG handshake test failed: sphincssha2128fsimple, return code: -5
SSL_accept() failed -1, 1SSL_connect() failed -1, 1  TLS-SIG handshake test failed: p256_sphincssha2128fsimple, return code: -5
SSL_accept() failed -1, 1SSL_connect() failed -1, 1  TLS-SIG handshake test failed: rsa3072_sphincssha2128fsimple, return code: -5
SSL_accept() failed -1, 1SSL_connect() failed -1, 1  TLS-SIG handshake test failed: sphincssha2128ssimple, return code: -5
SSL_accept() failed -1, 1SSL_connect() failed -1, 1  TLS-SIG handshake test failed: sphincsshake128fsimple, return code: -5
SSL_accept() failed -1, 1SSL_connect() failed -1, 1  TLS-SIG handshake test failed: p256_sphincsshake128fsimple, return code: -5
SSL_accept() failed -1, 1SSL_connect() failed -1, 1  TLS-SIG handshake test failed: rsa3072_sphincsshake128fsimple, return code: -5

[baentsch]

Figuring this out now.

OK. FWIW, some CI failures should be fixed when you re-base to latest "main".

[trigpolynom]

@baentsch do you have any hunch on why only the above mentioned sigalgs are failing? The error code that seems to represent the cause has to do with SSL_ERROR_ZERO_RETURN. For context, the test certs are being generated and verified just fine. I also re-based with main.

[baentsch]

I'm afraid I don't (see any "communalities"): Plain as well as Hybrid algs.... Weird. This will possibly require some serious OpenSSL debugging... I'd start with just activating OpenSSL logging. But gdb may lead to quicker insights...

[baentsch]

Looks much better now (passes all tests locally), Thanks! Looking at the IDs (code points), it seems they are simply manually assigned sequentially. Is there no other implementation using other IDs, e.g., in https://github.com/IETF-Hackathon/pqc-certificates with which this could be tested for interoperability? Do you intend to automate the config writing somewhat? All combinations now must be manually created (and maintained), right? Also sensible would be a mechanism to assert no IDs are duplicated (as per the last problem): Stuff for subsequent PRs?

[trigpolynom]

@baentsch I definitely think the config needs to be automated and checked for duplicates. My thinking is that this should be its own PR. Which I of course would love to take on

[baentsch]

This is impossible: How can OID remain the same but code point change? Please explain.

[trigpolynom]

What exactly is the relationship between OID and code point? Or what should it be?

[baentsch]

IMO it should be a one-to-one relationship as per TLS1.3: If it were not, TLS clients and servers using different algorithm implementations, but the same code point would not interoperate and nobody would understand why. This of course only is a problem until an algorithm gets standardized

[baentsch]

Thanks again for all the work that went into this @trigpolynom ! When going over it I noticed many code point changes without corresponding/matching OID changes: Why has this been done? Inadvertent? Matching up with other implementations? This needs to be clarified before merge.

Also, the sheer size (number of algorithms added) of this code change makes me nervous: Would it be conceivable to wrap this into a config variable, e.g., "OQS_ENABLE_HASHNSIGN"? By how much does the size of the provider grow with hashnsign? What's the smallest provider that can be configured/built now? This is a "nice to know" for me but possibly relevant for users, so any insight?

[ghost]

Can you explain why you commented on these three lines?

[ghost]

We should indent this line.
It makes me think that we don't have a style guide for our CMake files…

[trigpolynom]

Do we not have one for CMake files?

[baentsch]

Do we not have one for CMake files?

Not that I knew of -- pointers welcome.

[trigpolynom]

@baentsch and @thb-sb . Thank you for these comments and suggestions, they are super helpful. I'll get to the requested changes ASAP. (Going on vaction this week, but will dive in when i get back). I'll also investigate the effect on the size of the provider that hashnsign has. Perhaps it is best if the addition of the new sigalgs is configurable and automated by a new method in generate.py?

[baentsch]

Perhaps it is best if the addition of the new sigalgs is configurable and automated by a new method in generate.py?

Yes, this sounds like a doable approach.

Going on vaction this week, but will dive in when i get back

Thanks for letting us know. Enjoy the holiday season then.

[trigpolynom]

Recently picked this back up. Thinking that a good bit of refactoring needs to occur to make it configurable.

[trigpolynom]

this seems to be a good thing to monitor regarding this functionality:

https://groups.google.com/a/list.nist.gov/g/pqc-forum/c/JKMh0D0pa30/m/vbflXolxAQAJ?utm_medium=email&utm_source=footer

[baentsch]

What's your take as to how to move this PR forward, @trigpolynom ? Are you expecting input from either @thb-sb or myself? Do you want to put this into hiatus (Draft mode) until there's more clarity on the mailing list? Or even close for now?

[trigpolynom]

@baentsch Given where the conversation is in the mailing list. I would vote for closing for now.

[baentsch]

@baentsch Given where the conversation is in the mailing list. I would vote for closing for now.

Just glancing over the discussion(s) I tend to agree... Thanks again for your work @trigpolynom and please consider submitting a new PR as and when the dust settled.