pk: fix return value of curve_name_to_grp_id

Synss / python-mbedtls

Cryptographic library with an mbed TLS back end

MIT License

79 stars 28 forks source link

pk: fix return value of curve_name_to_grp_id #63

Closed isombyt closed 2 years ago

isombyt commented 2 years ago

The PR fulfills these requirements

[ ] This change is a single feature in a single commit.
[x] The commit message follows our commit guidelines.
[x] The code formatting follows our coding guidelines.
[ ] Tests have been added for the changes.
[ ] Docs have been added / updated (docstrings and README).
[ ] A one-liner has been added to the ChangeLog (for new features).

More details in CONTRIBUTING.

I am submitting a …

[x] Bug fix
[ ] New feature
[x] Tests update (new test vector, etc.)
[ ] Docs update
[ ] Other (please complete)

Description

Other information

Synss commented 2 years ago

Hi @isombyt!

Nice catch! It indeed looks like the code was wrong there but some tests do not pass with your patch. Would you mind looking into it? I will investigate on my side as well.

Synss commented 2 years ago

I have just added a couple of patch on the dev branch (will merge them into master when they are green) to help with debugging. Your fix seems to break CURVE448 and CURVE25519. This is not so surprising. I am still working on it.

I can also confirm that, without your patch, ECC.generate() does not use the curve that was configured.

isombyt commented 2 years ago

Didn't expected this would break the unit tests. I'm looking into it.

isombyt commented 2 years ago

refer to upstream code. https://github.com/Mbed-TLS/mbedtls/blob/d65aeb37349ad1a50e0f6c9b694d4b5290d60e49/library/oid.c#L481 mbedtls have not OID definition for x25519 or x448, which means mbedtls does not have support export x25519 or x448 to DER/PEM. In this case, maybe I should fix the unit test.

isombyt commented 2 years ago

update: still need to solve the export format problem. do not merge.

Synss commented 2 years ago

update: still need to solve the export format problem. do not merge.

OK, looks great so far.

isombyt commented 2 years ago

should be good to go.

coveralls commented 2 years ago

Coverage increased (+0.05%) to 86.86% when pulling d88ab2b781cf67bb94d9f49947b6c23fdd0e393f on isombyt:patch-1 into 93e4d3d06425401172ffa92f1a7e5b284f982eea on Synss:master.

Synss commented 2 years ago

Thank you for your very clear contribution! I'll take care of the formatting errors.

Synss commented 2 years ago

merged in master