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

driver-only ECC: BN.TLS testing #8008

Merged
merged 5 commits into from
Aug 11, 2023
Merged

driver-only ECC: BN.TLS testing #8008

merged 5 commits into from
Aug 11, 2023

Conversation

valeriosetti
Copy link
Contributor

@valeriosetti valeriosetti commented Jul 31, 2023 •
edited by daverodgman
Loading

This PR is the continuation of #7992 and it enables also TLS and key exchanges as well as ssl-opt testing.

Depends on #7992 and #7999
Resolves #7756

PR checklist

  • changelog will be provided in Driver-only ECC: BN wrap-up #7757
  • backport not required since it's an enhancement
  • tests provided as ecc_no_bignum() components (driver and reference) are both updated in this PR

@valeriosetti valeriosetti self-assigned this Jul 31, 2023
@valeriosetti valeriosetti added needs-work needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Jul 31, 2023
mpg
mpg reviewed Aug 3, 2023
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me except for one minor thing, and also not formally marking approved as it will need removing the temporary commit once 7999 is merge.

tests/ssl-opt.sh Outdated
@@ -2275,6 +2282,7 @@ run_test "Opaque key for server authentication: invalid alg: ECDHE-ECDSA with

requires_config_enabled MBEDTLS_USE_PSA_CRYPTO
requires_config_enabled MBEDTLS_X509_CRT_PARSE_C
requires_config_enabled MBEDTLS_RSA_C
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks redundant with the auto-detecting based on server2 / server7 added above. What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like a left over. It's probably the 1st error which I tried to solve, then I added the general rule while forgetting to remove this line. Since it seems you don't have any major concerns about this PR, if you don't mind, I would like to fix this error once I will do the final rebase after the preceding PRs will be merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to fix this error once I will do the final rebase after the preceding PRs will be merged.

Sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here 522ce71

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, should have mentioned: two other occurrences (lines 2305 and 2325).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I updated the last commit

@valeriosetti valeriosetti removed the needs-ci Needs to pass CI tests label Aug 4, 2023
@tom-daubney-arm tom-daubney-arm self-requested a review August 4, 2023 12:35
@tom-daubney-arm tom-daubney-arm removed the needs-reviewer This PR needs someone to pick it up for review label Aug 7, 2023
@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Aug 8, 2023
@valeriosetti
test: enable TLS, key exchances and ssl-opt teting in ecc_no_bignum()
18535c3 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
ssl_tls13: fix guard for FFDH function
711f853 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
ssl-opt: add RSA_C requirement when RSA encryption is used in certifi…
4f577f3 
…cate

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
analyze_outcomes: skip tests that depend on BIGNUM_C
e0be95e 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
Copy link
Contributor Author

valeriosetti commented Aug 11, 2023 •
edited
Loading

@valeriosetti valeriosetti requested a review from mpg August 11, 2023 04:38
@valeriosetti valeriosetti removed the needs-preceding-pr Requires another PR to be merged first label Aug 11, 2023
mpg
mpg reviewed Aug 11, 2023
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.

The rebase looks good and the temporary commit was removed, but the new commit addressing my previous feedback is incomplete.

@valeriosetti
ssl-opt: remove redundant requirement for RSA_C
36344ce 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti requested a review from mpg August 11, 2023 08:17
mpg
mpg approved these changes Aug 11, 2023
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.

LGTM now, thanks!

tom-daubney-arm
tom-daubney-arm approved these changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@tom-daubney-arm tom-daubney-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.

@tom-daubney-arm tom-daubney-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 11, 2023
@daverodgman daverodgman added this pull request to the merge queue Aug 11, 2023
Merged via the queue into Mbed-TLS:development with commit 963513d Aug 11, 2023
@valeriosetti valeriosetti deleted the issue7756 branch December 6, 2024 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-daubney-arm tom-daubney-arm tom-daubney-arm approved these changes

@mpg mpg mpg approved these changes

Assignees

@valeriosetti valeriosetti

Labels
approved Design and code approved - may be waiting for CI or backports priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

driver-only ECC: BN.TLS testing
4 participants
@valeriosetti @mpg @tom-daubney-arm @daverodgman