Skip to content

Fix d2i_ECPKParameters_fp and i2d_ECPKParameters_fp macroes#16355

Closed
romen wants to merge 8 commits intoopenssl:masterfrom
romen:issues/3.0.0/i2d2i_ecpkparams
Closed

Fix d2i_ECPKParameters_fp and i2d_ECPKParameters_fp macroes#16355
romen wants to merge 8 commits intoopenssl:masterfrom
romen:issues/3.0.0/i2d2i_ecpkparams

Conversation

@romen
Copy link
Member

@romen romen commented Aug 18, 2021 •
edited
Loading

This PR is the master counterpart of #12457, and fixes #12443.

This is the original message for the 1.1.1 commit:

Fix d2i_ECPKParameters_fp and i2d_ECPKParameters_fp macroes

These functions are part of the public API but we don't have tests
covering their usage.
They are actually implemented as macroes and the absence of tests has
caused them to fall out-of-sync with the latest changes to ASN1 related
functions and cause compilation warnings.

This commit fixes the public headers to reflect these changes.

In master we inherited the defect in the public header.

The test for this uncovered a second bug, dealing with curves without associated OIDs.
This PR (as well as #12457) also includes a fix for this second issue.

Tasks

@romen
Add tests for i2d_TYPE_fp and d2i_TYPE_fp
135b515 
These functions are part of the public API but we don't have tests
covering their usage.
They are actually implemented as macroes and the absence of tests has
caused them to fall out-of-sync with the latest changes to ASN1 related
functions and cause compilation warnings.

@@ Note: This commit limits to ECPKParameters as a type.

(cherry picked from commit 52a0e66)
@romen romen added the branch: master Applies to master branch label Aug 18, 2021
@romen romen self-assigned this Aug 18, 2021
@romen
Fix d2i_ECPKParameters_fp and i2d_ECPKParameters_fp macroes
23363f7 
These functions are part of the public API but we don't have tests
covering their usage.
They are actually implemented as macroes and the absence of tests has
caused them to fall out-of-sync with the latest changes to ASN1 related
functions and cause compilation warnings.

This commit fixes the public headers to reflect these changes.

Fixes openssl#12443

(cherry picked from commit 7e1636d)
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Aug 18, 2021
@romen
[ec] Do not default to OPENSSL_EC_NAMED_CURVE for curves without OID
7b34642 
Some curves don't have an associated OID: for those we should not
default to `OPENSSL_EC_NAMED_CURVE` encoding of parameters and instead
set the ASN1 flag to `OPENSSL_EC_EXPLICIT_CURVE`.

This is a follow-up to openssl#12312

(cherry picked from commit b0527a5)
@romen romen mentioned this pull request Aug 18, 2021
Closed
1 task
@romen romen added this to the 3.0.0 milestone Aug 18, 2021
@romen romen added triaged: bug The issue/pr is/fixes a bug hold: discussion The community needs to establish a consensus how to move forward with the issue or PR labels Aug 18, 2021
@romen
Copy link
Member Author

romen commented Aug 18, 2021

The fix touches public API, so mandatory OTC decision label added.

@romen romen changed the title Add tests for i2d_TYPE_fp and d2i_TYPE_fp Fix d2i_ECPKParameters_fp and i2d_ECPKParameters_fp macroes Aug 18, 2021
paulidale
paulidale reviewed Aug 18, 2021
View reviewed changes
@romen
Copy link
Member Author

romen commented Aug 19, 2021

Apart from the ongoing discussion on consistency in handling EC_GROUP_get_asn1_flag() this PR is now stuck (as #12457 for 1.1.1 has been for the past year) on Windows-specific test failures.

I still have the same questions about these failures:

  • are the failures just a peculiarity of our test rigging?
  • are the _fp variants of our functions supposed to work in Windows?
    • if not, should the prototypes for these functions be masked by some #ifdef in the public API?
    • if yes, can we get our testing framework to play nice with whatever needs to happen for these functions to work?
      • if we cannot fix the testing framework, should we disable this specific test for Windows? (what kind of #ifdef are rocmmended for this?)

I don't like the idea of disabling the test in Windows: if those functions are supposed to work in a Windows environment.
It feels like cheating, and the lack of testing of these functions is what made #12443 possible in the first place.

@romen
Use applink to fix windows tests
d1202e8 
(readapted from 5c69c66a6972f84d56160c9ea4b30bab8fc2d3d4 by @bernd-edlinger)
@romen
Copy link
Member Author

romen commented Aug 21, 2021

@levitte can you check if d1202e8 is OK or if I need to tweak it differently?
The original implementation for 1.1.1 was by @bernd-edlinger but in master things changed quite a bit so I had to get creative in my copypaste from apps/build.info.

One thought I had ( which maybe won't work because this whole windows workaround remains a misery to me) was to instead incorporate this in the test static lib rather than each individual binary that might need it.

romen added 2 commits August 23, 2021 12:44
@romen
squash! Add tests for i2d_TYPE_fp and d2i_TYPE_fp
5ba5d65 
@todo: replace "macroes" with "macros" in the commit message
@romen
squash! Fix d2i_ECPKParameters_fp and i2d_ECPKParameters_fp macroes
1158bd1 
@todo: replace "macroes" with "macros" in the commit message
@romen romen marked this pull request as ready for review August 23, 2021 09:48
@romen
Copy link
Member Author

romen commented Aug 23, 2021

The last two commits do not change any code, and are just there as a remainder about fixing the typos in the commit messages when merging.

@romen
Copy link
Member Author

romen commented Aug 24, 2021

@levitte can you check if d1202e8 is OK or if I need to tweak it differently?
The original implementation for 1.1.1 was by @bernd-edlinger but in master things changed quite a bit so I had to get creative in my copypaste from apps/build.info.

One thought I had ( which maybe won't work because this whole windows workaround remains a misery to me) was to instead incorporate this in the test static lib rather than each individual binary that might need it.

Ping @levitte

@mattcaswell
Copy link
Member

mattcaswell commented Aug 24, 2021

OTC: This is fine for inclusion in 3.0 subject to the normal review process.

@mattcaswell mattcaswell removed this from the 3.0.0 milestone Aug 24, 2021
@mattcaswell mattcaswell removed the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Aug 24, 2021
t8m
t8m requested changes Aug 24, 2021
View reviewed changes
@t8m t8m added the approval: done This pull request has the required number of approvals label Aug 26, 2021
@romen
Copy link
Member Author

romen commented Aug 27, 2021

I assume @levitte has no objections regarding #16355 (comment)

@openssl-machine
Copy link
Collaborator

openssl-machine commented Aug 27, 2021

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.

@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Aug 30, 2021
@t8m
Copy link
Member

t8m commented Aug 30, 2021

@romen will you merge this or should I?

@romen
Copy link
Member Author

romen commented Aug 30, 2021

I was planning to do it in the weekend but I had to deal with unexpected things. I will merge it shortly as I had to squash and fix commit messages!

openssl-machine pushed a commit that referenced this pull request Aug 30, 2021
@romen
Add tests for i2d_TYPE_fp and d2i_TYPE_fp
ea1128e 
These functions are part of the public API but we don't have tests
covering their usage.
They are actually implemented as macros and the absence of tests has
caused them to fall out-of-sync with the latest changes to ASN1 related
functions and cause compilation warnings.

@@ Note: This commit limits to ECPKParameters as a type.

Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #16355)
openssl-machine pushed a commit that referenced this pull request Aug 30, 2021
@romen
Fix d2i_ECPKParameters_fp and i2d_ECPKParameters_fp macros
cca8a4c 
These functions are part of the public API but we don't have tests
covering their usage.
They are actually implemented as macros and the absence of tests has
caused them to fall out-of-sync with the latest changes to ASN1 related
functions and cause compilation warnings.

This commit fixes the public headers to reflect these changes.

Fixes #12443

Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #16355)
openssl-machine pushed a commit that referenced this pull request Aug 30, 2021
@romen
[ec] Do not default to OPENSSL_EC_NAMED_CURVE for curves without OID
7aa3dfc 
Some curves don't have an associated OID: for those we should not
default to `OPENSSL_EC_NAMED_CURVE` encoding of parameters and instead
set the ASN1 flag to `OPENSSL_EC_EXPLICIT_CURVE`.

This is a follow-up to #12312

Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #16355)
openssl-machine pushed a commit that referenced this pull request Aug 30, 2021
@romen
Use applink to fix windows tests
7808276 
(readapted from 5c69c66a6972f84d56160c9ea4b30bab8fc2d3d4 by @bernd-edlinger)

Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #16355)
@romen
Copy link
Member Author

romen commented Aug 30, 2021

Merged to master with:

  • ea1128e Add tests for i2d_TYPE_fp and d2i_TYPE_fp
  • cca8a4c Fix d2i_ECPKParameters_fp and i2d_ECPKParameters_fp macros
  • 7aa3dfc [ec] Do not default to OPENSSL_EC_NAMED_CURVE for curves without OID
  • 7808276 Use applink to fix windows tests

Thanks @t8m for the review, @bernd-edlinger to solve the Applink issue!

@romen romen closed this Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulidale paulidale paulidale left review comments

@t8m t8m t8m approved these changes

Assignees

@romen romen

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch severity: fips change The pull request changes FIPS provider sources triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

d2i_ECPKParameters_fp throws lots of warnings

5 participants

@romen @mattcaswell @openssl-machine @t8m @paulidale