QCP-N-QSCD 411 1(411 2), 412-2 and 412 5 #129

breynders-cb · 2024-10-28T12:58:03Z

As previously mentioned in #124, a first PR which extends pkilint with QCP-N-QSCD for 411-1 (with restrictions of 411-2), 412-2 and 412-5.

I did my best in maintaining the existing structure of everything, please let me know where you'd want changes and we'll use this PR to get everything aligned as much as possible.

I've generated certificates as integration tests to validate most (or all) rules that I've added as part of the qcp-n-qscd profile.

Additionally:

I did not add tests, what's the testing policy? I see there's a lot of integration tests and I'm wondering if you have some tool/project set up that can already easily generate all these certificates in the right (i.e. wrong) format. If not I'll start building something for ourselves to generate some test cases for these types (but will take a bit).
For etsi the finding_metadata.csv seems to be empty, I tried to document all the sources (and changes) but is it on the roadmap to fill in that csv?

And some further questions inlined ⬇️

breynders-cb · 2024-10-28T12:59:26Z

pkilint/etsi/en_319_412_5.py

+            # PR Question: Is this from 415_5.qcs-4.2? Needs different classifier?
            allowances[en_319_412_5.id_etsi_qcs_QcCClegislation] = Rfc2119Word.MUST


I was wondering what the source was for this rule. I couldn't really find it other than the reference in 412-5 QCS 4.2. If so, would it need a different source in the validation finding?

See table 1A in EN 319 412 5, clause 4.2.1. The CCLegislation statement is needed for certs that are qualified but not in the EU.

pkilint/etsi/en_319_412_5.py

breynders-cb · 2024-11-01T15:26:23Z

Moved to ready-for-review since the ETSI rules are now considered feature-complete from my end (and I'm going to shift towards implementing the POR rules now), looking forward to the feedback!

CBonnell · 2024-11-04T15:27:43Z

Thank you this great contribution, @breynders-cb! I'm currently traveling for work this week, but will review this PR fully when I return next week.

As for the test case generation, we use der-ascii to generate test artifacts. It has a bit of learning curve to use, but quite powerful and flexible. The test case file format is the PEM text of the artifact followed by the CSV-formatted output of findings. This makes it relatively simple to write test case generation scripts.

We originally did not flesh out the ETSI finding_metadata.csv file, as the ETSI linter references the citation directly in the finding codes. We can certainly flesh out that file if that would be helpful. We are planning to revamp and improve the documentation for findings as part of #64, but haven't had the cycles yet do to so.

breynders-cb · 2024-11-05T07:42:00Z

Thank you this great contribution, @breynders-cb! I'm currently traveling for work this week, but will review this PR fully when I return next week.

As for the test case generation, we use der-ascii to generate test artifacts. It has a bit of learning curve to use, but quite powerful and flexible. The test case file format is the PEM text of the artifact followed by the CSV-formatted output of findings. This makes it relatively simple to write test case generation scripts.

We originally did not flesh out the ETSI finding_metadata.csv file, as the ETSI linter references the citation directly in the finding codes. We can certainly flesh out that file if that would be helpful. We are planning to revamp and improve the documentation for findings as part of #64, but haven't had the cycles yet do to so.

Great, thanks! I'll add der-ascii to my list of tools, for now I spruced up some of our test code and generated test certificates through bouncy castle so all new rules should have tests in the PEM+csv format.

CBonnell

Thanks for this contribution! Overall, I think this PR is in pretty good shape. I added some comments for relatively minor things.

In regard to the development of test cases, do you plan on adding more, or should I assist in creating them?

CBonnell · 2024-11-13T13:28:05Z

pkilint/etsi/etsi_constants.py

@@ -27,6 +27,7 @@ class CertificateType(enum.IntEnum):
    QNCP_W_GEN_LEGAL_PERSON_NON_EIDAS_PRE_CERTIFICATE = auto()
    QEVCP_W_PSD2_EIDAS_PRE_CERTIFICATE = auto()
    QEVCP_W_PSD2_EIDAS_NON_BROWSER_PRE_CERTIFICATE = auto()
+    QCP_N_QSCD_PRE_CERTIFICATE = auto()


Certificate Transparency for eSig/eSeal certs likely will not be a thing for a long time (if ever), so I think this type can be removed. That being said, we can keep it if we see value in having it.

CBonnell · 2024-11-15T13:47:33Z

pkilint/etsi/etsi_constants.py

@@ -256,6 +266,10 @@ def from_option_str(value):

 NON_EU_QWAC_TYPES = QWAC_TYPES - EU_QWAC_TYPES

+EU_SSCD = QCP_N_QSCD_CERTIFICATE_TYPES


Nit: rename to EU_SSCD_TYPES to match naming convention of other certificate type sets

CBonnell · 2024-11-15T13:47:43Z

pkilint/etsi/etsi_constants.py

@@ -256,6 +266,10 @@ def from_option_str(value):

 NON_EU_QWAC_TYPES = QWAC_TYPES - EU_QWAC_TYPES

+EU_SSCD = QCP_N_QSCD_CERTIFICATE_TYPES
+
+EU = EU_QWAC_TYPES | EU_SSCD


Nit: rename to EU_TYPES to match naming convention of other certificate type sets

CBonnell · 2024-11-15T13:58:33Z

tests/integration_certificate/etsi/qcp_n_qscd_final_certificate/invalid_issuer.crttest

+certificate.tbsCertificate.issuer.rdnSequence,LegalPersonIssuerAttributeAllowanceValidator,ERROR,etsi.en_319_412_2.gen-4.2.3.1-2.country_attribute_absent,
+certificate.tbsCertificate.issuer.rdnSequence,LegalPersonIssuerAttributeAllowanceValidator,ERROR,etsi.en_319_412_2.gen-4.2.3.1-2.common_name_attribute_absent,
+certificate.tbsCertificate.issuer.rdnSequence,LegalPersonIssuerAttributeAllowanceValidator,ERROR,etsi.en_319_412_2.gen-4.2.3.1-2.organization_name_attribute_absent,
+certificate.tbsCertificate.extensions.2.extnValue.subjectKeyIdentifier,SubjectKeyIdentifierValidator,INFO,pkix.subject_key_identifier_method_1_identified,


Nit: add newline to end of file

CBonnell · 2024-11-15T13:58:39Z

tests/integration_certificate/etsi/qcp_n_qscd_final_certificate/missing_qc_statements.crttest

+
+node_path,validator,severity,code,message
+certificate.tbsCertificate.extensions,QcStatementPresenceValidator,ERROR,etsi.en_319_412_5.qcs-5.01,
+certificate.tbsCertificate.extensions.1.extnValue.subjectKeyIdentifier,SubjectKeyIdentifierValidator,INFO,pkix.subject_key_identifier_method_1_identified,


Nit: add newline to end of file

CBonnell · 2024-11-15T14:05:40Z

pkilint/etsi/en_319_412_5.py

+            # PR Question: Is this from 415_5.qcs-4.2? Needs different classifier?
            allowances[en_319_412_5.id_etsi_qcs_QcCClegislation] = Rfc2119Word.MUST


See table 1A in EN 319 412 5, clause 4.2.1. The CCLegislation statement is needed for certs that are qualified but not in the EU.

CBonnell · 2024-11-15T14:17:44Z

pkilint/etsi/en_319_412_2.py

@@ -463,6 +463,10 @@ class QualifiedCertificatePoliciesValidator(validation.Validator):
            etsi_constants.QNCP_W_GEN_NP_EIDAS_CERTIFICATE_TYPES,
            en_319_411_2.id_qncp_web_gen,
        ),
+        (
+            etsi_constants.QCP_N_QSCD_CERTIFICATE_TYPES,


Can you remove or change the TODO comment on line 453 to note that QSCD certs are now supported?

CBonnell · 2024-11-15T15:17:14Z

pkilint/etsi/__init__.py

@@ -312,6 +317,16 @@ def create_validators(
            en_319_412_2.NaturalPersonSubjectAttributeAllowanceValidator()
        )

+        if certificate_type in etsi_constants.EU:


I think this if statement can be removed, as the issuer requirements are applicable for non-EU certs as well.

CBonnell · 2024-11-15T20:13:35Z

pkilint/etsi/etsi_shared.py

+
+
+class LegalPersonDuplicateAttributeAllowanceValidator(validation.Validator):
+    """


These citation comments should probably be moved to the appropriate sub-class so they document the validation finding declaration.

CBonnell · 2024-11-15T20:15:23Z

pkilint/etsi/en_319_412_5.py

@@ -307,6 +309,24 @@ def __init__(self):
        )


+class QcStatementPresenceValidator(extension.ExtensionPresenceValidator):


I don't think this class is needed, as the NaturalPersonExtensionIdentifierAllowanceValidator flags when the QCStatements extension is missing.

breynders-cb · 2024-11-19T15:52:49Z

Thanks for this contribution! Overall, I think this PR is in pretty good shape. I added some comments for relatively minor things.

In regard to the development of test cases, do you plan on adding more, or should I assist in creating them?

For test cases I worked mostly with integration tests by generating certificates so I'd like to leave it at that for this PR. I'll go through your comments and address them ASAP, thanks for the review!

breynders-cb commented Oct 28, 2024

View reviewed changes

pkilint/etsi/en_319_412_5.py Show resolved Hide resolved

breynders-cb force-pushed the qcp-n-qscd-411-1(412-2)-and-412-5 branch from 0929950 to 0e59ab3 Compare October 29, 2024 12:48

breynders-cb changed the title ~~QCP-N-QSCD 411 1(412 2) and 412 5~~ QCP-N-QSCD 411 1(411 2) and 412 5 Oct 29, 2024

breynders-cb force-pushed the qcp-n-qscd-411-1(412-2)-and-412-5 branch 2 times, most recently from e06fa2d to 6567785 Compare October 30, 2024 16:11

breynders-cb changed the title ~~QCP-N-QSCD 411 1(411 2) and 412 5~~ QCP-N-QSCD 411 1(411 2), 412-2 and 412 5 Oct 30, 2024

breynders-cb added 2 commits November 1, 2024 16:14

qcp-n-qscd: extend 412-5 with EU/EU-SSCD + questions

1a2b15e

qcp-n-qscd: 412-5 qcstatement presence validator

2931685

breynders-cb force-pushed the qcp-n-qscd-411-1(412-2)-and-412-5 branch from 6567785 to caa9f65 Compare November 1, 2024 15:23

breynders-cb marked this pull request as ready for review November 1, 2024 15:25

qcp-n-qscd: 412-2 qualified policy validator

91de6f9

breynders-cb force-pushed the qcp-n-qscd-411-1(412-2)-and-412-5 branch from caa9f65 to a0295ea Compare November 11, 2024 09:29

breynders-cb added 2 commits November 11, 2024 10:35

qcp-n-qscd: 412-2 check for legal person issuers by re-using 412_3 code

cf25115

qcp-n-qscd: 412-2 content-commitment to true

dd6e770

breynders-cb force-pushed the qcp-n-qscd-411-1(412-2)-and-412-5 branch from a0295ea to dd6e770 Compare November 11, 2024 09:37

CBonnell reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QCP-N-QSCD 411 1(411 2), 412-2 and 412 5 #129

QCP-N-QSCD 411 1(411 2), 412-2 and 412 5 #129

breynders-cb commented Oct 28, 2024 •

edited

Loading

breynders-cb Oct 28, 2024

CBonnell Nov 15, 2024

breynders-cb commented Nov 1, 2024

CBonnell commented Nov 4, 2024

breynders-cb commented Nov 5, 2024

CBonnell left a comment

CBonnell Nov 13, 2024

CBonnell Nov 15, 2024

CBonnell Nov 15, 2024

CBonnell Nov 15, 2024

CBonnell Nov 15, 2024

CBonnell Nov 15, 2024

CBonnell Nov 15, 2024

CBonnell Nov 15, 2024

CBonnell Nov 15, 2024

CBonnell Nov 15, 2024

breynders-cb commented Nov 19, 2024

		# PR Question: Is this from 415_5.qcs-4.2? Needs different classifier?
		allowances[en_319_412_5.id_etsi_qcs_QcCClegislation] = Rfc2119Word.MUST

		@@ -256,6 +266,10 @@ def from_option_str(value):

		NON_EU_QWAC_TYPES = QWAC_TYPES - EU_QWAC_TYPES

		EU_SSCD = QCP_N_QSCD_CERTIFICATE_TYPES



		class LegalPersonDuplicateAttributeAllowanceValidator(validation.Validator):
		"""

		@@ -307,6 +309,24 @@ def __init__(self):
		)


		class QcStatementPresenceValidator(extension.ExtensionPresenceValidator):

QCP-N-QSCD 411 1(411 2), 412-2 and 412 5 #129

Are you sure you want to change the base?

QCP-N-QSCD 411 1(411 2), 412-2 and 412 5 #129

Conversation

breynders-cb commented Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breynders-cb commented Nov 1, 2024

CBonnell commented Nov 4, 2024

breynders-cb commented Nov 5, 2024

CBonnell 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

Choose a reason for hiding this comment

breynders-cb commented Nov 19, 2024

breynders-cb commented Oct 28, 2024 •

edited

Loading