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

Jump to bottom

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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.15

Comments

@FiloSottile
Copy link
Contributor

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 FiloSottile added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal-Crypto Proposal related to crypto packages or other security issues labels Jul 29, 2018
@FiloSottile FiloSottile added this to the Unplanned milestone Jul 29, 2018
@FiloSottile FiloSottile 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. Proposal-Crypto Proposal related to crypto packages or other security issues labels Feb 4, 2019
@FiloSottile FiloSottile modified the milestones: Unplanned, Go1.13 Feb 4, 2019
@FiloSottile
Copy link
Contributor Author

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.

@FiloSottile FiloSottile mentioned this issue Feb 4, 2019
Closed
@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

@FiloSottile Is there something to do here for 1.14?

@lily-commure lily-commure mentioned this issue Jan 3, 2020
Open
@cryslith cryslith mentioned this issue Jan 3, 2020
Closed
@odeke-em
Copy link
Member

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

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

@briansmith
Copy link

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.

@dmitshur dmitshur mentioned this issue Jun 5, 2020
Closed
qinqon added a commit to qinqon/kube-admission-webhook that referenced this issue Oct 14, 2020
@qinqon
Add SubjectKeyId to CA certificate
1c57d6a 
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 qinqon mentioned this issue Oct 14, 2020
Merged
qinqon added a commit to qinqon/kubevirt that referenced this issue Oct 14, 2020
@qinqon
Add Subject Key Id field to CA certificate
6fd8b42 
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 qinqon mentioned this issue Oct 14, 2020
Closed
qinqon added a commit to qinqon/kubevirt that referenced this issue Oct 14, 2020
@qinqon
Add Subject Key Id field to CA certificate
46993a9 
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
@qinqon
Add SubjectKeyId to CA certificate
d2d8ed8 
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
@qinqon
Bump to go 1.15 to get SubjectKeyId at CA certificate
df6ee0e 
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
@qinqon
Bump to go 1.15 to get SubjectKeyId at CA certificate
4b008af 
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 qinqon mentioned this issue Oct 20, 2020
Merged
qinqon added a commit to qinqon/kubemacpool that referenced this issue Oct 20, 2020
@qinqon
Bump to go 1.15 to get SubjectKeyId at CA certificate
d6967c8 
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
@qinqon
Bump to go 1.15 to get SubjectKeyId at CA certificate
5f5d2b1 
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 qinqon mentioned this issue Oct 20, 2020
Merged
qinqon added a commit to qinqon/kubernetes-nmstate that referenced this issue Oct 20, 2020
@qinqon
Bump to go 1.15 to get SubjectKeyId at CA certificate
63641cd 
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
@qinqon
Bump to go 1.15 to get SubjectKeyId at CA certificate
76a3642 
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
@qinqon
Bump to go 1.15 to get SubjectKeyId at CA certificate
eea35d7 
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
@qinqon
Bump to go 1.15 to get SubjectKeyId at CA certificate
90a854f 
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
@qinqon
Bump to go 1.15 to get SubjectKeyId at CA certificate (#247)
4f9729f 
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
@qinqon
Bump to go 1.15 to get SubjectKeyId at CA certificate (#42)
50a6632 
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 pushed a commit to bcrochet/kubernetes-nmstate that referenced this issue Nov 10, 2020
@qinqon @bcrochet
Bump to go 1.15 to get SubjectKeyId at CA certificate
2f6b51f 
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
@qinqon
Bump go to 1.15
5f6598c 
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
@qinqon
Bump go to 1.15 (#612)
79795b8 
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>
@golang golang locked and limited conversation to collaborators Jun 5, 2021
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.
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
8 participants
@briansmith @rsc @andybons @FiloSottile @ianlancetaylor @odeke-em @gopherbot @katiehockman