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

first cut adding ML-* #348

Merged
merged 7 commits into from
Feb 24, 2024
Merged

first cut adding ML-* #348

merged 7 commits into from
Feb 24, 2024

Conversation

baentsch
Copy link
Member

@baentsch baentsch commented Feb 3, 2024

This is meant as a test-bed for upstream-triggered CI testing of algorithm-changing PRs such as open-quantum-safe/liboqs#1626

This PR thus replaces #344 introducing ML-KEM and ML-DSA

@baentsch baentsch marked this pull request as ready for review February 17, 2024 12:54
@baentsch baentsch requested review from bhess and a user February 19, 2024 10:18
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM! It's nice to see these ML- definitions here :)

| hqc256 | 1.3.9999.99.74 | OQS_OID_HQC256
| p521_hqc256 | 1.3.9999.99.73 | OQS_OID_P521_HQC256
| p521_kyber1024 | 1.3.9999.99.70 | OQS_OID_P521_KYBER1024
| mlkem512 | 1.3.6.1.4.1.22554.5.6.1 | OQS_OID_MLKEM512
Copy link
Member

Choose a reason for hiding this comment

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

ML-KEM (e.g., mlkem512) has the same OID as Kyber (e.g., kyber512), is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Kind-of: In the absence of a spec (or can you point to one, @bhess ?) I took the OIDs chosen by the IETF hackathon (assuming that's the latest ... is it, @praveksharma ?) And if they chose the same OID as for Kyber, well, that points to bad OID management (in this case apparently by the Legions of Bouncy Castle owning that range) and/or the need for someone to take the lead and draft a spec (@dstebila -- what about an "independent" PQ alliance taking that lead?)

Copy link
Member

Choose a reason for hiding this comment

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

Kind-of: In the absence of a spec (or can you point to one, @bhess ?) I took the OIDs chosen by the IETF hackathon (assuming that's the latest ... is it, @praveksharma ?) And if they chose the same OID as for Kyber, well, that points to bad OID management (in this case apparently by the Legions of Bouncy Castle owning that range) and/or the need for someone to take the lead and draft a spec (@dstebila -- what about an "independent" PQ alliance taking that lead?)

I don't know who is meant to assign the permanent OID for an algorithm -- is it the inventor? the standards body? the first implementer? an alliance? @bhess, if memory serves, IBM assigned some OIDs for earlier rounds of Kyber and Dilithium, are you planning to assign OIDs for ML--ipd or ML-?

Copy link
Member

Choose a reason for hiding this comment

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

NIST usually registers OIDs for approved/standardized algorithms in CSOR. We have in the past reserved OIDs for algorithms used but not already assigned (earlier round versions and ML-DSA-ipd).

Copy link
Member Author

Choose a reason for hiding this comment

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

We have in the past reserved OIDs for algorithms used

Please document this in generate.yml as per #351; more people taking responsibility for proper (O)ID allocation very welcome: We could split this task among all of us, e.g., along algorithms: Volunteers welcome!

| p521_frodo1344aes | 1.3.9999.99.57 | OQS_OID_P521_FRODO1344AES
| frodo1344shake | 1.3.9999.99.60 | OQS_OID_FRODO1344SHAKE
| p521_frodo1344shake | 1.3.9999.99.59 | OQS_OID_P521_FRODO1344SHAKE
| frodo640aes | 1.3.9999.99.58 | OQS_OID_FRODO640AES
Copy link
Member

Choose a reason for hiding this comment

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

Looks like all KEM OIDs changed, is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are generated OIDs, i.e. ones that no-one ever cared about (to specify). I'm thus inclined to leave as-is (and take complaints as indications that they ought to be defined in "generate.yml" -- with the complaints pointing to a spec about whose existence I'd gladly learn).

Copy link
Member

@bhess bhess Feb 19, 2024

Choose a reason for hiding this comment

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

Fine with me to use generated ones with the absence of specified ones. Just wondering why the PR updates the OIDs of algorithms unrelated to ML-KEM/DSA (e.g., frodo640aes:1.3.9999.99.50 -> 1.3.9999.99.58), did these algorithms change (in liboqs)?

Copy link
Member Author

Choose a reason for hiding this comment

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

why the PR updates the OIDs of algorithms

if 'hybrid_oid' in kem: phyb['hybrid_oid']=kem['hybrid_oid']
else: phyb['hybrid_oid'] = get_tmp_kem_oid()
kem['hybrids'].insert(0, phyb)
if not 'oid' in kem:
kem['oid'] = get_tmp_kem_oid()

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the pointer, I see two issues with the get_tmp_kem_oid() code.

def get_tmp_kem_oid():
global kemoidcnt
kemoidcnt = kemoidcnt+1
return "1.3.9999.99."+str(kemoidcnt)

  • OIDs of unrelated algorithms change if new ones are added: e.g., frodo640aes: 1.3.9999.99.50 -> 1.3.9999.99.58
  • OID collisions: e.g., before the PR, OID 1.3.9999.99.54 was for frodo976aes, now the same OID is for x448_bikel3

But maybe I misunderstand the purpose of the temp KEM OIDs. If they are just dummy values not supposed to be globally unique, why not assign the same temp OID for all algorithms without specified OID?

Copy link
Member Author

@baentsch baentsch Feb 22, 2024

Choose a reason for hiding this comment

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

does OQS Provider support generation of X.509 certs with KEM public keys as the subject public key?

No(t that I knew): How would the cert signature be done in such scenario? These OIDs are solely used to register a KEM en/decoder for public and private keys. Had I known that this would create such a fuzz, I'd never have done #266.

Copy link
Member Author

Choose a reason for hiding this comment

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

the OIDs will change when this PR lands, and then change back (!?) when the second PR lands? That seems like it will cause even more chaos.

The OIDs have changed already. They're random, after all -- since October 2023.

Copy link
Member

Choose a reason for hiding this comment

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

does OQS Provider support generation of X.509 certs with KEM public keys as the subject public key?

No(t that I knew): How would the cert signature be done in such scenario?

The subject's public key can be a KEM / public key encryption key, but the CA must use a digital signature scheme key in order to produce a signature on such a certificate.

Copy link
Member Author

Choose a reason for hiding this comment

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

So how is this facilitated using openssl? I for sure have never done this. If you know how to, please contribute a test for this.

Copy link
Member

Choose a reason for hiding this comment

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

the OIDs will change when this PR lands, and then change back (!?) when the second PR lands? That seems like it will cause even more chaos.

The OIDs have changed already. They're random, after all -- since October 2023.

Did they change in October 2023, or were they introduced in October 2023? I'm looking at 5515b4b#diff-ef6f384f213c1efa6cdc7974fec75a672ce414650acb8ec1f0abdd5984076595R167 which is dated October 5 2023 and it looks to me like this when OIDs for e.g. FrodoKEM were first added, and then they've been stable since them.

Copy link
Member

@bhess bhess 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 adding&testing this in parallel to the ML-* addition in liboqs!

@baentsch baentsch mentioned this pull request Feb 22, 2024
@baentsch baentsch merged commit 19e5a97 into main Feb 24, 2024
1 check passed
@baentsch baentsch deleted the bhe-fips-ipd-tracker branch February 24, 2024 06:34
@bhess bhess mentioned this pull request Mar 7, 2024
feventura pushed a commit to EntrustCorporation/oqs-provider that referenced this pull request Mar 13, 2024
* introducing ML-* algorithms

* split KEX testing in 2 and add openssl bug warning to README

* clarify utility of KEM OIDs
feventura pushed a commit to EntrustCorporation/oqs-provider that referenced this pull request Mar 16, 2024
* introducing ML-* algorithms

* split KEX testing in 2 and add openssl bug warning to README

* clarify utility of KEM OIDs

Signed-off-by: Felipe Ventura <[email protected]>
feventura pushed a commit to EntrustCorporation/oqs-provider that referenced this pull request Mar 17, 2024
* introducing ML-* algorithms

* split KEX testing in 2 and add openssl bug warning to README

* clarify utility of KEM OIDs
feventura pushed a commit to EntrustCorporation/oqs-provider that referenced this pull request Mar 17, 2024
* introducing ML-* algorithms

* split KEX testing in 2 and add openssl bug warning to README

* clarify utility of KEM OIDs
feventura pushed a commit to EntrustCorporation/oqs-provider that referenced this pull request Mar 17, 2024
* introducing ML-* algorithms

* split KEX testing in 2 and add openssl bug warning to README

* clarify utility of KEM OIDs

Signed-off-by: Felipe Ventura <[email protected]>
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.

3 participants