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.

Sign up for GitHub

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

Support indirect CRLs in generate-revocation-set.py #36502

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
+188 −89
Open

Support indirect CRLs in generate-revocation-set.py #36502

Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b9ad2ca
Updates to script to support indirect CRL signing.
bh3000 Nov 14, 2024
c49c7ce
Updates to script to support indirect CRL signing.
bh3000 Nov 14, 2024
cff715d
Restyled by autopep8
restyled-commits Nov 14, 2024
0edc3ce
Merge branch 'master' into indirect_crl
bh3000 Nov 14, 2024
83e687e
Merge branch 'master' into indirect_crl
bh3000 Nov 14, 2024
1b306c5
Merge branch 'project-chip:master' into indirect_crl
bh3000 Nov 14, 2024
794679f
Merge branch 'master' into indirect_crl
bh3000 Nov 21, 2024
b8f41ea
Merge branch 'project-chip:master' into indirect_crl
bh3000 Nov 26, 2024
c1ab43c
Updates to script to support indirect CRL signing.
bh3000 Nov 26, 2024
44b3baf
Restyled by autopep8
restyled-commits Nov 26, 2024
e6c3c6f
Merge branch 'master' into indirect_crl
bh3000 Nov 26, 2024
e341b13
Updates to script to support indirect CRL signing.
bh3000 Dec 9, 2024
0f2037b
Updates to script to support indirect CRL signing.
bh3000 Dec 9, 2024
ce025b1
Updates to script to support indirect CRL signing.
bh3000 Dec 9, 2024
24e4c03
Updates to script to support indirect CRL signing.
bh3000 Dec 9, 2024
30aefe5
Updates to script to support indirect CRL signing.
bh3000 Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 60 additions & 51 deletions credentials/generate-revocation-set.py
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be good to decode as much as possible before passing the values around (e.g. issuer DN).
I find that the handling of the exception is a bit inconsistent. For example, some are pretty must all runtime exception while others are not. When we get a response, I think it's better to check whether the response is empty or not and print out that error as opposed to have the runtime handle it. This is in comparison to errors such as the structure from the server is not compatible to the parsing attempt. I think that's an exception.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the approach to passing the x509 name around and i've added error handling for the requests that will log any errors that happen. I've kept the parsing so that it will log the issue and continue with the algorithm. My concern is that if there is a cert or other response that cannot be parsed, the entire script would fail and one would not be able to generate the rest of the revocation set. This way the reasoning for skipping gets logged and the script can continue. WDYT?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's critical failure if any part of the processing fails unless we think it's okay to mask the problem.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the missing akid/skid error so that it will prevent further processing. However I still am not sure this is best for the users of the script. Their goal is to get the most comprehensive list of revoked certs possible. By having the a single exception prevent all processing (for a bad cert of a single vendor, or a single timed out request) the user wont get any revocation sets for any of the vendors keys. If the issue is persistent, then they're blocked from identifying any revoked certs. In my opinion it is best to provide as much data as we can and just log these issues.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could introduce an option to "continue" when encountering failures for the best attempt route. WDYT?

Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,13 @@
# limitations under the License.
#

# Generates a basic RevocationSet from TestNet
# Generates a basic RevocationSet from TestNet or MainNet.
# Note: Indirect CRLs are only supported with py cryptography version 44.0.0.
# You may need to patch in a change locally if you are using an older
# version of py cryptography. The required changes can be viewed in this
# PR: https://github.com/pyca/cryptography/pull/11467/files. The file that
# needs to be patched is accessible from your local connectedhomeip
# directory at ./.environment/pigweed-venv/lib/python3.11/site-packages/cryptography/x509/extensions.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constraints.txt of the SDK should just ensure that the version of the module obtained during bootstrap is correct.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, however the cryptography version with the fix is not yet released. So anyone using this with the current version will have issues if they're using indirect CRLs. This note just helps those using indirect CRLs avoid having to track down and fix this issue until the new version is live.

# Usage:
# python ./credentials/generate-revocation-set.py --help

Expand Down Expand Up @@ -279,51 +285,6 @@ def get_revocation_points(self) -> list[dict]:

return response["PkiRevocationDistributionPoint"]

def get_issuer_cert(self, cert: x509.Certificate) -> Optional[x509.Certificate]:
'''
Get the issuer certificate for

Parameters
----------
cert: x509.Certificate
Certificate

Returns
-------
str
Issuer certificate in PEM format
'''
issuer_name_b64 = get_issuer_b64(cert)
akid = get_akid(cert)
if akid is None:
return

# Convert CRL Signer AKID to colon separated hex
akid_hex = akid.hex().upper()
akid_hex = ':'.join([akid_hex[i:i+2] for i in range(0, len(akid_hex), 2)])

logging.debug(
f"Fetching issuer from:{self.rest_node_url}/dcl/pki/certificates/{issuer_name_b64}/{akid_hex}")

if self.use_rest:
response = requests.get(
f"{self.rest_node_url}/dcl/pki/certificates/{issuer_name_b64}/{akid_hex}").json()
else:
response = self.get_dcld_cmd_output_json(
['query', 'pki', 'x509-cert', '-u', issuer_name_b64, '-k', akid_hex])

issuer_certificate = response["approvedCertificates"]["certs"][0]["pemCert"]

logging.debug(f"issuer: {issuer_certificate}")

try:
issuer_certificate_object = x509.load_pem_x509_certificate(bytes(issuer_certificate, 'utf-8'))
except Exception:
logging.error('Failed to parse PAA certificate')
return

return issuer_certificate_object

def get_revocations_points_by_skid(self, issuer_subject_key_id) -> list[dict]:
'''
Get revocation points by subject key ID
Expand All @@ -347,6 +308,53 @@ def get_revocations_points_by_skid(self, issuer_subject_key_id) -> list[dict]:

return response["pkiRevocationDistributionPointsByIssuerSubjectKeyID"]["points"]

def get_paa_cert(self, initial_cert: x509.Certificate) -> Optional[x509.Certificate]:
"""Get the PAA certificate for the CRL Signer Certificate."""
issuer_name_b64 = get_issuer_b64(initial_cert)
akid = get_akid(initial_cert)
if akid is None:
return
paa_certificate = None
while not paa_certificate:
try:
issuer_certificate = None
response = None
akid_hex = akid.hex().upper()
if self.use_rest:
akid_hex = ':'.join([akid_hex[i:i+2] for i in range(0, len(akid_hex), 2)])
logging.debug(
f"Fetching issuer from:{self.rest_node_url}/dcl/pki/certificates/{issuer_name_b64}/{akid_hex}")
response = requests.get(
f"{self.rest_node_url}/dcl/pki/certificates/{issuer_name_b64}/{akid_hex}").json()
if response["approvedCertificates"]["certs"][0]["pemCert"]:
issuer_certificate = x509.load_pem_x509_certificate(
bytes(response["approvedCertificates"]["certs"][0]["pemCert"], 'utf-8'))
else:
raise requests.exception.NotFound(
f"No certificate found for {self.rest_node_url}/dcl/pki/certificates/{issuer_name_b64}/{akid_hex}")
else:
query_cmd_list = ['query', 'pki', 'x509-cert', '-u', issuer_name_b64, '-k', akid_hex]

response = self.get_dcld_cmd_output_json(
['query', 'pki', 'x509-cert', '-u', issuer_name_b64, '-k', akid_hex])
if response["approvedCertificates"]["certs"][0]["pemCert"]:
issuer_certificate = x509.load_pem_x509_certificate(
bytes(response["approvedCertificates"]["certs"][0]["pemCert"], 'utf-8'))
else:
raise requests.exception.NotFound(f"No certificate found for dcl query{' '.join(query_cmd_list)}")
bh3000 marked this conversation as resolved.
Show resolved Hide resolved
if response["approvedCertificates"]["certs"][0]["isRoot"]:
paa_certificate = issuer_certificate
break

except Exception as e:
logging.error('Failed to get PAA certificate', e)
return
issuer_name_b64 = get_issuer_b64(issuer_certificate)
akid = get_akid(issuer_certificate)
if paa_certificate is None:
logging.warning("PAA Certificate not found, continue...")
return paa_certificate


@click.command()
@click.help_option('-h', '--help')
Expand Down Expand Up @@ -418,12 +426,12 @@ def main(use_main_net_dcld: str, use_test_net_dcld: str, use_main_net_http: bool
continue

# 5. Validate the certification path containing CRLSignerCertificate.
paa_certificate_object = dcld_client.get_issuer_cert(crl_signer_certificate)
paa_certificate_object = dcld_client.get_paa_cert(crl_signer_certificate)
if paa_certificate_object is None:
logging.warning("PAA Certificate not found, continue...")
continue

if validate_cert_chain(crl_signer_certificate, crl_signer_delegator_cert, paa_certificate_object) is False:
if not validate_cert_chain(crl_signer_certificate, crl_signer_delegator_cert, paa_certificate_object):
logging.warning("Failed to validate CRL Signer Certificate chain, continue...")
continue

Expand All @@ -449,7 +457,8 @@ def main(use_main_net_dcld: str, use_test_net_dcld: str, use_main_net_http: bool
if count_with_matching_vid_issuer_skid > 1:
try:
issuing_distribution_point = crl_file.extensions.get_extension_for_oid(
x509.OID_ISSUING_DISTRIBUTION_POINT).value
x509.oid.ExtensionOID.ISSUING_DISTRIBUTION_POINT
).value
except Exception:
logging.warning("CRL Issuing Distribution Point not found, continue...")
continue
Expand All @@ -465,8 +474,8 @@ def main(use_main_net_dcld: str, use_test_net_dcld: str, use_main_net_http: bool

# TODO: 8. Validate CRL as per Section 6.3 of RFC 5280

# 9. decide on certificate authority name and AKID
if revocation_point["isPAA"] and not is_self_signed_certificate(crl_signer_certificate):
# 9. Decide on certificate authority to match against CRL entries.
if revocation_point["isPAA"]:
certificate_authority_name_b64 = get_subject_b64(paa_certificate_object)
certificate_akid = get_skid(paa_certificate_object)
elif crl_signer_delegator_cert:
Expand Down
Loading