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

crypto/x509: add SubjectKeyId automatically when IsCA is true #26676

Closed
FiloSottile opened this issue Jul 29, 2018 · 5 comments
Closed

crypto/x509: add SubjectKeyId automatically when IsCA is true #26676

FiloSottile opened this issue Jul 29, 2018 · 5 comments
Assignees
Labels
Milestone

Comments

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jul 29, 2018

RFC 5280 provides a recommended algorithm to generate the SubjectKeyID, and since these are new public keys, we could use it to set it by default. It's mostly useful for CAs, so we can do it only when IsCA is true. We already automatically set AuthorityKeyID when the parent has SubjectKeyId.

@FiloSottile
Copy link
Member Author

@FiloSottile FiloSottile commented Feb 4, 2019

Section 4.2.1.2 of RFC 5280 says this is a MUST for CAs.

   To facilitate certification path construction, this extension MUST
   appear in all conforming CA certificates, that is, all certificates
   including the basic constraints extension (Section 4.2.1.9) where the
   value of cA is TRUE. 
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.14 Oct 23, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 5, 2019

@FiloSottile Is there something to do here for 1.14?

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jan 27, 2020

Might be too late to do anything for Go1.14 as the bell tolls. I shall move this to the backlog.

@odeke-em odeke-em modified the milestones: Go1.14, Go1.15, Backlog Jan 27, 2020
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.15 Feb 19, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 3, 2020

Change https://golang.org/cl/227098 mentions this issue: crypto/x509: generate SubjectKeyId for CAs

@gopherbot gopherbot closed this in 6f3a951 Apr 13, 2020
@briansmith
Copy link

@briansmith briansmith commented Jun 5, 2020

I understand the use of SHA-1 here isn't a security issue, and I understand that the RFC gives examples of using SHA-1 for this, but still the use of SHA-1 seems gratuitous given no unmentioned external constraints. I do think it is better to limit SHA-1 use at this point to uses that cannot be reasonable avoided so that at some point in the future we can get rid of SHA-1 and/or get rid of a bunch of code for optimizing SHA-1.

RFC 5280 provides a recommended algorithm to generate the SubjectKeyID

NIT: RFC 5280 doesn't recommend an algorithm to generate the SubjectKeyID. It seems to be carefully worded to let us know that the SHA-1-based approach is commonly used, without recommending its use.

In this case, using any any stronger hash function convenient, e.g. SHA-256 or SHA-512, and truncating to 20 bytes, would be good enough and IMO better.

One potential motivation for using SHA-1 would be to help a CA migrate to using this implementation from an implementation that was already using SHA-1, but that motivation isn't mentioned in the code, so I assume it isn't there.

It will be more difficult to change the algorithm used this ships in a release so it's worth reconsidering it before the next release, if it hasn't happened yet.

qinqon added a commit to qinqon/kube-admission-webhook that referenced this issue Oct 14, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] this PR replicate the golang >= 1.15 fix [2]
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kubevirt that referenced this issue Oct 14, 2020
The SKI field is mandatory for CA certificates, but Go crypto library for versions < 1.15
is not filling in the SKI field for CAs [1] this PR replicate the golang >= 1.15 fix [2].

Replicated from qinqon/kube-admission-webhook#42

[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kubevirt that referenced this issue Oct 14, 2020
The SKI field is mandatory for CA certificates, but Go crypto library for versions < 1.15
is not filling in the SKI field for CAs [1] this PR replicate the golang >= 1.15 fix [2].

Replicated from qinqon/kube-admission-webhook#42

[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kube-admission-webhook that referenced this issue Oct 15, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] this PR replicate the golang >= 1.15 fix [2]
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kube-admission-webhook that referenced this issue Oct 20, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kubernetes-nmstate that referenced this issue Oct 20, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kubemacpool that referenced this issue Oct 20, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kube-admission-webhook that referenced this issue Oct 20, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kubernetes-nmstate that referenced this issue Oct 20, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kubemacpool that referenced this issue Oct 20, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kubernetes-nmstate that referenced this issue Oct 20, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kube-admission-webhook that referenced this issue Oct 20, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
kubevirt-bot pushed a commit to k8snetworkplumbingwg/kubemacpool that referenced this issue Oct 20, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kube-admission-webhook that referenced this issue Oct 26, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
bcrochet added a commit to bcrochet/kubernetes-nmstate that referenced this issue Nov 10, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15
[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/kubernetes-nmstate that referenced this issue Nov 10, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15

[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
kubevirt-bot pushed a commit to nmstate/kubernetes-nmstate that referenced this issue Nov 10, 2020
Go crypto library for versions < 1.15 is not filling in the SKI field for CAs [1] so we bump to 1.15

[1] golang/go#26676
[2] https://go-review.googlesource.com/c/go/+/227098/

Signed-off-by: Quique Llorente <ellorent@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.