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

Fix for alg_support.cmake #1716

Merged
merged 8 commits into from
Mar 7, 2024
Merged

Fix for alg_support.cmake #1716

merged 8 commits into from
Mar 7, 2024

Conversation

bhess
Copy link
Member

@bhess bhess commented Mar 1, 2024

Addresses the following cases:

  • If I compile with -DOQS_ALGS_ENABLED=STD, I still expect the aarch64 (or avx2) variants available by default if they are supported on the target machine.
  • If I compile with -DOQS_ALGS_ENABLED=STD, I expect both ml-dsa-44-ipd and its alias ml-dsa-44 to be available, although only the alias was specified in the "STD" list. ml-dsa-44 is added to the STD list but not ml-dsa-44-ipd

It's addressed by refactoring alg_support.cmake and the fragments that generate it, by moving the cmake statements to conditionally activate platform-specific code and aliases below the calls to "filter_algs". filter_algs would otherwise remove these dependencies.

Adds a test to CI to check if both a ml-dsa-ipd and a ml-dsa alias is available with the STD build.

Adds missing bike_l5 to the NIST_R4 algs list (unless this variant wasn't there intentionally).

Fixes #1703
Fixes #1704

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

@bhess bhess requested review from baentsch and dstebila as code owners March 1, 2024 19:12
@bhess bhess changed the title Bhe algsupport fix Fix for alg_support.cmake Mar 1, 2024
@baentsch
Copy link
Member

baentsch commented Mar 2, 2024

compile with -DOQS_ALGS_ENABLED=STD, I expect both ml-dsa-44-ipd and its alias ml-dsa-44 to be available

Isn't that confusing? If a user wants standardized algorithms to be present, why present non-standardized name(s), particularly considering the algs are present under their standardized name(s)?

@dstebila
Copy link
Member

dstebila commented Mar 3, 2024

compile with -DOQS_ALGS_ENABLED=STD, I expect both ml-dsa-44-ipd and its alias ml-dsa-44 to be available

Isn't that confusing? If a user wants standardized algorithms to be present, why present non-standardized name(s), particularly considering the algs are present under their standardized name(s)?

Initially I had interpreted "STD" to mean "standards-track", because the various algorithms we had in there were not necessarily standards yet, but were on their way to standardization. Under that interpretation, having the "ipd" algorithms in there makes sense. But not everyone may have that interpretation.

@baentsch
Copy link
Member

baentsch commented Mar 4, 2024

Initially I had interpreted "STD" to mean "standards-track", because the various algorithms we had in there were not necessarily standards yet, but were on their way to standardization. Under that interpretation, having the "ipd" algorithms in there makes sense. But not everyone may have that interpretation.

Indeed, initially "STD" meant "standards-track" -- but time has moved on and NIST made standardization decisions, so IMO "STD" now means exactly what is documented: "STD" selecting all algorithms standardized by NIST. liboqs never meant to "select winners" but once they have been selected it should give them particular care and attention. Also, PQC is slowly becoming more "mainstream" -- and that means that users should not be concerned with "standardization details" but begin to rely on a library to (be able to exactly) deliver what is standardized.

@bhess
Copy link
Member Author

bhess commented Mar 4, 2024

NIST described the algorithms as Candidates to be Standardized, so I still interpret "STD" as standards-track at the moment. I think the relevant information is that there is a possibility that the algorithms are still changed before the final standard, and none of the algorithms in the STD list are a final standard yet.

@baentsch
Copy link
Member

baentsch commented Mar 4, 2024

NIST described the algorithms as Candidates to be Standardized, so I still interpret "STD" as standards-track at the moment. I think the relevant information is that there is a possibility that the algorithms are still changed before the final standard, and none of the algorithms in the STD list are a final standard yet.

The link quoted above is from 2022. Time has moved on and FIPS-203 has been published, clearly designating the term "ML-KEM" as a standard:

Name of Standard. Module-Lattice-based Key-Encapsulation Mechanism Standard (ML-
KEM) (FIPS PUB 203).

I'd concede that it might be prudent to introduce a new algorithm class ("below" STD) as for example "STD-track" to list the algs that probably are subject to re-naming. If you want to do this in this PR, fine with me.

But then again, this whole discussion aims to make life easier for consumers of the library (not having to adapt their integrations, configs and code to algorithm name changes). This of course only is a concern in case we want to make OQS more "production oriented", which I want to move towards. Accordingly I think retaining the "ipd" moniker is wrong, misleading and means lots of additional work (here and in all downstreams, now and in the future).

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Designating the "-ipd" variants as STD is misleading and not amenable to downstream use.

@bhess
Copy link
Member Author

bhess commented Mar 4, 2024

Thanks for the feedback @baentsch @dstebila ! I think it's good to have this discussion about what "STD" means and what the requirements on naming and algorithms are. If I understand your comment @baentsch you mainly consider "STD" algorithms with "names that won't change". My interpretation was more on the lines of "algorithm compatibility won't break".

The link quoted above is from 2022. Time has moved on and FIPS-203 has been published, clearly designating the term "ML-KEM" as a standard:

Although the ML-KEM name seems fixed for the final standard, it's still FIPS 203 (draft) and afaik the information on the 2022 page still holds: [.."After the close of the comment period, NIST will revise the draft standards as appropriate based on the feedback received."].

But then again, this whole discussion aims to make life easier for consumers of the library (not having to adapt their integrations, configs and code to algorithm name changes). This of course only is a concern in case we want to make OQS more "production oriented", which I want to move towards. Accordingly I think retaining the "ipd" moniker is wrong, misleading and means lots of additional work (here and in all downstreams, now and in the future).

I'm with you with the goal to make life easier for consumers of the library. The alias feature was meant to allow the use case you promote to always track the latest algorithm version without name changes (for consumers accepting that future versions of the algorithm might be not compatible).
But again, there are other "production" use cases where versioning is important: assume a consumer decides to deploy certificates with the current ML-DSA(-ipd) algorithms. They likely have to continue supporting the certificates for a while before updating to the final standard. Having an explicit "ipd" option that guarantees compatibility won't break in the future will make their life easier. Even after standardization versioning can be useful: TLS is a commonly known protocol name, but one might have to support both TLS 1.2 and TLS 1.3 for interoperability with different clients, which is only possible if one can state the protocol version.

I'd concede that it might be prudent to introduce a new algorithm class ("below" STD) as for example "STD-track" to list the algs that probably are subject to re-naming. If you want to do this in this PR, fine with me.

Agreed that the two classes make sense. Not sure if there is a consensus yet about what algorithms go to "STD" and "STD-track". Also, with this PR algorithm names and their alias are always both available and can be used interchangeably (e.g., either "ipd" can be used for forward-compatibility or "non-ipd" for stable names).

@baentsch
Copy link
Member

baentsch commented Mar 4, 2024

But again, there are other "production" use cases where versioning is important: assume a consumer decides to deploy certificates with the current ML-DSA(-ipd) algorithms. They likely have to continue supporting the certificates for a while before updating to the final standard. Having an explicit "ipd" option that guarantees compatibility won't break in the future will make their life easier.

Hmm... Two questions to check out this argument:

  1. Which specification(s) encode algorithm names in certificates?
  2. Which software uses liboqs directly to generate certificates (and encode its algorithm names into them)?

My understanding so far was that OIDs represent algorithms (and their versions) in certificates, but maybe I'm wrong. If so, we can also relax the requirement for proper OID management as per open-quantum-safe/oqs-provider#351 and move it to "management by names". I'd consider this dead wrong, but welcome comments.

Agreed that the two classes make sense. Not sure if there is a consensus yet about what algorithms go to "STD" and "STD-track".

Proposal:
STD: ML-KEM, ML-DSA
STD-track: Sphincs, Falcon, *-ipd

Also, with this PR algorithm names and their alias are always both available and can be used interchangeably

This exactly is what I don't agree with as sensible: The two cannot and shall not be used interchangeably: -ipd IMO is a non-persistent name; it's the first (and hopefully only) name in liboqs encoding algorithm version; "ML-KEM", "ML-DSA" are persistent names designating the standard algorithms (whichever their code versions).

Undisputed is that this PR fixes bugs, though, so I'm approving just because of this, not because I agree with your intentions -- which I infer from

"ipd" can be used for forward-compatibility

You want to establish these names and along with that, the need to maintain this code for a long time -- really through space and time: The former denotes the availability of "-ipd" in all OQS downstreams, the latter means OQS will never be able to drop it -- and even have to support this code base in addition to the standardized one if and when someone eventually decides to afford some "production qualities" to liboqs. This increases maintenance and development effort to OQS -- at a time we're pruning stuff as no-one steps forward to do more maintenance-type work (e.g., integrations, profiling, binary/distro management, automated build management, fuzzing, etcetc)...

@baentsch baentsch dismissed their stale review March 4, 2024 14:10

PR improves matters

@bhess
Copy link
Member Author

bhess commented Mar 5, 2024

The aliases give the possibility to support different versions at the same time. I don't see why you imply that algorithms therefore need to be supported forever.

I also don't mean to replace OIDs in certificates with names, but simply to support a use case where libOQS can be used (by software handling certificates) with different versions at the same time. I don't see a different way to do this other than a separate identifier.

The two cannot and shall not be used interchangeably

It seemed logical to me that the name and the alias are either both available or none, since they are the same algorithm. But I don't have a strong enough opinion on it to insist. I'm ok to update the PR to change this: for example, activating the ML-DSA macros won't automatically activate the ML-DSA-ipd macro (and vice versa).

@baentsch
Copy link
Member

baentsch commented Mar 5, 2024

I don't see why you imply that algorithms therefore need to be supported forever.

I don't imply this. You introduce this demand with the statement

Having an explicit "ipd" option that guarantees compatibility won't break in the future

Additionally you state,

I also don't mean to replace OIDs in certificates with names, but simply to support a use case where libOQS can be used (by software handling certificates) with different versions at the same time.

"different versions at the same time" exactly states that you want to keep supporting "ml-kem/dsa-ipd" for an unspecified "future" in parallel to the standardized "ml-kem/dsa"

I don't see a different way to do this other than a separate identifier.

Me neither. We simply need to accept your demand to retain several versions of the same algorithm in the library at the same time. We have not done this so far -- one reason for which was to reduce the support and testing effort.

An obvious alternative to support your use case is to create a "legacy provider" supporting old OIDs (e.g., those that you currently reserved for ml-dsa-ipd) built against a version of liboqs containing that (then old -ipd) code base. This way, neither liboqs nor oqsprovider would have to keep maintaining the "-ipd" code bases but can move forward as the standards evolve.

I'm ok to update the PR to change this: for example, activating the ML-DSA macros won't automatically activate the ML-DSA-ipd macro (and vice versa).

That would be a great (re)solution. That way we can manage these two separate algorithms separately. Yes, I know, right now they are identical -- and may well remain so; but if there's a different "ipd2" or final standard, we're covered and can prune "-ipd" without impacting the downstreams.

@baentsch baentsch added this to the 0.10.0 milestone Mar 5, 2024
@bhess bhess force-pushed the bhe-algsupport-fix branch from 3f7885c to 97d7b6a Compare March 6, 2024 10:05
@bhess
Copy link
Member Author

bhess commented Mar 6, 2024

activating the ML-DSA macros won't automatically activate the ML-DSA-ipd macro (and vice versa).

It's now integrated with the latest commit.

@@ -6,6 +6,7 @@

#if defined(OQS_ENABLE_KEM_classic_mceliece_348864)

#if defined(OQS_ENABLE_KEM_classic_mceliece_348864)
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? Duplication?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah this is superfluous, updated it to avoid the duplication.

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Thanks for working through this and addressing my previous comment(s) @bhess ! This seems LGTM (resolving the issues) but there seems to be a (hopefully minor) snafu in the generator scripts causing the PR to touch so (too?) many files. Can you please take a look?

@bhess
Copy link
Member Author

bhess commented Mar 7, 2024

generator scripts causing the PR to touch so (too?) many files

Thank you for the feedback. I've updated the PR to avoid the generator scripts touching the unrelated files.

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Thanks for the latest improvements, @bhess: Made reviewing much easier. There's just one "nit" remaining: Falcon_padded seems to not be part of the STD alg list. Possible omission due to #1710 just recently landing I suppose. Maybe worthwhile rebasing and updating that before merge?

@bhess bhess merged commit 0961090 into main Mar 7, 2024
75 of 76 checks passed
@bhess bhess deleted the bhe-algsupport-fix branch March 7, 2024 12:19
Eddy-M-K pushed a commit to Eddy-M-K/liboqs that referenced this pull request Apr 5, 2024
* Ensure aliases are activated with cmake
* Updates alg_support fragments: ensure that dependencies (aliases and platform-specific code) are activated after applying filter_algs
* Adds bike_l5 to NIST_R4 algorithms
* add CI test for aliases
* remove ml_kem ipds from STD filter_algs
* decouple name and alias
* fixing vector tests

Signed-off-by: Eddy Kim <Eddy.M.Kim@outlook.com>
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.

Add tests for alias behaving identically to at least one baseline alg. filter_args not correctly functioning
3 participants