Sunset subject:organizationalUnitName (Section 7.1.4.2.2.i, CAB-Forum BR) #643

mtgag · 2021-10-21T14:05:40Z

This pull request adds a lint for "Sunset subject:organizationalUnitName" - Section 7.1.4.2.2.i of CAB BR (v1.7.9 - https://cabforum.org/wp-content/uploads/CA-Browser-Forum-BR-1.7.9.pdf). In this pull request the time for the CABFBRs_1_8_0_Date in time.go (from 21st of August to 25th) has also been adjusted (see Section 1.2.1, https://cabforum.org/wp-content/uploads/CA-Browser-Forum-BR-1.8.0.pdf) and you may consider merging only this change to the main project.

This reverts commit 6c23670.

util: gtld_map autopull updates for 2021-10-21T07:25:20 UTC

christopher-henderson · 2021-10-23T17:29:52Z

v3/util/time.go

@@ -63,7 +63,8 @@ var (
 	CABFBRs_1_6_9_Date          = time.Date(2020, time.March, 27, 0, 0, 0, 0, time.UTC)
 	CABFBRs_1_7_1_Date          = time.Date(2020, time.August, 20, 0, 0, 0, 0, time.UTC)
 	AppleReducedLifetimeDate    = time.Date(2020, time.September, 1, 0, 0, 0, 0, time.UTC)
-	CABFBRs_1_8_0_Date          = time.Date(2021, time.August, 21, 0, 0, 0, 0, time.UTC)
+	CABFBRs_1_7_9_Date          = time.Date(2021, time.August, 16, 0, 0, 0, 0, time.UTC)
+	CABFBRs_1_8_0_Date          = time.Date(2021, time.August, 25, 0, 0, 0, 0, time.UTC)


Interesting, I wonder why it was set to the 21st to begin with.

christopher-henderson · 2021-10-23T17:32:29Z

v3/lints/cabf_br/lint_subject_contains_organizational_unit_name.go

+
+func (l *SubjectContainsOrganizationalUnitName) Execute(cert *x509.Certificate) *lint.LintResult {
+
+	if !cert.NotBefore.Before(time.Date(2022, time.September, 1, 0, 0, 0, 0, time.UTC)) {


I'm half tempted to make some BeforeOrOn and OnOrAfter util functions to help make statements like these read more like their source citations.

I think that's a great idea.

v3/lints/cabf_br/lint_subject_contains_organizational_unit_name.go

sleevi · 2021-10-26T17:05:12Z

v3/lints/cabf_br/lint_subject_contains_organizational_unit_name.go

+
+	if !util.TypeInName(&cert.Subject, util.OrganizationNameOID) {
+		return &lint.LintResult{Status: lint.Error, Details: "subject:organizationalUnitName is prohibited if subject:organizationName is absent"}
+	}


Question (and I'm not sure I feel too strongly, but I have opinions, and trying to solicit others): Should this be two lints?

One testing that the organization name is present (with the effective date of 1.7.1), and the other testing that the organizationalUnit is absent (with the effective date of 1.7.1, but with a CheckApplies that uses the sunset date on the notBefore)

We had exactly the same discussion here internally. We started implementing two distinct lints as you propose (they were actually three, one to cover also the deprecation case) since this felt better suited for the project, but the source code was quite similar so we merged them. I am not sure if code duplication is an issue here. In addition, this lint addresses this one sentence from the BR Document: "Prohibited if the subject:organizationName is absent or the certificate is issued on or after September 1, 2022.", so it feels OK to have one lint for that.
We are good with both options, but after this feedback we have a minor preference for two lints. Which option should we follow?

Yeah, I think the difference in effective dates justifies splitting those out, but happy if @cardonator or @christopher-henderson have different views.

Now that I think about it more, I also believe that two separate lints would be appropriate here (and I'll make a mental note to myself that an or in a requirement might be a hint in the future).

With regard to code duplication, the project tends to prefer that lints remain proper leaf nodes in the graph of computation and avoid interdependency. So many lints are so similar, that one can easily imagine trying to unify them in some clever way. But this is liable to degenerate into a God-lint of sorts which becomes quite impossible for any contributors to effectively grok. So if avoiding that means sprinkling a bit of code duplication here-or-there then I think that's the lesser of two evils. We do have the catch-all utils package, but that tends to be for answering questions that are both quite common and quite easy to get wrong.

…e.go Co-authored-by: Ryan Sleevi <ryan.sleevi@gmail.com>

sleevi

Thanks!

Some minor cleanups requested; we don't need to block on @christopher-henderson landing, although it's an easy rebase if he does :)

v3/integration/config.go

sleevi · 2021-11-01T15:01:09Z

v3/lints/cabf_br/lint_subject_contains_organizational_unit_name.go

@@ -48,18 +49,14 @@ func NewSubjectContainsOrganizationalUnitName() lint.LintInterface {
 }

 func (l *SubjectContainsOrganizationalUnitName) CheckApplies(cert *x509.Certificate) bool {
-	return util.TypeInName(&cert.Subject, util.OrganizationalUnitNameOID)
+	return !cert.NotBefore.Before(time.Date(2022, time.September, 1, 0, 0, 0, 0, time.UTC))


Perhaps @christopher-henderson is good with landing #644 ?

A minor change to align with #644 has been made

sleevi · 2021-11-01T15:03:36Z

...ints/cabf_br/lint_subject_contains_organizational_unit_name_and_no_organization_name_test.go

+			if result.Details != tc.ExpectedDetails {
+				t.Errorf("expected result details %q was %q", tc.ExpectedDetails, result.Details)
+			}


I believe you delete this (and line 29) - you never fill in an expected details here?

I realize it's largely a copy/paste from other templates, but since this is is self-contained, you should be fine here :)

in the second test case (subjectDnWithOuEntryButWithoutOEntry.pem) the expected details are filled.

mtgag · 2022-01-14T07:19:19Z

Is this pull request still considered for inclusion? Is there somethig to do on our side?

Please note this change: #643 (comment)

which is interesting for the main project and is independent of this new lint. I could make a new pull request that adresses only this issue.

v3/util/time.go

christopher-henderson · 2022-05-15T17:40:16Z

v3/lints/cabf_br/lint_subject_contains_organizational_unit_name.go

@@ -0,0 +1,64 @@
+package cabf_br


@mtgag I believe that #675 captures this requirement effectively, so perhaps we can simply delete this file and reduce this change to SubjectContainsOrganizationalUnitNameButNoOrganizationName.

@sleevi you were the primary blocker on this PR, were your concerns addressed (looking to move forward on this one and #675)?

Sure. Should we update/delete/refactor anything for this PR?

Please note that in https://github.com/zmap/zlint/blob/master/v3/util/time.go still the date for 1.8.0 needs to be adjusted. It is set to 21st of August but it should be the 25th. See https://cabforum.org/wp-content/uploads/CA-Browser-Forum-BR-1.8.0.pdf, page 14.

Right, I think that leaving the date update in this PR is positive since it was a good bugfix to find that the other PR does not capture.

Okay, thank you for your patience @mtgag I went ahead and merged #675 so I believe that this particular lint can be removed and we can move forward with the other changes in this PR.

mtgag · 2022-06-13T08:02:58Z

Comments have been addressed. The duplicate lint to #675 is removed along with the corresponding test data. Only e_subject_contains_organizational_unit_name_and_no_organization_name is still present.

mtg and others added 8 commits February 4, 2020 17:45

lint about the encoding of qcstatements for PSD2

6c23670

Revert "lint about the encoding of qcstatements for PSD2"

4666bb7

This reverts commit 6c23670.

Merge https://github.com/zmap/zlint

01996c6

Merge https://github.com/zmap/zlint

28481cc

Merge https://github.com/zmap/zlint

749d896

util: gtld_map autopull updates for 2021-10-21T07:25:20 UTC

e56e2a0

Merge pull request #1 from mtgag/zlint-gtld-update

8600050

util: gtld_map autopull updates for 2021-10-21T07:25:20 UTC

added lint for presence of OU in subject

52d8f1c

christopher-henderson self-requested a review October 23, 2021 17:08

christopher-henderson reviewed Oct 23, 2021

View reviewed changes

christopher-henderson approved these changes Oct 23, 2021

View reviewed changes

christopher-henderson requested a review from sleevi October 23, 2021 17:35

sleevi reviewed Oct 26, 2021

View reviewed changes

mtgag and others added 5 commits October 27, 2021 09:18

Update v3/lints/cabf_br/lint_subject_contains_organizational_unit_nam…

20439ba

…e.go Co-authored-by: Ryan Sleevi <ryan.sleevi@gmail.com>

separated lints to adress two requirements

6a37f8d

separated lints to adress two requirements

4e5dffb

separated lints to adress two requirements

7da5ee7

reverted change proposed by IDE

13a5761

sleevi suggested changes Nov 1, 2021

View reviewed changes

aligning to zmap#644

b341ce9

mtgag mentioned this pull request May 13, 2022

Add new lint to block organisational unit names as of 1st September 2022 #675

Merged

Merge branch 'master' into contains_ou

acc6d8a

christopher-henderson reviewed May 15, 2022

View reviewed changes

v3/util/time.go Outdated Show resolved Hide resolved

Update v3/util/time.go

45d8c7c

christopher-henderson reviewed May 15, 2022

View reviewed changes

v3/util/time.go Outdated Show resolved Hide resolved

Update v3/util/time.go

c2a70ce

christopher-henderson reviewed May 15, 2022

View reviewed changes

v3/util/time.go Outdated Show resolved Hide resolved

Update v3/util/time.go

87478fd

christopher-henderson reviewed May 15, 2022

View reviewed changes

christopher-henderson requested a review from sleevi May 15, 2022 18:09

christopher-henderson and others added 3 commits June 12, 2022 10:41

Merge branch 'master' into contains_ou

883a8b0

Merge branch 'master' into contains_ou

12576ee

addressed requested changes, removing lint that is implemented in 675

0bfd659

christopher-henderson merged commit c7955ed into zmap:master Jun 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sunset subject:organizationalUnitName (Section 7.1.4.2.2.i, CAB-Forum BR) #643

Sunset subject:organizationalUnitName (Section 7.1.4.2.2.i, CAB-Forum BR) #643

mtgag commented Oct 21, 2021

christopher-henderson Oct 23, 2021

christopher-henderson Oct 23, 2021

cardonator Oct 23, 2021

sleevi Oct 26, 2021

mtgag Oct 27, 2021

sleevi Oct 28, 2021

christopher-henderson Oct 30, 2021 •

edited

Loading

sleevi left a comment

sleevi Nov 1, 2021

mtgag Nov 1, 2021

sleevi Nov 1, 2021

mtgag Nov 1, 2021

mtgag commented Jan 14, 2022

christopher-henderson May 15, 2022

mtgag May 17, 2022

christopher-henderson May 22, 2022

christopher-henderson Jun 12, 2022

mtgag commented Jun 13, 2022


		func (l SubjectContainsOrganizationalUnitName) Execute(cert x509.Certificate) *lint.LintResult {

		if !cert.NotBefore.Before(time.Date(2022, time.September, 1, 0, 0, 0, 0, time.UTC)) {

Sunset subject:organizationalUnitName (Section 7.1.4.2.2.i, CAB-Forum BR) #643

Sunset subject:organizationalUnitName (Section 7.1.4.2.2.i, CAB-Forum BR) #643

Conversation

mtgag commented Oct 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopher-henderson Oct 30, 2021 • edited Loading

Choose a reason for hiding this comment

sleevi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtgag commented Jan 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtgag commented Jun 13, 2022

christopher-henderson Oct 30, 2021 •

edited

Loading