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

Qcstatem psd2 national scheme #861

Open
wants to merge 42 commits into
base: master
Choose a base branch
from

Conversation

mtgag
Copy link
Contributor

@mtgag mtgag commented Jun 21, 2024

Following discussion at #847.

This one is a PSD2 related lint.

mtg and others added 30 commits February 4, 2020 17:45
util: gtld_map autopull updates for 2021-10-21T07:25:20 UTC
@defacto64
Copy link
Contributor

@mtgag If I am not mistaken, this lint checks that, in case the organizationIdentifier contains a "locally defined identity type reference" (i.e. it starts with two characters followed by ":"), the QcStatement id-qcs-pkixQCSyntax-v2 is present and contains at least one element of type URI. This is fine, as it is in line with ETSI EN 319 412-1 (§5.1.4), however it's not a specific requirement for PSD2 certificates, but for all qualified certificates.

@mtgag
Copy link
Contributor Author

mtgag commented Jun 24, 2024

@mtgag If I am not mistaken, this lint checks that, in case the organizationIdentifier contains a "locally defined identity type reference" (i.e. it starts with two characters followed by ":"), the QcStatement id-qcs-pkixQCSyntax-v2 is present and contains at least one element of type URI. This is fine, as it is in line with ETSI EN 319 412-1 (§5.1.4), however it's not a specific requirement for PSD2 certificates, but for all qualified certificates.

In check applies it also checks that the certificate has the 0.4.0.19495.2 OID in QcStatements:

	_, isPresent := util.IsQcStatemPresent(c, &util.IdEtsiPsd2Statem)
	if !isPresent {
		return false
	}

in order to be triggered only for such certificates. What would be the proper check? IsAnyEtsiQcStatementPresent (cf. lint_qcstatem_etsi_present_qcs_critical.go)?

@defacto64
Copy link
Contributor

defacto64 commented Jul 2, 2024

@mtgag , sorry for the wait.

What I meant to say is that this check should be carried out regardless of whether the certificate is of PSD2 type or not, as it is a requirement that applies to all types of qualified certificates. At least, this is my understanding of ETSI EN 319 412-2, and I may well be wrong. If I am right, this lint should (ideally, IMO) not care if the certificate being linted contains the PSD2 QcStatement or not, even less parse it, but just verify if it is a qualified certificate (of any type). From this perspective - or perhaps in any case - I think the correct normative reference for this particular lint should be ETSI EN 319 412-1 §5.1.4 and not ETSI TS 119 495 §5.2.1.

Still from this perspective, some of the code and test files attached to this PR are perhaps, with all due respect, a little overabundant, as the check in question could be achieved in a more streamlined way.

I would like to clarify that IMO there is nothing wrong in doing this check on a PSD2 certificate, but only that it would be worth doing it in a more generalized way given that it is for a requirement that applies to a much broader set of certificates that not just the PSD2 ones, so why not try to hit many more birds with one stone?
(apologies to the birds)

@mtgag
Copy link
Contributor Author

mtgag commented Jul 5, 2024

@mtgag , sorry for the wait.

What I meant to say is that this check should be carried out regardless of whether the certificate is of PSD2 type or not, as it is a requirement that applies to all types of qualified certificates. At least, this is my understanding of ETSI EN 319 412-2, and I may well be wrong. If I am right, this lint should (ideally, IMO) not care if the certificate being linted contains the PSD2 QcStatement or not, even less parse it, but just verify if it is a qualified certificate (of any type). From this perspective - or perhaps in any case - I think the correct normative reference for this particular lint should be ETSI EN 319 412-1 §5.1.4 and not ETSI TS 119 495 §5.2.1.

Still from this perspective, some of the code and test files attached to this PR are perhaps, with all due respect, a little overabundant, as the check in question could be achieved in a more streamlined way.

I would like to clarify that IMO there is nothing wrong in doing this check on a PSD2 certificate, but only that it would be worth doing it in a more generalized way given that it is for a requirement that applies to a much broader set of certificates that not just the PSD2 ones, so why not try to hit many more birds with one stone? (apologies to the birds)

Thank you for the feedback. I will take a deeper look into the comments, implementation, test data, and so on. Currently, I am quite busy, and the holiday season is about to start, so I will probably get back to it in a few weeks.

@mtgag
Copy link
Contributor Author

mtgag commented Sep 2, 2024

Coming back to this issue. Please excuse the delayed answer.

On page 9 of https://www.etsi.org/deliver/etsi_en/319400_319499/31941202/02.02.01_60/en_31941202v020201p.pdf
the content of the subject is discussed. As far as organizationIdentifier and serialNumber concerning it states:

"Certificates may include one or more semantics identifiers as specified in ETSI
EN 319 412-1 [i.4], clause 5 which defines the semantics for the organizationIdentifier attribute.

.....

Certificates may include one or more semantics identifiers as specified in ETSI EN 319 412-1 [i.4], clause 5 which define the semantics
for the serialNumber attribute."

Therefore, the special format applies only when specific semantics identifiers are present and not to all qualified certificates.

I believe, that if it implemented as proposed in the following we would cover all cases where the ETSI specification is explicit and thus avoid any false positives:

IF
[
When the natural person semantics identifier is included (5.1.3, https://www.etsi.org/deliver/etsi_en/319400_319499/31941201/01.04.04_60/en_31941201v010404p.pdf)
AND
two characters according to local ... are present in serialNumber
]
OR
[
When the legal person semantics identifier is included (5.1.4, https://www.etsi.org/deliver/etsi_en/319400_319499/31941201/01.04.04_60/en_31941201v010404p.pdf)
AND
two characters according to local ... are present in organizationIdentifier]
]
OR
[
If a PSD2 Authorization Number was issued by the NCA (5.2.1,GEN-5.2.1-2, https://www.etsi.org/deliver/etsi_ts/119400_119499/119495/01.04.01_60/ts_119495v010401p.pdf) AND
two characters according to local ... are present in organizationIdentifier]
]
THEN
check that the remainder of the string also fulfills the national scheme syntax.

What do you think? If it is OK. I could start rewriting this lint.

@mtgag
Copy link
Contributor Author

mtgag commented Nov 6, 2024

There was no feedback on this ticket since several weeks. Is this issue still relevant for review/inclusion? If not, I believe we could close this PR.

@christopher-henderson
Copy link
Member

@defacto64 do you still have a particular opinion on this one?

@defacto64
Copy link
Contributor

Sorry for the delay, unfortunately I can only dedicate a very small fraction of my time to this project.
I will give a feedback by tomorrow at the latest.

@defacto64
Copy link
Contributor

@mtgag: could you repost your pseudo-code with some indentation so that the processing flow is better understood?

@mtgag
Copy link
Contributor Author

mtgag commented Nov 7, 2024

@mtgag: could you repost your pseudo-code with some indentation so that the processing flow is better understood?

Sure:

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

I left the references out to focus on the pseudo-code, the references to support this are found here #861 (comment)

@defacto64
Copy link
Contributor

Your pseudo code seems ok to me, at least the first two conditions.... not sure about the third.

It still does not catch non-PSD2 certificates that include the legal person semantics identifier and which therefore are subject to requirement LEG-5.1.4-05 of ETSI EN 319 412-1. Therefore, if the organizationIdentifier in those certificates starts with "two characters etc.", it would be good to "check that the remainder of the string also fulfills the national scheme syntax" in those certificates as well.

Besides, it seems to me (perhaps I am missing some cross reference) that ETSI TS 119 495, although requiring that the organizationIdentifier be encoded according to the syntax defined in clause 5.1.4 (Legal person semantics identifier) of ETSI EN 319 412-1, does not require the the certificate includes such semantics identifier. If I was right, than your third condition would be wrong..... frankly, I am not sure at this time. However, if indeed PSD2 certificates are required to include the Legal person semantics identifier when their organizationIdentifier is encoded according to a national scheme, then they would still be linted by the pseudo code that I am suggesting below.

Having said that, I would suggest to change the condition for the third case in order to check compliance to requirement LEG-5.1.4-05 of ETSI EN 319 412-1, which certainly applies to both PSD2 and non-PSD2 certificates, like follows:

IF
   [ (natural person semantics identifier is included) AND (two characters according to local ... are present in serialNumber) ] 
	OR 
   [ (natural person semantics identifier is included) AND (two characters according to local ... are present in organizationIdentifier) ] 
	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.

How about that?

Please note that this are just my own personal remarks, that I am expressing here in a personal capacity, and it is not my intent to oppose to this PR.

@mtgag
Copy link
Contributor Author

mtgag commented Nov 11, 2024

There was a transfer error from the first pseudocode here to the second simplified version: It should be:

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) ] 
	OR 
   [ (a PSD2 Authorization Number was issued by the NCA) AND (two characters according to local ... are present in organizationIdentifier) ]
THEN
	check that the remainder of the string also fulfills the national scheme syntax.

The corrected part is exactly your proposal which matches the first pseudo-code. Therefore, I think now everything is on the right track. Let us skip the PSD part and just check the natural and legal person aspects. So the final check is:

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.

Ok?

@defacto64
Copy link
Contributor

Well, if that's the case, then it's fine with me.

@mtgag
Copy link
Contributor Author

mtgag commented Nov 11, 2024

Well, if that's the case, then it's fine with me.

Great. I will start working on this and come back to you for a review if this is fine.

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