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 7.1.2.7.2 BR #810

Merged

Conversation

mtgag
Copy link
Contributor

@mtgag mtgag commented Mar 5, 2024

This pull request adds a lint for the presence of attributes other than CN and C in the subject of DV certificates. This work has been made in close cooperation with the D-Trust CA. We would be grateful if you could incorporate this PR in the main project.

Citation (https://cabforum.org/uploads/CA-Browser-Forum-BR-v2.0.0.pdf):
7.1.2.7.2 Domain Validated

The following table details the acceptable AttributeTypes that may appear within the type
field of an AttributeTypeAndValue, as well as the contents permitted within the value field.

Table 35: Domain Validated subject Attributes

countryName MAY The two-letter ISO 3166-1 country code for the country
associated with the Subject. Section 3.2.2.3

commonName NOT RECOMMENDED
If present, MUST contain a value derived from the
subjectAltName extension according to Section
7.1.4.3.

Any other attribute MUST NOT

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! I only had one comment regarding whether-or-not a common name should be a warning or a pass.

func (l *dvSubjectInvalidValues) Execute(cert *x509.Certificate) *lint.LintResult {
names := util.GetTypesInName(&cert.Subject)
for _, n := range names {
if n.Equal(util.CountryNameOID) || n.Equal(util.CommonNameOID) {

Choose a reason for hiding this comment

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

Given that commonName is not recommended, would you be amenable to making this a warning?

&lint.LintResult{Status: lint.Warn, Details: "DV certificate contains a subject common name, this is not recommended."}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes: ca69ecc

Copy link
Contributor

Choose a reason for hiding this comment

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

@christopher-henderson Isn't the warning for commonName already covered by lint_subject_common_name_included_sc62.go? Adding a warning means that it will now be reported twice right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is true. The contribution guidelines also cover this, specifying that a non-success status should match the lint's name prefix. I will create a PR for this if that is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christopher-henderson christopher-henderson merged commit 8d2c579 into zmap:master Mar 10, 2024
4 checks passed
@mtgag mtgag mentioned this pull request May 23, 2024
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