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

Duplicate lints about keyIdentifier in certificates #726

Merged
merged 10 commits into from
Jul 9, 2023
Merged

Duplicate lints about keyIdentifier in certificates #726

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
23def3d
Duplicate lints about keyIdentifier in certificates
christopher-henderson Jun 11, 2023
8a4fd62
fixed go imports styling
christopher-henderson Jun 11, 2023
88b9816
breaking up code comments to match conditional blocks
christopher-henderson Jun 11, 2023
a9d76ca
typo
christopher-henderson Jun 11, 2023
03efc09
Merge branch 'master' into duplicate_key_identifier
zakird Jun 11, 2023
1f42efd
simplifying check
christopher-henderson Jun 25, 2023
4666dd3
Merge branch 'master' into duplicate_key_identifier
christopher-henderson Jun 25, 2023
1fdf19b
Triggering GHA with empty commit
christopher-henderson Jun 25, 2023
1500060
adding one more error cert to the corpus
christopher-henderson Jul 9, 2023
f29cf02
Merge branch 'master' into duplicate_key_identifier
christopher-henderson Jul 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 0 additions & 65 deletions v3/lints/rfc/lint_ext_authority_key_identifier_missing.go
View file
Open in desktop

This file was deleted.

40 changes: 0 additions & 40 deletions v3/lints/rfc/lint_ext_authority_key_identifier_missing_test.go
View file
Open in desktop

This file was deleted.

20 changes: 19 additions & 1 deletion v3/lints/rfc/lint_ext_authority_key_identifier_no_key_identifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ package rfc
*/

import (
"bytes"

"github.com/zmap/zcrypto/x509"
"github.com/zmap/zlint/v3/lint"
"github.com/zmap/zlint/v3/util"
Expand Down Expand Up @@ -57,7 +59,23 @@ func (l *authorityKeyIdNoKeyIdField) CheckApplies(c *x509.Certificate) bool {
}

func (l *authorityKeyIdNoKeyIdField) Execute(c *x509.Certificate) *lint.LintResult {
if c.AuthorityKeyId == nil && !util.IsSelfSigned(c) { //will be nil by default if not found in x509.parseCert
if util.IsCACert(c) && util.IsSelfSigned(c) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can remove util.IsCACert(c)? Only if it is self-signed it may be omitted.

Copy link
Member Author

@christopher-henderson christopher-henderson Jun 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not quite convinced of that based on the language.

There is one exception; where a CA distributes its public key in the form of a "self-signed" certificate, the authority key identifier MAY be omitted.

It seems to me that this clause is stating that this does apply, specifically to CA certs and not just to any self signed.

Although what exists in practice may of course differ.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is fine.

if c.AuthorityKeyId == nil {
// There is one exception; where a CA distributes its public key in the form of a "self-signed"
// certificate, the authority key identifier MAY be omitted.
return &lint.LintResult{Status: lint.Pass}
} else if !bytes.Equal(c.AuthorityKeyId, c.SubjectKeyId) {
christopher-henderson marked this conversation as resolved.
Show resolved Hide resolved
// In this case, the subject and authority key identifiers would be identical...
return &lint.LintResult{
Status: lint.Error,
Details: "self signed CA certificate has an authority key id that does " +
"not match the certificate's subject key id"}
} else {
// ...but only the subject key identifier is needed for certification path building.
return &lint.LintResult{Status: lint.Pass}
}
}
if c.AuthorityKeyId == nil { //will be nil by default if not found in x509.parseCert
return &lint.LintResult{Status: lint.Error}
} else {
return &lint.LintResult{Status: lint.Pass}
Expand Down