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

RSA-PSS sign 1: PK #5728

Merged
merged 6 commits into from
Apr 21, 2022
Merged

RSA-PSS sign 1: PK #5728

merged 6 commits into from
Apr 21, 2022

Conversation

superna9999
Copy link
Contributor

@superna9999 superna9999 commented Apr 12, 2022 •
edited
Loading

Description

Opaque RSA keys introduced in #5625 can only do PKCS#1v1.5 signatures, with mbedtls_pk_sign(). This task is to extend them to also support PSS signatures, with mbedtls_pk_sign_ext() (introduced in #5559).

Resolves #5711

Status

READY

Requires Backporting

NO

Migrations

NO

Additional comments

N/A

Todos

  • Tests

Steps to test or reproduce

test_suite_pk must run clean

@superna9999
Allow RSA PK Opaque keys for RSA-PSS signing
7624a5a 
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
@superna9999
Implement PK Opaque RSA PSS signature
62d452b 
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
@superna9999 superna9999 added needs-work needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first Community labels Apr 12, 2022
@superna9999 superna9999 force-pushed the 5711-pk-opaque-rsa-pss-sign branch 2 times, most recently from ea0c59d to f6d7e8b Compare April 12, 2022 14:38
@superna9999
Add RSA PK Wrapped Sign ext tests
999930e 
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
@superna9999 superna9999 force-pushed the 5711-pk-opaque-rsa-pss-sign branch from f6d7e8b to 999930e Compare April 13, 2022 12:56
@superna9999 superna9999 added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-preceding-pr Requires another PR to be merged first needs-work labels Apr 14, 2022
@superna9999 superna9999 marked this pull request as ready for review April 14, 2022 08:41
@superna9999 superna9999 mentioned this pull request Apr 15, 2022
Merged
1 task
@mpg mpg removed the needs-ci Needs to pass CI tests label Apr 15, 2022
@mpg mpg self-requested a review April 15, 2022 11:02
mpg
mpg requested changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Looking mostly good, just two minor points.

library/pk.c Show resolved Hide resolved
library/pk.c Outdated Show resolved Hide resolved
@mprse mprse self-requested a review April 21, 2022 07:33
@mpg mpg mentioned this pull request Apr 21, 2022
Closed
2 tasks
mprse
mprse reviewed Apr 21, 2022
View reviewed changes
Copy link
Contributor

@mprse mprse left a comment

Choose a reason for hiding this comment

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

LGTM.
Left only one suggestion.

@superna9999
Update mbedtls_pk_wrap_as_opaque() public documentation for RSA & RSA…
23143dc 
…-PSS

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
@mpg
Copy link
Contributor

mpg commented Apr 21, 2022

Left only one suggestion.

I'm not sure if something went wrong with github or if it's just me having a brain glitch, but I can't seem to find the suggestion you left ^^

mprse
mprse reviewed Apr 21, 2022
View reviewed changes
tests/suites/test_suite_pk.function Outdated
size_t sig_len, pkey_len;
mbedtls_svc_key_id_t key_id;
unsigned char sig[MBEDTLS_PK_SIGNATURE_MAX_SIZE];
unsigned char pkey[400];
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Can we use macro here instead hard coded value?

@mprse
Copy link
Contributor

mprse commented Apr 21, 2022

I'm not sure if something went wrong with github or if it's just me having a brain glitch, but I can't seem to find the suggestion you left ^^

Your brain is working perfectly. Looks like a problem on my side (forgot to submit this comment).

@superna9999
Reorganize & simplify mbedtls_pk_sign_ext() handling of wrapped RSA-PSS
13e76be 
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
@superna9999
Use macro for public key buffer size in pk_psa_wrap_sign_ext()
843795a 
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
mpg
mpg approved these changes Apr 21, 2022
View reviewed changes
Copy link
Contributor

@mpg mpg 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 addressing our feedback. Looks all good to me now.

@mpg mpg removed the needs-reviewer This PR needs someone to pick it up for review label Apr 21, 2022
@mpg mpg requested a review from mprse April 21, 2022 10:27
@mpg mpg added approved Design and code approved - may be waiting for CI or backports needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests labels Apr 21, 2022
@mpg mpg merged commit 90c7014 into Mbed-TLS:development Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mprse mprse mprse approved these changes

@mpg mpg mpg approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RSA-PSS sign 1: PK
3 participants
@superna9999 @mpg @mprse