Skip to content

mbedtls-alt:Support for new algorithms in mbedtls alternative implementation #2621

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.

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

Merged
merged 5 commits into from
Sep 30, 2024

Conversation

ThePassionate
Copy link

@ThePassionate ThePassionate commented Sep 29, 2024

Summary

  1. support 'dup2' to dup a new fd for clone session in crypto driver
  2. add illegal parameter dection for aes
  3. add new algorithms about ripemd160/poly1305/bignum for mbedtls alternative implementation

Impact

N/A

Testing

ci & mbedselftest
nsh> mbedselftest ripemd160 poly1305 mpi

RIPEMD-160 test #1: passed
RIPEMD-160 test #2: passed
RIPEMD-160 test #3: passed
RIPEMD-160 test #4: passed
RIPEMD-160 test #5: passed
RIPEMD-160 test #6: passed
RIPEMD-160 test #7: passed
RIPEMD-160 test #8: passed

Poly1305 test 0 passed
Poly1305 test 1 passed

MPI test #1 (mul_mpi): passed
MPI test #2 (div_mpi): passed
MPI test #3 (exp_mod): passed
MPI test #4 (inv_mod): passed
MPI test #5 (simple gcd): passed

Executed 3 test suites

[ All tests PASS ]
can repeat with
CONFIG_MBEDTLS_POLY1305_ALT
CONFIG_MBEDTLS_RIPEMD160_ALT
CONFIG_MBEDTLS_BIGNUM_ALT
CONFIG_MBEDTLS_APP_SELFTEST
and execute mbedselftest ripemd160 poly1305 mpi

…tions

(1)aes-xts sets the key length to only 256 and 512 bits
(2)when the key length of aes-xts is 512 bits, MAX_KEY_SIZE needs to be expanded to 64 bytes.
(3)check invalid input length and mode

Signed-off-by: makejian <[email protected]>
@nuttxpr
Copy link

nuttxpr commented Sep 29, 2024

[Experimental Bot, please feedback here]

Summary:

The PR summary is generally okay but could be more detailed. For example:

  • Why is 'dup2' support necessary for the crypto driver? What problem does it solve?
  • What kind of illegal parameter detection is being added for AES, and why is it important?
  • Adding new algorithms is a significant change. What are the specific benefits of adding ripemd160/poly1305/bignum for an alternative mbedtls implementation?

Impact:

  • "N/A" is likely not accurate. Even if there's no direct user impact, there are almost always impacts to consider for changes of this nature. Think about:
    • Build: Do the new algorithms or crypto driver changes require any new dependencies or build flags?
    • Hardware: Are these changes specific to certain architectures or boards?
    • Documentation: Will any documentation need to be added or updated to explain the new features or changes?
    • Security: Security-related changes should always be carefully assessed. Are there any potential security implications of the new algorithms or changes to the crypto driver?
    • Compatibility: Will these changes affect compatibility with existing applications or systems using the previous crypto implementation?

Testing:

  • While mentioning CI and test suites is good, providing more specific details will make the review process smoother:
    • Build Host(s): Specify the operating systems, CPUs, and compiler versions used for testing.
    • Target(s): List the specific architectures, boards, and configurations used for testing.
    • Testing Logs: Instead of just saying "testing logs here," provide snippets of relevant logs demonstrating the issue before the change and the successful outcome after the change. Focus on the most critical parts.

Overall:

The PR does not fully meet the NuttX requirements yet. It needs more specific information in the Summary and Impact sections and more detailed evidence of testing.

@cederom
Copy link
Contributor

cederom commented Sep 29, 2024

Thank you @ThePassionate :-) Can you please provide output logs of testing after change with instructions on how to reproduce? :-)

Signed-off-by: makejian <[email protected]>
@ThePassionate
Copy link
Author

Thank you @ThePassionate :-) Can you please provide output logs of testing after change with instructions on how to reproduce? :-)

Thanks, patch has beed updated. please review again. :-)

@xiaoxiang781216 xiaoxiang781216 merged commit e42de5d into apache:master Sep 30, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants