Skip to content

ecdsa: fix -missing-prototypes warning when MBEDTLS_ECDSA_SIGN_ALT is defined #7415

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
Merged
merged 1 commit into from
Apr 17, 2023
Merged

ecdsa: fix -missing-prototypes warning when MBEDTLS_ECDSA_SIGN_ALT is defined #7415

merged 1 commit into from
Apr 17, 2023

Conversation

Harshal5
Copy link
Contributor

@Harshal5 Harshal5 commented Apr 7, 2023 •
edited by ronald-cron-arm
Loading

Description

Gatekeeper checklist

  • changelog required, bug fix
  • backport not required, fixes an issue introduced in 3.4
  • tests not required

yuhaoth
yuhaoth suggested changes Apr 10, 2023
View reviewed changes
Copy link
Contributor

@yuhaoth yuhaoth left a comment

Choose a reason for hiding this comment

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

Please add * \param md_alg The hash algorithm used to hash the original data. after line 325 to fix CI failure.

The ci failure for platform test should be ignore. It is not due to this PR.

@Harshal5 Harshal5 force-pushed the fix/declaration_of_mbedtls_ecdsa_sign_det_restartable_function branch from f1d380b to 8eb7155 Compare April 11, 2023 11:05
@ronald-cron-arm ronald-cron-arm self-requested a review April 11, 2023 12:03
@ronald-cron-arm ronald-cron-arm added bug component-crypto Crypto primitives and low-level interfaces priority-high High priority - will be reviewed soon labels Apr 11, 2023
@Harshal5 Harshal5 requested a review from yuhaoth April 11, 2023 15:14
@Harshal5
Copy link
Contributor Author

Please add * \param md_alg The hash algorithm used to hash the original data. after line 325 to fix CI failure.

The ci failure for platform test should be ignore. It is not due to this PR.

I have added the above line. PTAL.

yuhaoth
yuhaoth previously approved these changes Apr 12, 2023
View reviewed changes
Copy link
Contributor

@yuhaoth yuhaoth left a comment

Choose a reason for hiding this comment

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

Thanks your contribution, @Harshal5 . It looks good to me

ronald-cron-arm
ronald-cron-arm requested changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

This looks good to me but as it is a bug fix I think we need a change log. If you are not familiar with Mbed TLS change logs, please have a look to ChangeLog.d/00README.md.

@Harshal5 Harshal5 force-pushed the fix/declaration_of_mbedtls_ecdsa_sign_det_restartable_function branch from 8eb7155 to 07c9321 Compare April 15, 2023 09:05
@Harshal5
Copy link
Contributor Author

Harshal5 commented Apr 15, 2023 •
edited
Loading

This looks good to me but as it is a bug fix I think we need a change log. If you are not familiar with Mbed TLS change logs, please have a look to ChangeLog.d/00README.md.

PTAL. I have updated the change logs.
Also, could you please let me know what would be the timeline of the next mbedtls release which will include this fix?

@yuhaoth
Copy link
Contributor

yuhaoth commented Apr 17, 2023

PTAL. I have updated the change logs. Also, could you please let me know what would be the timeline of the next mbedtls release which will include this fix?

Next quarter, maybe . I am not very sure, that depends on the situation.

And there is CI failure on Change log file

yuhaoth
yuhaoth suggested changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@yuhaoth yuhaoth left a comment

Choose a reason for hiding this comment

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

A nitpick comments. Others looks good to me.

@Harshal5 Harshal5 force-pushed the fix/declaration_of_mbedtls_ecdsa_sign_det_restartable_function branch 2 times, most recently from f4d18c1 to 56f3660 Compare April 17, 2023 07:12
ronald-cron-arm
ronald-cron-arm requested changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

Minor suggestions for the change log otherwise this looks good to me.

@Harshal5
ecdsa: fix -missing-prototypes warning when `MBEDTLS_ECDSA_SIGN_ALT…
8c77644 
…` is defined

- In `mbedtls/v3.4.0`, ECDSA restartable sign and verify functions (`ecdsa.c`) were made public.
- But the `mbedtls_ecdsa_sign_det_restartable` function prototype was declared in the file `ecdsa.h`,
  only when `MBEDTLS_ECDSA_SIGN_ALT` is not defined.

Signed-off-by: harshal.patil <harshal.patil@espressif.com>
@Harshal5 Harshal5 force-pushed the fix/declaration_of_mbedtls_ecdsa_sign_det_restartable_function branch from 56f3660 to 8c77644 Compare April 17, 2023 07:23
ronald-cron-arm
ronald-cron-arm approved these changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

yuhaoth
yuhaoth approved these changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@yuhaoth yuhaoth left a comment

Choose a reason for hiding this comment

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

LGTM

@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Apr 17, 2023
Open
@ronald-cron-arm ronald-cron-arm merged commit f54762e into Mbed-TLS:development Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

+1 more reviewer

@yuhaoth yuhaoth yuhaoth approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
bug component-crypto Crypto primitives and low-level interfaces priority-high High priority - will be reviewed soon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build warning when MBEDTLS_ECDSA_SIGN_ALT is defined
3 participants
@Harshal5 @yuhaoth @ronald-cron-arm