Support indirect CRLs in generate-revocation-set.py #36502
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
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)
|
hwooley
reviewed
Nov 30, 2024
hwooley
reviewed
Nov 30, 2024
hwooley
reviewed
Nov 30, 2024
hwooley
reviewed
Nov 30, 2024
| issuer_name_b64 = get_issuer_b64(initial_cert) | ||
| akid = get_akid(initial_cert) | ||
| if akid is None: | ||
| return |
There was a problem hiding this comment.
This seems like an exception? At a minimal, should we log an error before returning?
There was a problem hiding this comment.
I’ve added a logging at error level. Note that get_akid will already log as well if its not found.
There was a problem hiding this comment.
I think the get_akid() show just throw and the get_paa_cert() could catch but print the exception detail. It's not clear to me if we should treat exception as "None" in get_akid(). Alternatively, you can check if the extension exists, if so return the value, otherwise, return None, otherwise, let the runtime exception be thrown and the get_paa_cert() catch that printing the exception detail. WDYT?
There was a problem hiding this comment.
I prefer to keep error handling consolidated in the get_akid and get_skid methods instead of doing it repeatedly outside the methods. However, since it is our understanding that CAs should be compliant with RFC5280 and that requires them to include the AKID and SKID, I think its best to let the exception be thrown. Note that get_extension_for_oid method will throw an ExtensionNotFound exception if it does not exist.
There was a problem hiding this comment.
OK. So the exception can be thrown only if get() does not find the extension or if something else is missing even if the extension is found?
hwooley
reviewed
Nov 30, 2024
hwooley
reviewed
Nov 30, 2024
hwooley
reviewed
Nov 30, 2024
|
|
||
| return issuer_certificate_object | ||
| self.rest_node_url = rest_node_url |
There was a problem hiding this comment.
Can we add basic URL checks to make sure it's of the correct form (i.e. ://:)?
There was a problem hiding this comment.
What are the allowed values preceding .dcl? Also, do you need to worry about the trailing slash after .org? Technically, one can have ":" as optional HTTP URL, not sure if we should account for that with RegEx.
There was a problem hiding this comment.
It should just be the main or test net URLs:
PRODUCTION_NODE_URL_REST = "https://on.dcl.csa-iot.org"
TEST_NODE_URL_REST = "https://on.test-net.dcl.csa-iot.org"
I've updated to limit the scope to just those subdomains and allow for an optional trailing slash. For the ":" i dont think i understand. The : in "https://" is not optional. I'll reach directly for clarification. Thanks
There was a problem hiding this comment.
I mean the port section which follows the FQDN, just to be comprehensive wih the pattern matching. For example, you can have http[s]://fqdn[:port].
There was a problem hiding this comment.
Of course, I see. I've updated the regex and simplified it a bit to cover all these options.
hwooley
reviewed
Nov 30, 2024
hwooley
reviewed
Nov 30, 2024
hwooley
reviewed
Nov 30, 2024
hwooley
reviewed
Nov 30, 2024
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think that's critical failure if any part of the processing fails unless we think it's okay to mask the problem.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We could introduce an option to "continue" when encountering failures for the best attempt route. WDYT?
bh3000
commented
Dec 9, 2024
| @@ -508,7 +617,7 @@ def main(use_main_net_dcld: str, use_test_net_dcld: str, use_main_net_http: bool | |||
| entry = { | |||
| "type": "revocation_set", | |||
| "issuer_subject_key_id": certificate_akid_hex, | |||
| "issuer_name": certificate_authority_name_b64, | |||
| "issuer_name": certificate_authority_name.rfc4514_string(), | |||
There was a problem hiding this comment.
Note: i've updated the output to provide the human readable issuer_name.
| issuer_name_b64 = get_issuer_b64(initial_cert) | ||
| akid = get_akid(initial_cert) | ||
| if akid is None: | ||
| return |
There was a problem hiding this comment.
I’ve added a logging at error level. Note that get_akid will already log as well if its not found.
|
|
||
| return issuer_certificate_object | ||
| self.rest_node_url = rest_node_url |
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: