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

Remove non-working check from x509_get_subject_alt_name #2855

Merged
merged 3 commits into from
May 26, 2020
Merged

Remove non-working check from x509_get_subject_alt_name #2855

merged 3 commits into from
May 26, 2020

Conversation

irwir
Copy link
Contributor

@irwir irwir commented Sep 21, 2019 •
edited by RonEld
Loading

Description

That fixes #2802
The changed comment might also be changed by @hanno-arm in his PR.

Status

READY

Requires Backporting

NO
Which branch?
Development

Migrations

NO

@RonEld RonEld added bug component-x509 needs-review Every commit must be reviewed by at least two team members, labels Sep 26, 2019
RonEld
RonEld previously approved these changes Sep 26, 2019
View reviewed changes
@k-stachowiak k-stachowiak self-requested a review October 23, 2019 15:06
k-stachowiak
k-stachowiak previously approved these changes Oct 23, 2019
View reviewed changes
Copy link
Contributor

@k-stachowiak k-stachowiak left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Looks good.

library/x509_crt.c
@@ -662,7 +658,7 @@ static int x509_get_subject_alt_name( unsigned char **p,
}

/*
* Check that the SAN are structured correct.
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is outside of the PRs scope. I'd suggest not mixing in extra changes with the main ones. But this is far from being a blocker.

@irwir
Copy link
Contributor Author

irwir commented Oct 29, 2019

The state is approved, but needs: review tag is still present.
Does it need more than 2 reviews?

@Patater Patater added needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 29, 2019
piotr-now
piotr-now previously approved these changes Dec 12, 2019
View reviewed changes
@gilles-peskine-arm gilles-peskine-arm added needs: changelog and removed needs-ci Needs to pass CI tests labels Apr 2, 2020
@gilles-peskine-arm
Copy link
Contributor

This is a minor improvement that doesn't require backporting. It does require a changelog. Can you please rebase this PR on top of development and add a .txt file in ChangeLog.d? Suggested content:

Bugfix
   * Remove dead code in X.509 certificate parsing. Contributed by irwir in #2855.

@irwir irwir dismissed stale reviews from piotr-now, k-stachowiak, and RonEld via b94f9c6 April 18, 2020 20:55
@irwir irwir force-pushed the fix_x509_crt.c branch 3 times, most recently from a9783de to f8c6ea6 Compare April 18, 2020 21:13
@irwir
Copy link
Contributor Author

irwir commented Apr 27, 2020

It does require a changelog.

Thanks.
Changelog entry was added as a separate commit.

@danh-arm
Copy link
Contributor

Hi @irwir . Sorry this PR stalled. Unfortunately you're change log entry filename conflicts with another one. Can you use a name that uniquely identifies this PR? Thanks.

@irwir
Add changelog entry
d742a24 
Signed-off-by: irwir <[email protected]>
@irwir irwir force-pushed the fix_x509_crt.c branch from 240ba49 to d742a24 Compare May 20, 2020 15:25
@irwir
Copy link
Contributor Author

irwir commented May 20, 2020

Altered the file name.
I expected that the entries were to be added automatically for every merged PR, then file names were irrelevant. But apparently this is (yet?) not so.

@danh-arm
Copy link
Contributor

danh-arm commented May 20, 2020 •
edited
Loading

Altered the file name.

Thanks.

I expected that the entries were to be added automatically for every merged PR, then file names were irrelevant. But apparently this is (yet?) not so.

The entries are only collated at release time so they must all be unique between releases. I accept the guidance needs improvement.

@danh-arm danh-arm self-assigned this May 20, 2020
ronald-cron-arm
ronald-cron-arm approved these changes May 26, 2020
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-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

@gilles-peskine-arm gilles-peskine-arm merged commit b1ccff8 into Mbed-TLS:development May 26, 2020
@irwir irwir deleted the fix_x509_crt.c branch October 6, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@danh-arm danh-arm danh-arm approved these changes

@piotr-now piotr-now piotr-now left review comments

@RonEld RonEld RonEld left review comments

@k-stachowiak k-stachowiak k-stachowiak left review comments

Assignees

@danh-arm danh-arm

Labels
bug component-x509
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Condition always false in x509_get_subject_alt_name
8 participants
@irwir @gilles-peskine-arm @danh-arm @k-stachowiak @RonEld @ronald-cron-arm @piotr-now @Patater