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

configure: introduce no-ecx to remove ECX related feature #20781

Closed
wants to merge 1 commit into from
Closed

configure: introduce no-ecx to remove ECX related feature #20781

wants to merge 1 commit into from

Conversation

liyi77
Copy link
Contributor

@liyi77 liyi77 commented Apr 20, 2023

Users who are sensitive to size can use this to remove the code related to edward curve.

Will reduce ~120KB binaries size when building crypto library in EDK2(original size is 1578KB).

Checklist
  • documentation is added or updated
  • tests are added or updated

@t8m
Copy link
Member

t8m commented Apr 20, 2023

The curve type is Edwards not Edward. However you're removing not just the EdDSA but also the Montgomery X curves. So I think the build option should be named no-ecx instead as ECX is the name used to refer to these curve implementations in the sources.

@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Apr 20, 2023
@t8m
Copy link
Member

t8m commented Apr 20, 2023

This also needs testing in form of runchecker no-ecx build in the run-checker-merge.yml

@liyi77
Copy link
Contributor Author

liyi77 commented Apr 21, 2023

The curve type is Edwards not Edward. However you're removing not just the EdDSA but also the Montgomery X curves. So I think the build option should be named no-ecx instead as ECX is the name used to refer to these curve implementations in the sources.

Thanks for review.
Based on feedback I will:

  1. rename 'no-edward' to 'no-ecx'.
  2. add testing of no-ecx build in the run-checker-merge.yml

I am not familiar with testing, does it means add 'no-ecx' to

openssl/.github/workflows/run-checker-daily.yml

Lines 22 to 27 in a901b31

opt: [
386,
no-afalgeng,
no-aria,
no-asan,
no-asm,
?

@paulidale
Copy link
Contributor

Yes, add a line there and it will be tested.

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Apr 21, 2023
@t8m
Copy link
Member

t8m commented Apr 21, 2023

I am not familiar with testing, does it means add 'no-ecx' to

Please add it at the similar place in run-checker-merge.yml instead of run-checker-daily.yml.

@liyi77 liyi77 changed the title configure: introduce no-edward to remove edward curve related feature configure: introduce no-ecx to remove ECX related feature Apr 24, 2023
@liyi77
Copy link
Contributor Author

liyi77 commented Apr 24, 2023

Updated, please take a look

t8m
t8m requested changes Apr 24, 2023
View reviewed changes
apps/list.c Outdated Show resolved Hide resolved
crypto/asn1/standard_methods.h Outdated Show resolved Hide resolved
crypto/evp/p_lib.c Outdated Show resolved Hide resolved
crypto/evp/pmeth_lib.c Show resolved Hide resolved
include/crypto/ecx.h Outdated Show resolved Hide resolved
test/recipes/80-test_ssl_new.t Outdated Show resolved Hide resolved
test/ssl-tests/28-seclevel.cnf.in Outdated Show resolved Hide resolved
test/sslapitest.c Outdated Show resolved Hide resolved
test/sslapitest.c Outdated Show resolved Hide resolved
test/tls13ccstest.c Outdated Show resolved Hide resolved
@liyi77 liyi77 force-pushed the add-no-edward branch 4 times, most recently from 4569a01 to 632f83a Compare April 25, 2023 05:43
@liyi77 liyi77 mentioned this pull request May 5, 2023
Closed
@liyi77 liyi77 force-pushed the add-no-edward branch 3 times, most recently from a800dbc to 4eedabf Compare May 16, 2023 03:21
@liyi77 liyi77 closed this May 16, 2023
@liyi77 liyi77 reopened this May 16, 2023
@liyi77
Copy link
Contributor Author

liyi77 commented May 16, 2023

Thanks for review, I resolved previous comments, with the following changes:

  1. When ECX is disabled, the default KEMID of HPKE is changed to P256.
  2. When ECX is disabled, the ecdh key_share group defaults to P256, which will cause some test cases to fail to trigger HRR. So replace P256 with P384 in these cases.
  3. Divide evppkey_mismatch.txt into two parts: ecx and no-ecx.

Patch updated.

@t8m 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 tests: present The PR has suitable tests present labels May 16, 2023
@liyi77
Copy link
Contributor Author

liyi77 commented Jun 7, 2023

++visibility
Hello, any comments here?

@t8m
Copy link
Member

t8m commented Jun 7, 2023

I see this failure in your on-push actions: https://github.com/liyi77/openssl/actions/runs/4988224062/jobs/8930739805 could you please resolve it?

@liyi77
Copy link
Contributor Author

liyi77 commented Jun 7, 2023

Oh I didn't realize there were some tests here, I'll fix it ASAP

@liyi77
configure: introduce no-ecx to remove ECX related feature
56954ef 
This can effectively reduce the binary size for platforms
that don't need ECX feature(~100KB).

Signed-off-by: Yi Li <yi1.li@intel.com>
@liyi77
Copy link
Contributor Author

liyi77 commented Jun 8, 2023

This error is because a defined but not used function in test/evp_pkey_provided_test.c, fixed:
int test_print_key_using_encoder_public()
https://github.com/liyi77/openssl/actions/runs/5207464163/jobs/9395036607

Maybe to demonstrate that everything works, could you please add no-ecx run to the run-checker-ci.yml as a separate commit with drop! annotation?

I'm not sure if I understand it correctly, does it mean: liyi77#5

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Jun 9, 2023
@paulidale 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 Jun 13, 2023
@liyi77
Copy link
Contributor Author

liyi77 commented Jun 13, 2023

Hello, can this PR be applied to openssl3.0 or openssl3.1?

@paulidale
Copy link
Contributor

As per our stable releases policy, without an OTC exception, only bug fixes can be back ported.

Make a case for this being either a bug or sufficiently worthy for back port as a feature and the OTC can make a decision.

@liyi77
Copy link
Contributor Author

liyi77 commented Jun 13, 2023

Make a case for this being either a bug or sufficiently worthy for back port as a feature and the OTC can make a decision.

For users who don't need ECX, this feature can save ~120KB, about 8% (tested with UEFI system, compiled with VS2019).
This is an optimization worth considering. I also noticed that in the community, some users have mentioned concerns about the size of openssl3.0, more options are always better.
#13219
#13270

@paulidale paulidale added the hold: need otc decision The OTC needs to make a decision label Jun 13, 2023
@paulidale
Copy link
Contributor

OTC: Back port to 3.1 and/or 3.0 or not?

@t8m
Copy link
Member

t8m commented Jun 13, 2023

OTC: We are disinclined to backport this to 3.0, 3.1. However we are open to additional community feedback influencing this decision.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@paulidale
Copy link
Contributor

Merged to master, thanks for the contribution.

openssl-machine pushed a commit that referenced this pull request Jun 14, 2023
@liyi77 @paulidale
configure: introduce no-ecx to remove ECX related feature
4032cd9 
This can effectively reduce the binary size for platforms
that don't need ECX feature(~100KB).

Signed-off-by: Yi Li <yi1.li@intel.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20781)
@t8m t8m closed this Jun 14, 2023
@mattcaswell
Copy link
Member

Errr....was this closed prematurely? As I recall OTC were ok with master, but the 3.1/3.0 backport still has an OTC hold on it...

@t8m t8m reopened this Jun 14, 2023
@t8m
Copy link
Member

t8m commented Jun 14, 2023

OK, reopening.

@t8m t8m added branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 and removed branch: master Merge to master branch labels Jun 14, 2023
@github-actions github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label Jun 14, 2023
@paulidale
Copy link
Contributor

Errr....was this closed prematurely? As I recall OTC were ok with master, but the 3.1/3.0 backport still has an OTC hold on it...

That's why I left it open after merging 😺

@paulidale paulidale mentioned this pull request Jun 27, 2023
Open
@t8m
Copy link
Member

t8m commented Jul 4, 2023

OTC: We will not backport this to 3.1, 3.0 now.

@t8m t8m closed this Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FdaSilvaYY FdaSilvaYY FdaSilvaYY left review comments

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 hold: need otc decision The OTC needs to make a decision tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@liyi77 @t8m @paulidale @openssl-machine @mattcaswell @FdaSilvaYY