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

Improve X.509 cert writing serial number management #6883

Merged
merged 17 commits into from
Jan 30, 2023
Merged

Improve X.509 cert writing serial number management #6883

merged 17 commits into from
Jan 30, 2023

Conversation

valeriosetti
Copy link
Contributor

@valeriosetti valeriosetti commented Jan 5, 2023 •
edited
Loading

Description

The goal of this PR is to remove any direct dependency of X509 certificate writing from BIGNUM_C.

Resolves #6843

Gatekeeper checklist

@valeriosetti valeriosetti self-assigned this Jan 5, 2023
@valeriosetti valeriosetti added enhancement needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) labels Jan 5, 2023
@mpg mpg mentioned this pull request Jan 9, 2023
Closed
mpg
mpg requested changes Jan 9, 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.

I think there was a misunderstanding about the intended API of the new function. Also, a bit more work is needed to fully handle the deprecation, and as you correctly pointed out, our code was not compliant before this PR, so there's also a bit more work that comes with fixing that bug!

@mpg mpg added bug needs-work needs-backports Backports are missing or are pending review and approval. and removed needs-review Every commit must be reviewed by at least two team members, labels Jan 9, 2023
@valeriosetti valeriosetti force-pushed the issue6843 branch 4 times, most recently from f0f6b57 to fda367e Compare January 10, 2023 07:56
mpg
mpg requested changes Jan 10, 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.

This is going in the right direction, the two main things remaining are the serial argument of the example program, and encoding of of numbers that would look negative. For that part, I think (but didn't check) that we might have a test gap, as it looks "01" is the only value of serial that we are testing. If that's the case, please add tests with other relevant serial values.

@mpg
Copy link
Contributor

mpg commented Jan 12, 2023

Note to self: I've reviewed up to 679474d included.

If you want, feel free to restyle your branch and force-push anytime to resolve the conflict. I've make a local copy of what I've reviewed to far, so this won't impact the continuity of my review.

@valeriosetti
x509: remove direct dependency from BIGNUM_C
da0afcc 
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
Changelog: list changes in x509write_crt module
903b6aa 
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
fix: add missing deprecation guards
5d164c4 
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
test: ensure X509 has no dependency on BIGNUM when built without MBED…
41b5fb6 
…TLS_DEPRECATED_REMOVED

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
x509: fix endianness and input data format for x509write_crt_set_seri…
acf12fb 
…al_new

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
test: fix tests for x509write_crt_set_serial(_new)
aad8dbd 
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
changelog: fixed typos
ea19d2d 
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
x509: renaming of buffer variables in new serial setting function
746def5 
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
test: fix: remove invalid comment
b8dc18f 
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
x509: enhancement and fixes
4752aac 
- enhance mbedtls_x509write_crt_set_serial(): avoid use of useless
  temporary buffer

- fix mbedtls_x509write_crt_der(): add an extra 0x00 byte at the
  beginning of serial if the MSb of serial is 1, as required from
  ASN.1

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels Jan 13, 2023
@AndrzejKurek AndrzejKurek self-requested a review January 26, 2023 13:27
AndrzejKurek
AndrzejKurek suggested changes Jan 26, 2023
View reviewed changes
Copy link
Contributor

@AndrzejKurek AndrzejKurek left a comment

Choose a reason for hiding this comment

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

Some changes requested.
It would be great to add some tests using mbedtls_x509_crt_info and check that the set serial number is then properly printed (and test it), but I'm not sure if it isn't a bit too much. Wdyt, @mpg?

@valeriosetti
x509: replace/fix name of new function for setting serial
af4815c 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti mentioned this pull request Jan 26, 2023
Merged
3 tasks
@valeriosetti
Copy link
Contributor Author

It would be great to add some tests using mbedtls_x509_crt_info and check that the set serial number is then properly printed (and test it)

I also added some crt files which were generated by openssl and not the internal cert_write app. These were meant to test all the corner cases that arouse during the development of this PR.
I thought this should guarantee/prove the accuracy of the code in a more independent fashion since both certificates generated by openssl and mbedtls are then memcmp-ed in the end.
Wdyt?

mpg
mpg reviewed Jan 27, 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.

Looking good to me, but I seem to always want more test cases :)

@valeriosetti
fixing a typo in comment
9b5e1da 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
test: improve error handling in x509_set_serial_check()
a87f839 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
test: add test for a full length serial of 0xFF
18b9b03 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
mpg
mpg approved these changes Jan 27, 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

AndrzejKurek
AndrzejKurek reviewed Jan 27, 2023
View reviewed changes
include/mbedtls/x509_crt.h
* \param ctx CRT context to use
* \param serial A raw array of bytes containing the serial number in big
* endian format
* \param serial_len Length of valid bytes (expressed in bytes) in \p serial
Copy link
Contributor

Choose a reason for hiding this comment

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

Length of valid bytes (expressed in bytes) is redundant, but that's just a minor.

AndrzejKurek
AndrzejKurek suggested changes Jan 27, 2023
View reviewed changes
Copy link
Contributor

@AndrzejKurek AndrzejKurek left a comment

Choose a reason for hiding this comment

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

The only blocker for me is the fact that newly added lines 134 - 137 of x509write_crt.c are not covered by any test.

AndrzejKurek
AndrzejKurek approved these changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@AndrzejKurek AndrzejKurek left a comment

Choose a reason for hiding this comment

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

OpenCI seems to have a spurious failure. Thanks for addressing all the comments!

@AndrzejKurek AndrzejKurek 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, needs-reviewer This PR needs someone to pick it up for review labels Jan 30, 2023
@mpg
Copy link
Contributor

mpg commented Jan 30, 2023

These changes the ABI but not the API, as the changes fields were private, so the failure of ABI-API-checking is acceptable.

@mpg mpg removed the needs-backports Backports are missing or are pending review and approval. label Jan 30, 2023
@mpg mpg merged commit aae6125 into Mbed-TLS:development Jan 30, 2023
@valeriosetti valeriosetti deleted the issue6843 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

@mpg mpg mpg approved these changes

@AndrzejKurek AndrzejKurek AndrzejKurek approved these changes

Assignees

@valeriosetti valeriosetti

Labels
approved Design and code approved - may be waiting for CI or backports bug enhancement size-s Estimated task size: small (~2d)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve X.509 cert writing serial number management
3 participants
@valeriosetti @mpg @AndrzejKurek