Support indirect CRLs in generate-revocation-set.py #36502
+157
−58
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review changes with |
|
|
|
PR #36502: Size comparison from acea464 to 0edc3ce Full report (24 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, telink, tizen)
|
|
PR #36502: Size comparison from 355a2a6 to 83e687e Full report (53 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36502: Size comparison from 73ff58f to 1b306c5 Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Comment on lines
323
to
344
| 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)}") |
There was a problem hiding this comment.
Can you please abstract all the data-getting into a new interface (base class) + 2 concrete classes for dcld and HTTP REST respectively. This would make it easier to maintain and split the code of the different methods of getting the raw data (i.e. what is the input of load_pem_x509_certificate) into common code.
There was a problem hiding this comment.
Good thinking, that will be useful for the next change that allows for local files to be used. The interface has been added, the two classes, and the main impl updated to pick between the two. Thanks!
Comment on lines
+20
to
+25
| # 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 |
There was a problem hiding this comment.
The constraints.txt of the SDK should just ensure that the version of the module obtained during bootstrap is correct.
There was a problem hiding this comment.
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.
|
PR #36502: Size comparison from 093aff8 to 794679f Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36502: Size comparison from e782f53 to b8f41ea Full report (11 builds for cc13x4_26x4, cc32xx, qpg, stm32, tizen)
|
|
PR #36502: Size comparison from e75d6da to e6c3c6f Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes included: