Adding signature algorithms via provider interface #19312

baentsch · 2022-09-30T11:07:39Z

~~First cut tackling~~ Fixing #10512 ~~as per discussion there~~. Fixing #19369.

Still considered WIP: Missing e.g. is a test within the OpenSSL test harness. Testing right now done via oqs-provider. Testing available in https://github.com/baentsch/openssl/compare/sigload...baentsch:openssl:sigload-test?expand=1

paulidale

Looks pretty good for the most part.

paulidale · 2022-10-10T00:24:22Z

doc/man7/provider-base.pod

+
+The "TLS-SIGALG" capability can be queried by libssl to discover the list of
+TLS signature algorithms that a provider can support. Each signature supported
+can be used for client- or server-authentication in addition to the build-in


built-in not build-in

so changed in e558217

This seems not to be fixed after all?

In this case, what's not done is s/build-in/built-in/

doc/man7/provider-base.pod

ssl/ssl_cert.c

ssl/ssl_ciph.c

ssl/t1_lib.c

baentsch · 2022-10-10T08:26:52Z

Thanks for the review, @paulidale. I'd like to ask for advice on one more thing: How much effort should be put in the development of a dedicated "fetchable sigalgs" test case for this functionality within the test directory?

Background: The oqsprovider has tests that exercize these code paths much more than anything "artificial" in the test directory: When comparing the test code for fetchable KEMs via tls-provider and what oqsprovider does, the test in the main OpenSSL test folder doesn't seem more comprehensive. It obviously pre-dates oqsprovider and thus was key to ensure (our) smooth OQS-KEM integration, so this is a moot point.

With fetchable sigalgs it's now different, though: oqsprovider already does (X.509) encode/decode --for which there are even (OQS-)OpenSSL1.1.1 interop tests and this PR now uses those certificates with the new dynamically fetched signature algorithms. This "external" testing even discovered --by the sheer number of algorithms added-- previously unseen OpenSSL bugs.

--> Should all of this ("artificial" sign/verify combined with encode/decode for cert/key file generation) be re-created in the test directory or could you agree that presence of oqsprovider as a test facility --beyond all the other tests already existent and already triggering (nearly) all the new code paths-- is sufficient to move this PR to "Ready for Review"?

paulidale · 2022-10-10T08:49:42Z

I'll flag the testing for OTC discussion, relying on OQS testing requires an exception to the testing required rule.

OTC: is the OQS testing sufficient or does there need to be an artifical test in OpenSSL?

mattcaswell · 2022-10-10T09:21:57Z

IMO external testing should only ever be supplementary. We should have our own tests that we can easily extend and modify as required over time. For example, lets say we encounter a bug at some point in the future for this feature that was not picked up by the oqsprovider external testing. Without our own tests to build from we cannot easily add a regression test for the bug. Also consider the case where the maintainers of an external test decide to take things in a different technical direction and no longer maintain the old external test code. Without our own testing we could lose the test coverage we get there.

beldmit · 2022-10-10T10:10:47Z

I agree that we need our own tests here but don't think we are to put this burden on the developer of this PR

baentsch · 2022-10-10T10:46:08Z

We should have our own tests that we can easily extend and modify as required over time. For example, lets say we encounter a bug at some point in the future for this feature that was not picked up by the oqsprovider external testing. Without our own tests to build from we cannot easily add a regression test for the bug.

Agreed -- but this is not time-critical now, right? This could be done as and when the need arises.

Also consider the case where the maintainers of an external test decide to take things in a different technical direction and no longer maintain the old external test code. Without our own testing we could lose the test coverage we get there.

Completely understandable. I can promise to move all required code (basically gutted to not use liboqs calls but using "dummy" KEM/SIG "algs") from oqsprovider into the test directory should such event occur. But then again, I'd understand such willingness to be considered "unreliable" -- particularly if the proverbial truck runs me over...

So what about creating a new issue for this and I (or anyone else with too much time :) works on this separately along the lines of

I agree that we need our own tests here but don't think we are to put this burden on the developer of this PR

mattcaswell · 2022-10-10T11:24:22Z

The test policy says this:

All Pull Requests adding new functionality to the applications, libraries, providers or engines must include suitable tests.

But it also says:

It is acceptable for the tests to be added via a different Pull Request to the main Pull Request.

https://www.openssl.org/policies/technical/testing.html

So I would be ok if the tests are not added in this PR but are instead added via a follow up PR - as long as there is a real commitment to actual produce such a PR!

baentsch · 2022-10-10T13:06:41Z

as long as there is a real commitment to actual produce such a PR!

There definitely is. My goal is to be a trusted contributor -- not just to OQS but also to OpenSSL. FWIW, I started doing OSS when at university in the last century (sigh :) and "only" had to suspend it during the time my (ex-)employer prescribed what I do. But now I'm free again to make good on my words!

Splitting things has the great benefit that different activities can proceed in parallel:

This PR can move forward -- and we can next have "hands-on" discussions on message lengths -- benefitting the OQS and OpenSSL communities as well as standardization efforts.
Further down-stream integration work can be meaningfully tackled (e.g., nginx serving all these algorithms via the oqsprovider) -- benefitting interop testing (e.g., at https://test.openquantumsafe.org).
Adding standalone provider testing as per the above -- providing the benefits you're listing above.

The separate issue for this is now opened: #19369: Please comment/amend as you see fit. In turn, I'm now asking for Review on this PR.

paulidale · 2022-10-10T23:12:21Z

Removing the hold since the decision seems to have been made in favour of having our own test.

mattcaswell

This looks good. Mostly minor comments below. I am surprised not to see any changes in the X.509 code....do we already have everything we need there for certs with provider based sig algs? Is it just a case of the provider registering the correct sigalg oid? I know we talked about this previously but I forget where we ended up.

Also - I'm looking forward to seeing a test for this (in a separate PR). There's quite a lot of code here that I'm anxious about in the absence of that test.

doc/man7/provider-base.pod

ssl/ssl_cert.c

ssl/ssl_ciph.c

ssl/t1_lib.c

mattcaswell · 2022-11-03T15:09:48Z

Oh....this is a significant feature, so it should have an entry in CHANGES.md

baentsch · 2022-11-04T06:59:22Z

@mattcaswell Thanks for the review.

Also - I'm looking forward to seeing a test for this (in a separate PR). There's quite a lot of code here that I'm anxious about in the absence of that test.

Working on it -- but it's an even bigger piece of work than I originally anticipated: The updated test tls-provider not only will "invent" a sig alg and encode/decode logic but also has to have an additional test hash alg as oqs-provider doesn't trigger the "additional hash alg" logic (none of the PQ sigalgs needs a digest).

Your feedback above tempted me briefly to completely eliminate (the already optional) support for a pluggable sigalg provider to set a hash alg: oqsprovider doesn't need it, the current openssl core logic isn't quite geared to handle the situation and the new tests are pretty convoluted. But then again, I don't want to shy back from a conceptually sensible/logical feature just because of the effort involved. All thoughts/preferences/suggestions from your side welcome.

Edit/Add: The more I think about it, the more I am (now also) convinced that #19369 needs to be fixed within this PR. The only reason speaking against that would be if OpenSSL3.2 would be released before I get that done: Can you share your timeline for that?

mattcaswell

Your feedback above tempted me briefly to completely eliminate (the already optional) support for a pluggable sigalg provider to set a hash alg: oqsprovider doesn't need it, the current openssl core logic isn't quite geared to handle the situation and the new tests are pretty convoluted. But then again, I don't want to shy back from a conceptually sensible/logical feature just because of the effort involved. All thoughts/preferences/suggestions from your side welcome.

Yeah - conceptually it seems to make sense to have this capability.

Edit/Add: The more I think about it, the more I am (now also) convinced that #19369 needs to be fixed within this PR. The only reason speaking against that would be if OpenSSL3.2 would be released before I get that done: Can you share your timeline for that?

That would be great. The 3.2 freeze won't be for some months yet so you've got some time.

CHANGES.md

doc/man7/provider-base.pod

ssl/statem/statem_clnt.c

mattcaswell · 2022-11-04T09:43:20Z

ssl/t1_lib.c

        sinf->hash_algorithm = NULL;
-    }
+    else if (p->data_type != OSSL_PARAM_UTF8_STRING)


Since one else branch uses {} our coding style says that all branches should use {} even if they are only one line.

So done in all places in 8be6985

This doesn't seem to be actually done?

Ah, @mattcaswell is correct. Since the else body a couple of lines down is bracketed, all of the if ... else if ... else bodies must be bracketed.

mattcaswell · 2022-11-04T09:43:47Z

ssl/t1_lib.c

-        ERR_raise(ERR_LIB_SSL, ERR_R_PASSED_INVALID_ARGUMENT);
+    if (p == NULL)
+        sinf->oid = NULL;
+    else if (p->data_type != OSSL_PARAM_UTF8_STRING)


All branches should use {}

Done in 8be6985

Here, all of the if ... else if ... else bodies must be bracketed.

ssl/t1_lib.c

baentsch · 2023-02-22T07:27:15Z

@paulidale @mattcaswell @levitte Thanks again for your feedback. With the latest commit all of it should be addressed. Instead of commenting on each of your comments, may I ask you this time to mark things resolved as you see fit and let me know if/when anything still needs to be addressed?

There should be an option to the list command (apps/list.c) to list sigalgs and their parameters. I'm okay if this isn't part of this PR (but it would be nice if it were).

This already works:

openssl list -signature-algorithms -provider oqsprovider
  dilithium2 @ oqsprovider
  p256_dilithium2 @ oqsprovider
  rsa3072_dilithium2 @ oqsprovider
  dilithium3 @ oqsprovider
  p384_dilithium3 @ oqsprovider
  dilithium5 @ oqsprovider
  p521_dilithium5 @ oqsprovider
  falcon512 @ oqsprovider
  p256_falcon512 @ oqsprovider
  rsa3072_falcon512 @ oqsprovider
  falcon1024 @ oqsprovider
  p521_falcon1024 @ oqsprovider
  sphincssha256128ssimple @ oqsprovider
  p256_sphincssha256128ssimple @ oqsprovider
  rsa3072_sphincssha256128ssimple @ oqsprovider
  sphincsshake256128fsimple @ oqsprovider
  p256_sphincsshake256128fsimple @ oqsprovider
  rsa3072_sphincsshake256128fsimple @ oqsprovider

mattcaswell · 2023-02-22T10:14:25Z

All of my comments seem to be addressed with the exception of the one seeking input from @levitte. I have marked them as resolved.

levitte · 2023-02-22T11:22:27Z

All of my comments seem to be addressed with the exception of the one seeking input from @levitte. I have marked them as resolved.

Uhmmm, I did comment on that: #19312 (comment)

mattcaswell

LGTM

openssl-machine · 2023-02-23T23:00:33Z

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

paulidale · 2023-02-24T00:03:09Z

Merged, thanks for the contribution.

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #19312)

mattcaswell · 2023-02-24T07:59:22Z

Woop! Thanks to @baentsch for this cool feature!

baentsch · 2023-02-24T08:34:58Z

Woop! Thanks to @baentsch for this cool feature!

Also very glad to see this. Thanks in turn particularly to @levitte @mattcaswell @paulidale to "shepard" this through. Only little concern remaining: Why is there a "symbol error" in cross-compilation CI now?

mattcaswell · 2023-02-24T10:00:31Z

Also very glad to see this. Thanks in turn particularly to @levitte @mattcaswell @paulidale to "shepard" this through. Only little concern remaining: Why is there a "symbol error" in cross-compilation CI now?

Seems somehow related to #20331 which was also recently merged.

mattcaswell · 2023-02-24T10:01:58Z

@levitte?

https://build.opensuse.org/request/show/1092835 by user msmeissn + dimstar_suse - updated to 0.5.0: - oqs-provider now also enables use of QSC algorithms during TLS1.3 handshake. The required OpenSSL code updates are contained in openssl/openssl#19312. * Algorithm updates All algorithms no longer supported in the NIST PQC competition and not under consideration for standardization by ISO have been removed. All remaining algorithms with the exception of McEliece have been lifted to their final round 3 variants as documented in liboqs. Most notably, algorithm names for Sphincs+ have been changed to the naming chosen by its authors. * Functional updates - Enablement of oqs-provider as a (first) dynamically fetchable OpenSSL3 TLS1.3 signature provider. - OSX support - Full support for CA functionality - Algorithms can now be se

baentsch mentioned this pull request Sep 30, 2022

Pluggable Signatures mechanism implementation approaches #10512

Closed

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Sep 30, 2022

baentsch force-pushed the sigload branch from 692ec44 to b1d9006 Compare October 9, 2022 13:21

paulidale reviewed Oct 10, 2022

View reviewed changes

paulidale added hold: need otc decision The OTC needs to make a decision branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Oct 10, 2022

baentsch mentioned this pull request Oct 10, 2022

Implement tests for provider-based signature algorithms #19369

Closed

baentsch marked this pull request as ready for review October 10, 2022 13:06

paulidale removed the hold: need otc decision The OTC needs to make a decision label Oct 10, 2022

baentsch mentioned this pull request Oct 26, 2022

Semantics of OSSL_FUNC_keymgmt_has/wrong PEM_read_bio_PrivateKey_ex selector logic #19498

Closed

baentsch force-pushed the sigload branch from e558217 to 7cb1a9b Compare November 3, 2022 07:54

t8m added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Nov 3, 2022

mattcaswell requested changes Nov 3, 2022

View reviewed changes

mattcaswell requested changes Nov 4, 2022

View reviewed changes

This was referenced Nov 21, 2022

Add hybrid KEX to TLS benchmarking tests open-quantum-safe/profiling#83

Closed

When using PEM_read_bio_PrivateKey_ex() the public key is optional #19505

Closed

t8m added tests: present The PR has suitable tests present and removed tests: present The PR has suitable tests present labels Nov 25, 2022

baentsch added 2 commits February 22, 2023 08:03

addressing PR feedback

a6ee6e2

more PR feedback incorporated

1ea1216

remove reference to legacy keys

280f9d4

mattcaswell approved these changes Feb 22, 2023

View reviewed changes

mattcaswell removed the approval: otc review pending This pull request needs review by an OTC member label Feb 22, 2023

baentsch mentioned this pull request Feb 22, 2023

Extend speed testing to provider-based sig and kem algorithms #19968

Closed

paulidale approved these changes Feb 22, 2023

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Feb 22, 2023

paulidale closed this Feb 24, 2023

openssl-machine pushed a commit that referenced this pull request Feb 24, 2023

first cut at sigalg loading

ee58915

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #19312)

This was referenced Feb 25, 2023

Implement full TLS1.3 OQS signature support open-quantum-safe/oqs-provider#9

Closed

replace oqs-openssl111 open-quantum-safe/oqs-demos#182

Open

mattcaswell mentioned this pull request Mar 2, 2023

RFC7250 (RPK) support (Fixes #6929) #18185

Closed

2 tasks

baentsch mentioned this pull request Mar 3, 2023

Default and mandatory digest for provider-based signature algorithms #20428

Closed

VeronikaNguyen mentioned this pull request Mar 9, 2023

Support for large PQC signature algorithms #20478

Closed

baentsch mentioned this pull request Mar 13, 2023

fixing up EVP_PKEY_get_id for providers #20501

Closed

baentsch mentioned this pull request Jul 11, 2023

Missing algorithms even though enabled in liboqs open-quantum-safe/oqs-provider#210

Closed

ambrisko mentioned this pull request Jul 18, 2023

Provider signing with CMS silently fails ... I thought the change was upstreamed in OpenSSL 3.2-dev? open-quantum-safe/oqs-provider#218

Closed

baentsch mentioned this pull request Aug 18, 2023

No QSC TLS signature support open-quantum-safe/oqs-provider#231

Closed

Adding signature algorithms via provider interface #19312

Adding signature algorithms via provider interface #19312

Conversation

baentsch commented Sep 30, 2022 • edited

paulidale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baentsch commented Oct 10, 2022

paulidale commented Oct 10, 2022 • edited

mattcaswell commented Oct 10, 2022

beldmit commented Oct 10, 2022

baentsch commented Oct 10, 2022

mattcaswell commented Oct 10, 2022

baentsch commented Oct 10, 2022

paulidale commented Oct 10, 2022

mattcaswell left a comment

Choose a reason for hiding this comment

mattcaswell commented Nov 3, 2022

baentsch commented Nov 4, 2022 • edited

mattcaswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baentsch commented Feb 22, 2023

mattcaswell commented Feb 22, 2023

levitte commented Feb 22, 2023

mattcaswell left a comment

Choose a reason for hiding this comment

openssl-machine commented Feb 23, 2023

paulidale commented Feb 24, 2023

mattcaswell commented Feb 24, 2023

baentsch commented Feb 24, 2023

mattcaswell commented Feb 24, 2023

mattcaswell commented Feb 24, 2023

baentsch commented Sep 30, 2022 •

edited

paulidale commented Oct 10, 2022 •

edited

baentsch commented Nov 4, 2022 •

edited