CABF SMIME BR Appendix A.1 - countryName matches registration scheme id #768

eliot-gs · 2023-11-13T15:48:53Z

Adding lint to check the 2 character countryName in the subject matches the countryName in the registration scheme id as per Appendix A.1: The country code used in the Registration Scheme identifier SHALL match that of the subject:countryName in the Certificate as specified in Section 7.1.4.2.2.

…lint

robplee

Nothing super major, but a couple of nits to address.

robplee · 2023-11-13T17:16:11Z

v3/lints/cabf_smime_br/lint_registration_scheme_id_matches_subject_country.go

+		LintMetadata: lint.LintMetadata{
+			Name:          "e_registration_scheme_id_matches_subject_country",
+			Description:   "The country code used in the Registration Scheme identifier SHALL match that of the subject:countryName in the Certificate as specified in Section 7.1.4.2.2",
+			Citation:      "Appendix A A.1",


Suggested change

Citation: "Appendix A A.1",

Citation: "Appendix A.1",

robplee · 2023-11-13T17:16:31Z

v3/lints/cabf_smime_br/lint_registration_scheme_id_matches_subject_country.go

+type registrationSchemeIDMatchesSubjectCountry struct{}
+
+// NewRegistrationSchemeIDMatchesSubjectCountry creates a new linter to enforce SHALL requirements for registration scheme identifiers matching subject:countryName
+func NewRegistrationSchemeIDMatchesSubjectCountry() lint.LintInterface {


Suggested change

func NewRegistrationSchemeIDMatchesSubjectCountry() lint.LintInterface {

func NewRegistrationSchemeIDMatchesSubjectCountry() lint.CertificateLintInterface {

Should have seen LintInterface was deprecated. Updated as suggested.

robplee · 2023-11-13T17:18:29Z

v3/lints/cabf_smime_br/lint_registration_scheme_id_matches_subject_country.go

+    return &registrationSchemeIDMatchesSubjectCountry{}
+}
+
+// CheckApplies returns true if the provided certificate contains subject:countryName and one-or-more of the following SMIME BR policy identifiers:


Firstly, this comment doesn't mention the update to need a partially valid subject.organizationID.

Secondly, I wonder if you could compress the list of policies by just saying "and the certificate contains an Organization or Sponsor Validated policy OID"

Made comment more relevant to full content of CheckApplies

robplee · 2023-11-13T17:20:06Z

v3/lints/cabf_smime_br/lint_registration_scheme_id_matches_subject_country.go

+//   - Sponsor Validated Multipurpose
+//   - Sponsor Validated Legacy
+func (l *registrationSchemeIDMatchesSubjectCountry) CheckApplies(c *x509.Certificate) bool {
+	if c.Subject.Country == nil {


It's a bit picky but I wonder if this would be better if you also checked that there was a non-empty-string value (or make sure that there's at least one value that's two characters long?) in c.Subject.Country?

Added subsequent check that c.Subject.Country[0] is 2 characters in length because as you raised in a different comment, the Execute function of this lint is assuming that the first countryName value is present and (at least could be) valid - e.g. ISO 3166 or XX

robplee · 2023-11-13T17:23:50Z

v3/lints/cabf_smime_br/lint_registration_scheme_id_matches_subject_country.go

+
+// Execute applies the requirements on matching subject:countryName with registration scheme identifiers
+func (l *registrationSchemeIDMatchesSubjectCountry) Execute(c *x509.Certificate) *lint.LintResult {
+	country := c.Subject.Country[0]


I wonder how well this combines with the CheckApplies. I have got a comment in this review suggesting you loop to find one countryName of length 2 but I wonder if you should be saying this lint applies if something like this is true:
c.Subject.Country[0] == <TWO CAPITAL LETTERS>
Given that the Execute function of this lint is assuming that the first countryName value is present and valid.

robplee · 2023-11-17T15:45:25Z

Sorry about this @eliot-gs, but there's currently some discussion about removing this requirement to have the country in the organizationIdentifier match the provided countryName attribute (will link the first email on the subject below). I wonder if we should wait for some additional clarity before merging this PR?

I guess on the flip-side though, the change hasn't been made and as it's a substantive one I imagine it will be brought in in the future making this lint useful for certs issued since 20230901 (yyyymmdd). So, perhaps this could be merged with the caveat that we may need to add an ineffective date once there's a date for when any future changes are coming into force?

SMIME WG email: https://lists.cabforum.org/pipermail/smcwg-public/2023-November/000863.html

robplee · 2023-11-27T17:01:26Z

Hi @eliot-gs, I've been thinking about this PR a bit and I think we can merge it in. There is a decent chance that the rule you're enforcing with your lint is going to be removed, but that change is a substantive change to the BRs and should be made effective from a future date. Therefore, the requirement from appendix A1 is effective from 1st September 2023 until the TBC future date.

My thinking is we should merge this in, keep our eyes open on the conversations around SMCWG and once that conversation is resolved we can do another PR to set the ineffective date for the lint in this PR.

What do you think @christopher-henderson?

christopher-henderson

Thank you for your thorough policy reviews @robplee and thank you the lint @eliot-gs!

I had one minor request on a code comment, however it appears that a file needs to be go fmted before we can merge.

christopher-henderson · 2023-12-03T15:41:14Z

v3/lints/cabf_smime_br/lint_registration_scheme_id_matches_subject_country.go

+
+	for _, id := range c.Subject.OrganizationIDs {
+		submatches := countryRegex.FindStringSubmatch(id)
+		if len(submatches) < 3 {


Thankfully len(nil) won't explode here 😄 (I had to double check this myself, it returns 0 which makes sense).

christopher-henderson · 2023-12-03T15:43:27Z

v3/lints/cabf_smime_br/lint_registration_scheme_id_matches_subject_country.go

+// verifySMIMEOrganizationIdentifierContainSubjectNameCountry verifies that the country code used in the subject:organizationIdentifier matches subject:countryName
+func verifySMIMEOrganizationIdentifierContainsSubjectNameCountry(id string, country string) error {
+	submatches := countryRegex.FindStringSubmatch(id)
+	// Captures the country code from the organization identifier


Suggested change

// Captures the country code from the organization identifier

// Captures the country code from the organization identifier

// Note that this raw indexing into the second position is only safe

// due to a length check done in CheckApplies

I often find it useful for the poor unfortunate souls in the future to leave a note as to why an unchecked index/dereference is safe, just in case those safety guarantees are broken by someone in the future and now the cause has to be hunted down.

I have added this suggested comment.

eliot-gs · 2023-12-04T14:27:10Z

Thanks @christopher-henderson, I have addressed the issues you raised.

chrisbn · 2023-12-12T13:35:27Z

I'd like to highlight that the current requirement (and lint) effectively prevents the use of LEI and INT as registration schemes.
As per 7.1.4.2.2 d note 2 and Appendix A A.1, the country code shall be set to XG (example INTXG, LEIXG).

The Subject:CountryName SHALL contain the country code associated with the location of the Subject verified, which cannot be XG, and so this lint would fail.

Also, the existing lint e_subject_country_not_iso does not recognize XG as a valid country, so even if the Subject:CountryName would be set to XG, this lint would block it.

robplee · 2023-12-13T08:54:57Z

Hi @chrisbn, thanks for the comment however the point you're raising is not a zlint problem. zlint exists to apply the rules as they are written and, to me, your comment sounds like the SMIME BRs are broken because of the inconsistency between the parts of the document you've highlighted. As you've pointed out, the 7.1.4.2.2 d note 2 text says that in some cases the country code in the organizationIdentifier should be "XG", Appendix A.1 says the country codes in the organizationIdentifier and countryName must match, and 7.1.4.2.2 n doesn't support the countryName being set to "XG". This is a failure of the SMIME BRs and should be fixed there before any changes to zlint are made.

tl;dr
We don't write the rules here, we just enforce them :)

eliot-gs and others added 6 commits November 7, 2023 14:04

lint and unit test subject country in organization id

81102a2

Merge branch 'zmap:master' into smime_country_matches_organizationID_…

5a54aaf

…lint

add lints and unit test for matching country in format id

17ea105

deletes accidential workflow additions

9f62043

updates according to PR comments

b90ae52

fixes indentation

3405958

robplee suggested changes Nov 13, 2023

View reviewed changes

updates following PR comments

7015ab3

eliot-gs changed the title ~~CABF SMIME BR Appendix A A.1 - countryName matches registration scheme id~~ CABF SMIME BR Appendix A.1 - countryName matches registration scheme id Nov 14, 2023

robplee approved these changes Nov 15, 2023

View reviewed changes

Merge branch 'master' into smime_country_matches_organizationID_lint

a7d0512

christopher-henderson self-requested a review December 2, 2023 17:23

christopher-henderson reviewed Dec 3, 2023

View reviewed changes

eliot-gs and others added 2 commits December 4, 2023 14:12

updates comment and formatting

108a22b

Merge branch 'master' into smime_country_matches_organizationID_lint

243a430

christopher-henderson self-requested a review December 4, 2023 15:01

christopher-henderson approved these changes Dec 4, 2023

View reviewed changes

christopher-henderson merged commit bc2c0fd into zmap:master Dec 4, 2023
4 checks passed

robplee mentioned this pull request Dec 14, 2023

Limit e_registration_scheme_id_matches_subject_country to no longer apply to LEI or INT organizationIdentifiers #781

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CABF SMIME BR Appendix A.1 - countryName matches registration scheme id #768

CABF SMIME BR Appendix A.1 - countryName matches registration scheme id #768

eliot-gs commented Nov 13, 2023 •

edited

Loading

robplee left a comment

robplee Nov 13, 2023

eliot-gs Nov 14, 2023

robplee Nov 13, 2023

eliot-gs Nov 14, 2023

robplee Nov 13, 2023

eliot-gs Nov 14, 2023

robplee Nov 13, 2023

eliot-gs Nov 14, 2023

robplee Nov 13, 2023

robplee commented Nov 17, 2023

robplee commented Nov 27, 2023

christopher-henderson left a comment

christopher-henderson Dec 3, 2023

christopher-henderson Dec 3, 2023

eliot-gs Dec 4, 2023

eliot-gs commented Dec 4, 2023

chrisbn commented Dec 12, 2023 •

edited

Loading

robplee commented Dec 13, 2023

	func NewRegistrationSchemeIDMatchesSubjectCountry() lint.LintInterface {
	func NewRegistrationSchemeIDMatchesSubjectCountry() lint.CertificateLintInterface {

CABF SMIME BR Appendix A.1 - countryName matches registration scheme id #768

CABF SMIME BR Appendix A.1 - countryName matches registration scheme id #768

Conversation

eliot-gs commented Nov 13, 2023 • edited Loading

robplee 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

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

robplee commented Nov 17, 2023

robplee commented Nov 27, 2023

christopher-henderson 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

eliot-gs commented Dec 4, 2023

chrisbn commented Dec 12, 2023 • edited Loading

robplee commented Dec 13, 2023

eliot-gs commented Nov 13, 2023 •

edited

Loading

chrisbn commented Dec 12, 2023 •

edited

Loading