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.

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

[Backport 2.16] Mark basic constraints critical as appropriate. #4044

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

darrenkrahn
Copy link
Contributor

Per RFC 5280 4.2.1.9 if the 'cA' field is set to true, the extension
must be marked critical.

Signed-off-by: Darren Krahn dkrahn@google.com

Description

This is a backport to 2.16 of #3698.

Status

READY

Per RFC 5280 4.2.1.9 if the 'cA' field is set to true, the extension
must be marked critical.

Signed-off-by: Darren Krahn <dkrahn@google.com>
@daverodgman daverodgman added bug Community component-x509 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 17, 2021
@darrenkrahn
Copy link
Contributor Author

@hanno-arm can you review?

Copy link

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @darrenkrahn

@hanno-becker hanno-becker removed the needs-review Every commit must be reviewed by at least two team members, label Feb 8, 2021
@darrenkrahn
Copy link
Contributor Author

@daverodgman anything else we need before this can be merged?

@gilles-peskine-arm gilles-peskine-arm added needs: changelog and removed needs-reviewer This PR needs someone to pick it up for review labels Feb 25, 2021
@gilles-peskine-arm
Copy link
Contributor

@darrenkrahn This is approved and CI is passing so in principle it's ready for merge, however as a gatekeeper I find it surprising to have what looks like a bug fix in a long-time support branch without a changelog entry describing the impact of the bug. Can you please summarize the bug and its impact in a few words (is it just a compliance issue? Can it permit exploits in common scenarios?) and add a file in ChangeLog.d (see others and the main ChangeLog file for examples)? And once the changelog entry has been reviewed, please create a new pull request in development to add the changelog entry.

@darrenkrahn
Copy link
Contributor Author

Yes, this is just a compliance issue; specifically when creating CA certs. For motivation on why one may want to do this in an embedded environment see https://trustedcomputinggroup.org/resource/dice-layering-architecture/.

I missed the ChangeLog requirement, I'll take a look. FWIW, I don't feel strongly about whether or not to backport this fix, if you'd like to reverse the decision and abandon this PR I'm fine with that.

davidhorstmann-arm added a commit to davidhorstmann-arm/mbedtls that referenced this pull request Aug 24, 2021
The change made by PR Mbed-TLS#4044 was previously advertised in the
2.16.10 ChangeLog, however Mbed-TLS#4044 had not yet been merged.
Create a new entry for Mbed-TLS#4044, with a note that the previous
entry was in error.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
@yanesca
Copy link
Contributor

yanesca commented Aug 26, 2021

The CI passed 5 months ago: re-running the merge job to get current results.

@yanesca yanesca added needs-ci Needs to pass CI tests and removed needs: changelog labels Aug 26, 2021
@yanesca
Copy link
Contributor

yanesca commented Aug 26, 2021

@yanesca yanesca merged commit f1b0c70 into Mbed-TLS:mbedtls-2.16 Aug 26, 2021
@yanesca yanesca removed the needs-ci Needs to pass CI tests label Aug 26, 2021
yanesca added a commit that referenced this pull request Aug 26, 2021
Create ChangeLog entry correcting the record on #4044
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Basic constraints extension fix in x509 code was not ported to 2.16 as believed.
6 participants