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.

Sign up for GitHub

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

Jump to bottom

Remove unmanaged KEM OIDs #522

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Remove unmanaged KEM OIDs #522

wants to merge 5 commits into from

Conversation

baentsch
Copy link
Member

Fixes #520

@baentsch
disable tmp OID generation
a60f6b7 
Signed-off-by: Michael Baentsch <[email protected]>
@baentsch
disable tests on no-OID KEMs
e94338d 
Signed-off-by: Michael Baentsch <[email protected]>
@baentsch baentsch requested review from a team, bhess and SWilson4 September 16, 2024 06:53
@baentsch baentsch mentioned this pull request Sep 16, 2024
Open
@dstebila
Copy link
Member

Do we have any idea of who might be using this functionality? For example, the IETC PQC certificate hackathon group? If so, would be good to know if/how this impacts them. @praveksharma have you been in recent contact with them at all? Or perhaps experiment in the NCCoe -- @christianpaquin any insight from NCCoE?

Did you consider removing our hard-coded OIDs but still letting it be enabled at runtime if the relevant environment variable is set? (Without needing to regenerate from generate.yml and rebuild.)

@christianpaquin
Copy link

For example, the IETC PQC certificate hackathon group? If so, would be good to know if/how this impacts them. @praveksharma have you been in recent contact with them at all? Or perhaps experiment in the NCCoe -- @christianpaquin any insight from NCCoE?

The NCCoE follows closely the work of the IETF hackathon, so their feedback would be helpful. Can you give some guidance, @johngray-dev?

@praveksharma
Copy link
Member

Do we have any idea of who might be using this functionality? For example, the IETC PQC certificate hackathon group?

I haven't had recent contact regarding OID mappings but the relevant documentation in the IETF repo does not list any of the OIDs affected by this PR.

@bhess bhess mentioned this pull request Sep 19, 2024
Open
@baentsch
Copy link
Member Author

Did you consider removing our hard-coded OIDs but still letting it be enabled at runtime if the relevant environment variable is set? (Without needing to regenerate from generate.yml and rebuild.)

No. I have never received positive nor negative feedback regarding this feature (added originally in support of an IETF hackathon) so consider it safe to drop for unassigned OIDs. Again, if someone wants to test any of the KEMs without OIDs, they should have an idea about the OID -- it's otherwise impossible to test :) -- and this PR is also meant to create an incentive to contribute such ideas to the community.

Also, please note again that this is a completely voluntary effort on my part: I have to be(come) more efficient in how many features/options I keep supporting in my unpaid time. Things are even more acute as I have to now devote (waste) my time to working around (or using their processes to revert) problems created by LinuxFoundation: That org has easily cost (lost) me several hundred work hours this year....

But looking at the code again, it seems I created it such to enable the feature you're asking for @dstebila . It really would be nice if more people were contributing to the project..... I seem to forget more about the code than all other contributors combined added :-/

I haven't had recent contact regarding OID mappings but the relevant documentation in the IETF repo does not list any of the OIDs affected by this PR.

Thanks for that re-confirmation, @praveksharma . Supports my line of thinking.

@SWilson4
Copy link
Member

Did you consider removing our hard-coded OIDs but still letting it be enabled at runtime if the relevant environment variable is set? (Without needing to regenerate from generate.yml and rebuild.)

...

But looking at the code again, it seems I created it such to enable the feature you're asking for @dstebila . It really would be nice if more people were contributing to the project..... I seem to forget more about the code than all other contributors combined added :-/

I confirmed (locally) that the en/decode tests do indeed pass for the NULL-ified KEMs when run with an appropriate environment variable set.

SWilson4
SWilson4 reviewed Sep 20, 2024
View reviewed changes
test/oqs_test_endecode.c Outdated Show resolved Hide resolved
test/oqs_test_endecode.c Outdated
@@ -175,6 +175,11 @@ static int test_oqs_encdec(const char *alg_name) {
if (pkey == NULL)
goto end;

if (!OBJ_sn2nid(alg_name)) {
printf("No OID registered for %s", alg_name);
ok = 1;
Copy link
Member

Choose a reason for hiding this comment

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

May I suggest that we mark these as "skipped" instead of "passed"? We might be grateful for it later if we end up revisiting this code---we'll be able to tell from output alone if the tests are actually running for a given algorithm.

I'll add a suggestion to the test code as well to handle the extra return value.

Suggested change
ok = 1;
ok = -1;

Copy link
Member

Choose a reason for hiding this comment

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

OK, it seems that I can't add a suggestion directly on unmodified lines of code, but we could replace lines 221--231 with

switch(test_oqs_encdec(algs->algorithm_names)) {
    case 1:
        fprintf(stderr,
                cGREEN "  Encoding/Decoding test succeeded: %s" cNORM "\n",
                algs->algorithm_names);
        break;
    case -1:
        fprintf(stderr,
                cBLUE "  Encoding/Decoding test skipped: %s" cNORM "\n",
                algs->algorithm_names);
        break;
    default:
        fprintf(stderr,
                cRED "  Encoding/Decoding test failed: %s" cNORM "\n",
                algs->algorithm_names);
        ERR_print_errors_fp(stderr);
        errcnt++;
        break;
}

Copy link
Member

Choose a reason for hiding this comment

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

If you think this is a good idea, I can commit it directly to the branch to save you the trouble of copy-paste-format.

Copy link
Member

Choose a reason for hiding this comment

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

@baentsch are you OK with my pushing the above code snippet to print "skipped" instead of "passed" on a -1 return code?

baentsch and others added 2 commits September 23, 2024 11:18
@baentsch @SWilson4
Update test/oqs_test_endecode.c
3d5b68e 
Co-authored-by: Spencer Wilson <[email protected]>
Signed-off-by: Michael Baentsch <[email protected]>
@baentsch @SWilson4
Update test/oqs_test_endecode.c
f0fe7d1 
Co-authored-by: Spencer Wilson <[email protected]>
Signed-off-by: Michael Baentsch <[email protected]>
ghen2
ghen2 reviewed Sep 23, 2024
View reviewed changes
ALGORITHMS.md
| p384_mlkem768 | NULL | OQS_OID_P384_MLKEM768
| x448_mlkem768 | NULL | OQS_OID_X448_MLKEM768
| x25519_mlkem768 | NULL | OQS_OID_X25519_MLKEM768
| p256_mlkem768 | NULL | OQS_OID_P256_MLKEM768
Copy link

Choose a reason for hiding this comment

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

You are dropping the two standardized *_mlkem768 hybrids while retaining the unused *_mlkem512 ones, shouldn't it be the other way around?

@baentsch baentsch marked this pull request as draft September 23, 2024 10:56
@baentsch
Copy link
Member Author

baentsch commented Sep 23, 2024 •
edited
Loading

Thanks for this catch @ghen2 -- as per the title of the PR this is not the intention given that generate.yml does contain IDs for x25519_mlkem768 . So need to debug what's going wrong here...

Now I have to correct myself: There is a code point but no OID for x25519_mlkem768. So the logic is correct. @ghen2 Can you please confirm what OID you are aware of for that algorithm? Same with your comment on the MLKEM512 hybrids: They do have code points, so should not be NULL. What leads you to think otherwise?

In other words, the PR is OK.

@ghen2
Copy link

ghen2 commented Sep 23, 2024

OIDs denoted with NULL are not maintained and may lead to errors in code execution. [...]

This comment gave the impression that those algorithms would no longer be supported.
But for OID's this only affects X.509 usage?

@baentsch
Copy link
Member Author

This comment gave the impression that those algorithms would no longer be supported.

Ah; no, this never was the intention. Hence there's a "Yes" for all those algs higher up in the list where the code points are listed.

But for OID's this only affects X.509 usage?

Exactly. No OID means no X.509 storage/retrieval. Rest of functionality unaffected. If you'd have a better way to formulate things, by all means please provide suggested wording.

@baentsch baentsch marked this pull request as ready for review September 23, 2024 15:40
@dstebila
Copy link
Member

I confirmed (locally) that the en/decode tests do indeed pass for the NULL-ified KEMs when run with an appropriate environment variable set.

Thanks, Spencer!

SWilson4 added a commit that referenced this pull request Sep 26, 2024
@SWilson4
Merge KEM OID branch (#522)
a5a7753 
commit c4f6eac
Merge: f0fe7d1 0312c00
Author: Michael Baentsch <[email protected]>
Date:   Mon Sep 23 17:05:42 2024 +0200

    Merge branch 'main' into mb-disabletempoids

    Signed-off-by: Michael Baentsch <[email protected]>

commit f0fe7d1
Author: Michael Baentsch <[email protected]>
Date:   Mon Sep 23 11:19:08 2024 +0200

    Update test/oqs_test_endecode.c

    Co-authored-by: Spencer Wilson <[email protected]>
    Signed-off-by: Michael Baentsch <[email protected]>

commit 3d5b68e
Author: Michael Baentsch <[email protected]>
Date:   Mon Sep 23 11:18:58 2024 +0200

    Update test/oqs_test_endecode.c

    Co-authored-by: Spencer Wilson <[email protected]>
    Signed-off-by: Michael Baentsch <[email protected]>

commit e94338d
Author: Michael Baentsch <[email protected]>
Date:   Sun Sep 15 18:19:33 2024 +0200

    disable tests on no-OID KEMs

    Signed-off-by: Michael Baentsch <[email protected]>

commit a60f6b7
Author: Michael Baentsch <[email protected]>
Date:   Sun Sep 15 17:31:57 2024 +0200

    disable tmp OID generation

    Signed-off-by: Michael Baentsch <[email protected]>
@praveksharma praveksharma mentioned this pull request Sep 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghen2 ghen2 ghen2 left review comments

@SWilson4 SWilson4 SWilson4 left review comments

@dstebila dstebila dstebila approved these changes

@bhess bhess Awaiting requested review from bhess

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mismatched hybrid default OIDs
6 participants
@baentsch @dstebila @christianpaquin @praveksharma @SWilson4 @ghen2