E qcstatem correct national scheme

[mtgag]

This PR replaces the previous PR (PR #861) based on our discussion there. I will close the original PR so we can continue work here.

This lint implements the following pseudocode:

IF
[ (natural person semantics identifier is included) AND (two characters according to local ... are present in serialNumber) ]
OR
[ (legal person semantics identifier is included) AND (two characters according to local ... are present in organizationIdentifier) ]
THEN
check that the remainder of the string also fulfills the national scheme syntax.

[christopher-henderson]

Thank you @mtgag! I'll start reviewing this (which will likely take longer than usual because we have historical context to catch up on).

Including @defacto64 because you were party to the conversation last fall in #861

[christopher-henderson]

It took me a bit to understand the changes in this file, but from what I can see it's simply lifting out complex blocks into their own functions.

Is there anything that you would like to call out for closer review? It's difficult to tell what is new and what is simply re-arranging code for readability.

[mtgag]

There were some older PRs that didn't make it to the main branch. I believe this version was used in those PRs and should include all the necessary functionality related to QC. So it is a mix of extracting existing functionality and adding features that are not currently required. Since the other ETSI lint tests are running fine, I do not think it is necessary to take a closer look.

[christopher-henderson]

I'm surprised that the linter isn't complaining that a public function is returning a private type. 🤷 I can't say that I'm a huge fan of golangci.

[mtgag]

refactored the code to address it.

[christopher-henderson]

Would you mind including the original error in the message? It'll help with debugging some day, I am sure of it.

[mtgag]

added

[christopher-henderson]

Suggested change

	re := regexp.MustCompile(`^.{2}:`)
	re := regexp.MustCompile(`^[a-zA-Z]{2}:`)

I am not 100% certain of this, but do at least consider making this a bit more precise (if possible).

[mtgag]

sounds good. Changed.

[christopher-henderson]

I am unsure of this block. The psuedo-code that describes it is...

[ (legal person semantics identifier is included) AND (two characters according to local ... are present in organizationIdentifier) ]

However, it will also succeed if there are simply no orgIDs or if none of the orgIDs that are present match the ^.{2}: regex. Are you expecting both of these scenarios to also generate a PASS?

[mtgag]

The CheckApplies should cover this. If the certificate does not have any orgIDs or none of the orgIDs that are present match the regexp then it is an NA.

[christopher-henderson]

Would you mind placing this regex in the global context so that CheckApplies and Execute use the precise same object?

[mtgag]

changed

[defacto64]

Hello!

My first observation is that this PR includes quite a bit of code that is not really needed for the specific lint proposed. I am referring to a number of structs and utility functions (mostly in the qc_stmt.go source) related to the etsi-psd2-qcStatement (from ETSI TS 119 495), which would be useful for additional lints that @mtgag will presumably propose later, but not for this one here. Personally I have no objection to this "anticipated" utility code, from a technical point of view. However, I do have some general doubts in the prospect of PSD2-oriented lints, because ETSI TS 119 495 presents some "undeterminacies" that IMO are not a good basis on which to build certificate lints. But we can discuss this later, case by case, if and when some lints based on ETSI TS 119 495 will be proposed.

Apart from the said additions, the rest of the changes made to the qc_stmt.go source are basically an enhancement of the util.ParseQcStatem() function in order to also handle two additional QcStatements, namely qcs-pkixQCSyntax-v2 (needed for this lint) and etsi-psd2-qcStatement (not needed for this lint).

On this specific lint I have a couple of doubts:

• it seems to me the two functions CheckApplies() and Execute() do almost the same thing, that is, they check if there is a semanticsIdentifier and if the subject identifier (be it serialNumber or orgId) starts with two characters followed by a ":"; the only difference is that Execute() also checks if after the ":" there are two other characters and a "-". This is not wrong, but it means that you parse the QcStatements twice and in particular the qcs-pkixQCSyntax-v2 element; the whole thing could be simplified, without impacting performance, by differentiating the tasks of the two functions.
• this lint fails to verify that, if the subject identifier (serialNumber or orgId) uses a "national scheme", the NameRegistrationAuthority be also present as prescribed by ETSI EN 319 412-1:

When a locally defined identity type reference is provided (two characters followed by ":"), the nameRegistrationAuthorities element of SemanticsInformation (IETF RFC 3739 [1]) shall be present and shall contain at least a uniformResourceIdentifier generalName.

This seems more important to me than verifying that the subject identifier has two characters and a "-" after the ":".

[mtgag]

Hello!

My first observation is that this PR includes quite a bit of code that is not really needed for the specific lint proposed. I am referring to a number of structs and utility functions (mostly in the qc_stmt.go source) related to the etsi-psd2-qcStatement (from ETSI TS 119 495), which would be useful for additional lints that @mtgag will presumably propose later, but not for this one here. Personally I have no objection to this "anticipated" utility code, from a technical point of view. However, I do have some general doubts in the prospect of PSD2-oriented lints, because ETSI TS 119 495 presents some "undeterminacies" that IMO are not a good basis on which to build certificate lints. But we can discuss this later, case by case, if and when some lints based on ETSI TS 119 495 will be proposed.

I am a bit concerned as well and not entirely satisfied with the current code. It might be useful in the future, but it is difficult to estimate how much effort we could invest in the ETSI lints. If you or the project team do not consider this a showstopper, I suppose we can proceed as is. Otherwise, I suggest we close this PR for now until I have the opportunity to refactor the code.

Apart from the said additions, the rest of the changes made to the qc_stmt.go source are basically an enhancement of the util.ParseQcStatem() function in order to also handle two additional QcStatements, namely qcs-pkixQCSyntax-v2 (needed for this lint) and etsi-psd2-qcStatement (not needed for this lint).

On this specific lint I have a couple of doubts:

* it seems to me the two functions `CheckApplies()` and `Execute()` do almost the same thing, that is, they check if there is a semanticsIdentifier and if the subject identifier (be it serialNumber or orgId) starts with two characters followed by a ":"; the only difference is that `Execute()` also checks if after the ":" there are two other characters and a "-". This is not wrong, but it means that you parse the QcStatements twice and in particular the **qcs-pkixQCSyntax-v2** element; the whole thing could be simplified, without impacting performance, by differentiating the tasks of the two functions.

We could simply check whether IdQcsPkixQCSyntaxV2 is present and return true from CheckApplies. Would that resolve the issue?

* this lint fails to verify that, if the subject identifier (serialNumber or orgId) uses a "national scheme", the NameRegistrationAuthority be also present as prescribed by ETSI EN 319 412-1:

When a locally defined identity type reference is provided (two characters followed by ":"), the nameRegistrationAuthorities element of SemanticsInformation (IETF RFC 3739 [1]) shall be present and shall contain at least a uniformResourceIdentifier generalName.

This seems more important to me than verifying that the subject identifier has two characters and a "-" after the ":".

When I reviewed the specification a few days ago, it also seemed more important to me. This should be implemented as a new lint.

[defacto64]

My first doubt on this specific lint ("On this specific lint I have a couple of doubts...") is mainly a matter of taste: I tend to avoid doing the same thing twice if it's not really necessary, even when the performance impact is negligible. If it was me to code this lint, I would somehow "memorize" some of the results of the checks made in CheckApplies() for subsequent use by Execute(). Not all the results, but only the parts that are strictly necessary to Execute(), based on the fact that if Execute() is invoked than the certificate necessarily has the features that were checked for by CheckApplies(). For instance, one could store some info into some private field(s) of the qcStatemNationalScheme struct. After all, Zlint only processes one certificate at a time, so this should not have unwanted side-effects. But again, it's a matter of personal taste and you are not supposed to share my personal inclinations. 😀 In any case, for questions of style it is better to refer to @christopher-henderson's opinion.

By the way, @mtgag: would you point us to some real certificates that would not pass this lint (AS IS) ?

[defacto64]

I've been thinking about this lint a bit more and have one more point to make, in addition to what I said above.
Let me be clear that this is just my personal opinion, and @christopher-henderson might see it differently.

This lint performs a check that is a subset of a broader check that I think would be worth performing instead.
In fact, this lint only considers the so-called "local scheme", but I see no good reason to limit it to that scheme, especially considering that the "local scheme" entails additional requirements that are better covered by a separate lint.
According to ETSI EN 319 412-1, paragraphs 5.1.3 and 5.1.4, if the certificate contains one of the two semantic identifiers defined there (i.e. Natural or Legal), then the serialNumber attribute or the organizationaldentifier attribute (as applicable) SHALL (uppercase mine) be encoded as specified there. And since N different schemes are allowed (including the "local" one), in my opinion it would be worth checking all the allowed possibilities, i.e. without limiting oneselves to the "local scheme", but rather validating the appropriate attribute (i.e. organizationalidentifier or serialNumber) with suitable regexps corresponding to all the allowed schemes (i.e. one for the VAT scheme, one for the NTR scheme, one for the PSD scheme, one for the "local scheme", and so on). What do you think?

[christopher-henderson]

If I am understanding your thinking @defacto64, you would like CheckApplies to mutate the struct so that it can remember the result of that operation in Execute.

Rust is, perhaps, a better tool to visualize this difference since it has explicit, enforced, mutability rules. Essentially, it would be the difference between...

// Immutable
fn check_applies(&self, cert: &Certificate) -> bool {}

// Mutable
fn check_applies(&mut self, cert: &Certificate) -> bool {}

After all, Zlint only processes one certificate at a time

This is certainly true of the CLI usage of ZLint. However, I too frequently forget that CAs use ZLint as a library.

Given that we store these lint structs in a global registry, that means that we have invited the devil into our house - Global Mutable State. When operating in "library mode", it would be possible for one run of a lint to poison consequent runs.

As such, I think that I would prefer it if we simply eat the computational cost and keep these structs as stateless as is reasonable.

---

With regard to dead code util functions, it's more a matter of reviewability than anything else. Dead code in a compile language is trivial to identify, so if we fast forward several years and find that these functions are still unused then it is easy enough to delete them. Better to have it and not need it....

Although it does make code reviews more difficult, which then reduces due diligence from reviewers (we are all human, after all).

---

With regard to the interpretation of the requirements, I admit that I am quite far from caught up on these documents. @mtgag it sounds like you are not quite confident in these changes either, and that you would like to put this lint back into the oven?

[defacto64]

If I am understanding your thinking @defacto64, you would like CheckApplies to mutate the struct so that it can remember the result of that operation in Execute.

Rather than wanting it, let's say I had hypothesized it, but what you say convinces me that it would not be a good idea. I also forgot that ZLint is also used as a library.

[mtgag]

I've been thinking about this lint a bit more and have one more point to make, in addition to what I said above. Let me be clear that this is just my personal opinion, and @christopher-henderson might see it differently.

This lint performs a check that is a subset of a broader check that I think would be worth performing instead. In fact, this lint only considers the so-called "local scheme", but I see no good reason to limit it to that scheme, especially considering that the "local scheme" entails additional requirements that are better covered by a separate lint. According to ETSI EN 319 412-1, paragraphs 5.1.3 and 5.1.4, if the certificate contains one of the two semantic identifiers defined there (i.e. Natural or Legal), then the serialNumber attribute or the organizationaldentifier attribute (as applicable) SHALL (uppercase mine) be encoded as specified there. And since N different schemes are allowed (including the "local" one), in my opinion it would be worth checking all the allowed possibilities, i.e. without limiting oneselves to the "local scheme", but rather validating the appropriate attribute (i.e. organizationalidentifier or serialNumber) with suitable regexps corresponding to all the allowed schemes (i.e. one for the VAT scheme, one for the NTR scheme, one for the PSD scheme, one for the "local scheme", and so on). What do you think?

My first doubt on this specific lint ("On this specific lint I have a couple of doubts...") is mainly a matter of taste: I tend to avoid doing the same thing twice if it's not really necessary, even when the performance impact is negligible. If it was me to code this lint, I would somehow "memorize" some of the results of the checks made in CheckApplies() for subsequent use by Execute(). Not all the results, but only the parts that are strictly necessary to Execute(), based on the fact that if Execute() is invoked than the certificate necessarily has the features that were checked for by CheckApplies(). For instance, one could store some info into some private field(s) of the qcStatemNationalScheme struct. After all, Zlint only processes one certificate at a time, so this should not have unwanted side-effects. But again, it's a matter of personal taste and you are not supposed to share my personal inclinations. 😀 In any case, for questions of style it is better to refer to @christopher-henderson's opinion.

By the way, @mtgag: would you point us to some real certificates that would not pass this lint (AS IS) ?

I am afraid I cannot provide any real world certificates. Also the test corpus does not contain one. It would be quite interesting to locate a few ETSI/PSD certificates to run some tests. Trying https://crt.sh/?CAName=%25QSEAL%25 or https://crt.sh/?CAName=%25PSD%25 does locate a few CAs, but as far as I can tell they have not issued many certificates.

[mtgag]

I've been thinking about this lint a bit more and have one more point to make, in addition to what I said above. Let me be clear that this is just my personal opinion, and @christopher-henderson might see it differently.

This lint performs a check that is a subset of a broader check that I think would be worth performing instead. In fact, this lint only considers the so-called "local scheme", but I see no good reason to limit it to that scheme, especially considering that the "local scheme" entails additional requirements that are better covered by a separate lint. According to ETSI EN 319 412-1, paragraphs 5.1.3 and 5.1.4, if the certificate contains one of the two semantic identifiers defined there (i.e. Natural or Legal), then the serialNumber attribute or the organizationaldentifier attribute (as applicable) SHALL (uppercase mine) be encoded as specified there. And since N different schemes are allowed (including the "local" one), in my opinion it would be worth checking all the allowed possibilities, i.e. without limiting oneselves to the "local scheme", but rather validating the appropriate attribute (i.e. organizationalidentifier or serialNumber) with suitable regexps corresponding to all the allowed schemes (i.e. one for the VAT scheme, one for the NTR scheme, one for the PSD scheme, one for the "local scheme", and so on). What do you think?

You may find some interesting lints about ETSI/PSD on this branch https://github.com/mtgag/zlint/tree/all/v3/lints/etsi.
This is the list of new lints which are not in the main project:

• e_ev_ntr_subject_jurisdiction_serial
• e_ev_orgid
• e_ev_orgid_encoding
• e_ev_orgid_well_formed
• e_ev_orgidext_matches_subject
• e_ev_orgidext_present_mandatory
• e_qcstatem_psd2_national_scheme (has been completely refactored in this PR)
• e_qcstatem_psd2_orgid
• e_qcstatem_psd2_policy_mandatory
• w_qcstatem_psd2_policy_recommended
• e_qcstatem_psd2_psd2statem_encoding
• w_qcstatem_psd2_psd2statem_ncaid_eulist
• e_qcstatem_psd2_psd2statem_ncaid_format
• e_qcstatem_psd2_psd_subject
• e_qcstatem_psd2_roles
• e_qcstatem_psd2_vat_ntr
• e_qcstatem_repeated_stmt

You may take a quick view to see if any lint addresses your suggestion. We can then reactivate it and suggest a PR. We could also just try to integrate all of them into the project. If I have a significant amount of ETSI certificates I could test the lints against them to assess their quality.

[mtgag]

With regard to the interpretation of the requirements, I admit that I am quite far from caught up on these documents. @mtgag it sounds like you are not quite confident in these changes either, and that you would like to put this lint back into the oven?

I would prefer to test this and other ETSI lints against real certificates as @defacto64 suggested. Please let us wait, if we can locate a good number of ETSI certificates to test against.