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

crypto/x509: ParseRevocationList does not populate Number and AuthorityKeyId fields #53726

Closed
aarongable opened this issue Jul 7, 2022 · 7 comments
Closed

crypto/x509: ParseRevocationList does not populate Number and AuthorityKeyId fields #53726

aarongable opened this issue Jul 7, 2022 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.19

Comments

@aarongable
Copy link
Contributor

What version of Go are you using (go version)?

go1.19rc1

Does this issue reproduce with the latest release?

Yes

What did you do?

Use x509.ParseRevocationList() to parse a CRL which contains the crlNumber and authorityKeyIdentifer extensions (such as one produced using x509.CreateRevocationList()).

See https://go.dev/play/p/6gSi8pmdzBd?v=gotip for a demonstration.

What did you expect to see?

The RevocationList.Number and RevocationList.AuthorityKeyId fields should be populated from the values in their corresponding extensions.

What did you see instead?

The .Number and .AuthorityKeyId fields retain their zero-values.
@gopherbot
Copy link

Change https://go.dev/cl/416354 mentions this issue: crypto/x509: populate Number and AKI of parsed CRLs

@FiloSottile
Copy link
Contributor

@rolandshoemaker this might be worth a freeze exception since it's a new API.

@rolandshoemaker
Copy link
Member

Yup agreed, cc @golang/release this adds expected functionality in a new API that was missing and the patch is minimal.

@rolandshoemaker rolandshoemaker changed the title crypto/x509: ParseRevocationList does not populate Number and AuthorityKeyId fields crypto/x509: ParseRevocationList does not populate Number and AuthorityKeyId fields [freeze exception] Jul 7, 2022
@dmitshur
Copy link
Contributor

dmitshur commented Jul 7, 2022

Note that fixing a bug or problem discovered in a new API thanks to pre-release testing is generally in scope of the freeze (within balance), so a freeze exception might not be needed if you think this fix is okay to accept at this stage.

@heschi heschi added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels Jul 7, 2022
@heschi heschi added this to the Go1.19 milestone Jul 7, 2022
@heschi
Copy link
Contributor

heschi commented Jul 7, 2022

Marking as tentative release-blocker since it's a change to a new API.

@dmitshur
Copy link
Contributor

dmitshur commented Jul 7, 2022

I think we should aim to make it clear whose input an issue in a NeedsDecision state is waiting on.

I think at this point this doesn't warrant a freeze exception (it's within scope) and so the crypto/x509 owners should (i.e., we should remove "[freeze exception]" suffix). Thoughts?

@heschi heschi changed the title crypto/x509: ParseRevocationList does not populate Number and AuthorityKeyId fields [freeze exception] crypto/x509: ParseRevocationList does not populate Number and AuthorityKeyId fields Jul 7, 2022
@heschi
Copy link
Contributor

heschi commented Jul 7, 2022

Works for me; done.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jul 7, 2022
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@aarongable @jproberts
crypto/x509: populate Number and AKI of parsed CRLs
1279e0d 
The x509.RevocationList type has two fields which correspond to
extensions, rather than native fields, of the underlying ASN.1 CRL:
the .Number field corresponds to the crlNumber extension, and
the .AuthorityKeyId field corresponds to the authorityKeyIdentifier
extension.

The x509.CreateRevocationList() function uses these fields to populate
their respective extensions in the resulting CRL. However, the
x509.ParseRevocationList() function does not perform the reverse
operation: the fields retain their zero-values even after parsing a CRL
which contains the relevant extensions.

Add code which populates these fields when parsing their extensions.
Add assertions to the existing tests to confirm that the values are
populated appropriately.

Fixes golang#53726

Change-Id: Ie5b71081e53034e0b5b9ff3c122065c62f15cf23
Reviewed-on: https://go-review.googlesource.com/c/go/+/416354
Run-TryBot: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
@golang golang locked and limited conversation to collaborators Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
6 participants
@FiloSottile @dmitshur @aarongable @rolandshoemaker @gopherbot @heschi