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

Lint for S/MIME BR 7.1.2.3.g #797

Merged
merged 6 commits into from
Feb 20, 2024
Merged

Lint for S/MIME BR 7.1.2.3.g #797

merged 6 commits into from
Feb 20, 2024

Conversation

bitlux
Copy link
Contributor

@bitlux bitlux commented Feb 14, 2024

No description provided.

Copy link
Member

@christopher-henderson christopher-henderson left a comment

Choose a reason for hiding this comment

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

Thank you @bitlux!

I have some small suggestions. I believe that the code snippet below implements them. Let me know what you think.

func (l *authorityKeyIdentifierCorrect) CheckApplies(c *x509.Certificate) bool {
	return true
}

func (l *authorityKeyIdentifierCorrect) Execute(c *x509.Certificate) *lint.LintResult {
	var keyID keyIdentifier

	ext := util.GetExtFromCert(c, util.AuthkeyOID)
	if ext == nil {
		return &lint.LintResult{Status: lint.Error, Details: "missing authorityKeyIdentifier"}
	}
	if ext.Critical {
		return &lint.LintResult{Status: lint.Error}
	}
	if _, err := asn1.Unmarshal(ext.Value, &keyID); err != nil {
		return &lint.LintResult{
			Status:  lint.Fatal,
			Details: fmt.Sprintf("error unmarshalling authority key identifier extension: %v", err),
		}
	}

	hasKeyID := len(keyID.KeyIdentifier.Bytes) > 0
	hasCertIssuer := len(keyID.AuthorityCertIssuer.Bytes) > 0
	hasCertSerial := len(keyID.AuthorityCertSerialNumber.Bytes) > 0
	if !hasKeyID {
		return &lint.LintResult{Status: lint.Error, Details: "keyIdentifier not present"}
	}
	if hasCertIssuer {
		return &lint.LintResult{Status: lint.Error, Details: "authorityCertIssuer is present"}
	}
	if hasCertSerial {
		return &lint.LintResult{Status: lint.Error, Details: "authorityCertSerialNumber is present"}
	}
	return &lint.LintResult{Status: lint.Pass}
}

func (l *authorityKeyIdentifierCorrect) Execute(c *x509.Certificate) *lint.LintResult {
var keyID keyIdentifier

// ext is assumed not-nil based on CheckApplies.

Choose a reason for hiding this comment

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

So from the language authorityKeyIdentifier SHALL be present I reckon that CheckApplies should always return true and that nil AKI here would be an error condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks for the review.

if !hasKeyID {
return &lint.LintResult{Status: lint.Error}
}
if hasCertIssuer || hasCertSerial {

Choose a reason for hiding this comment

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

The Details member of the lint.LintResult type is quite useful when a lint such as this has multiple error scenarios. Otherwise, operators are left guess as to which one triggered the issue.

Here is my own take on this code block.

	if !hasKeyID {
		return &lint.LintResult{Status: lint.Error, Details: "keyIdentifier not present"}
	}
	if hasCertIssuer {
		return &lint.LintResult{Status: lint.Error, Details: "authorityCertIssuer is present"}
	}
	if hasCertSerial {
		return &lint.LintResult{Status: lint.Error, Details: "authorityCertSerialNumber is present"}
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

func (l *authorityKeyIdentifierCorrect) CheckApplies(c *x509.Certificate) bool {
return util.IsExtInCert(c, util.AuthkeyOID)

Choose a reason for hiding this comment

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

Suggested change
return util.IsExtInCert(c, util.AuthkeyOID)
return true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we want to at least check that it's a subscriber cert and an S/MIME cert, which was missing from the original commit.

Copy link
Contributor

@toddgaunt-gs toddgaunt-gs left a comment

Choose a reason for hiding this comment

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

LGTM

@christopher-henderson christopher-henderson merged commit 060b385 into zmap:master Feb 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants