Remove NOCSR Request size limits and fix ReadDerLength bugs #28899
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
- Some implementers ran out of space when adding even a single extension. - 400 is the same size as the max compressed cert format, which we use many places. The total increase is thus small, for a transient buffer. - ReadDerLength was private and insufficiently tested. Some issues were found by @bluebin14. Testing done: - Existing test coverage passes - Added exhaustive unit tests for ReadDerLength, covering all failing cases, and expanding it usability beyond 8 bits of range.
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
gjc13,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo and
jtung-apple
pullapprove
bot
requested review from
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
|
PR #28899: Size comparison from c3fb124 to 37a462a Increases (33 builds for bl702, bl702l, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, psoc6, qpg)
Decreases (4 builds for linux)
Full report (62 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
bluebin14
reviewed
Aug 26, 2023
- Works in every situation. - Keeps existing constant as deprecated since main legacy usage will still work. - Remove a needless verification of size that was the root cause of issues with the constant before.
tcarmelveilleux
changed the title
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
|
PR #28899: Size comparison from c3fb124 to dac0e5b Increases (30 builds for bl702, bl702l, cc32xx, cyw30739, esp32, linux, nrfconnect, psoc6, telink)
Decreases (5 builds for efr32, linux)
Full report (62 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
bluebin14
approved these changes
Aug 28, 2023
bzbarsky-apple
approved these changes
Aug 28, 2023
Yes |
andy31415
approved these changes
Aug 28, 2023
tcarmelveilleux
added a commit
to tcarmelveilleux/connectedhomeip
that referenced
this pull request
Aug 29, 2023
mergify bot
pushed a commit
that referenced
this pull request
Aug 29, 2023
* Fix corner cases of handling of Common Name fallback encoding Problem: - Appearance of a Mpid:/Mvid: in a DAC/PAI/PAA DN was deemed OK by previous code, but this caused a critical ambiguity in PAIs which would possibly cause fall-back to non-PID-scoped PAI interpretation. - Related to CHIP-Specifications/connectedhomeip-spec#7470 - Fixes #28898 This PR: - Replaces the logic for fallback encodign conversion to take the first legitimate fully matching case for Mvid: and Mpid: and detect errors where either of these is present but without a following Mpid/Mvid. - Updates unit tests to improve coverage and to properly mark as invalid some cases marked invalid in spec which where deemed valid by prior code by mistake Testing done: - Integration tests still pass (relater to Commissioner DUT). - Test vectors updated. - New unit tests added. * Restyled by clang-format * Restyled by prettier-json * Address review comments by revamping algorithm * Fix leftover comment follow-ups from @bzbarsky-apple from #28899 * Restyled by clang-format * Add more comments and fix clang-tidy * Address more review comments --------- Co-authored-by: tennessee.carmelveilleux@gmail.com <tennessee@google.com> Co-authored-by: Restyled.io <commits@restyled.io>
github-actions bot
pushed a commit
that referenced
this pull request
Aug 29, 2023
* Fix corner cases of handling of Common Name fallback encoding Problem: - Appearance of a Mpid:/Mvid: in a DAC/PAI/PAA DN was deemed OK by previous code, but this caused a critical ambiguity in PAIs which would possibly cause fall-back to non-PID-scoped PAI interpretation. - Related to https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/7470 - Fixes #28898 This PR: - Replaces the logic for fallback encodign conversion to take the first legitimate fully matching case for Mvid: and Mpid: and detect errors where either of these is present but without a following Mpid/Mvid. - Updates unit tests to improve coverage and to properly mark as invalid some cases marked invalid in spec which where deemed valid by prior code by mistake Testing done: - Integration tests still pass (relater to Commissioner DUT). - Test vectors updated. - New unit tests added. * Restyled by clang-format * Restyled by prettier-json * Address review comments by revamping algorithm * Fix leftover comment follow-ups from @bzbarsky-apple from #28899 * Restyled by clang-format * Add more comments and fix clang-tidy * Address more review comments --------- Co-authored-by: tennessee.carmelveilleux@gmail.com <tennessee@google.com> Co-authored-by: Restyled.io <commits@restyled.io>
HunsupJung
pushed a commit
to HunsupJung/connectedhomeip
that referenced
this pull request
Oct 23, 2023
…chip#28899) * Increase available size for NOCSR Request - Some implementers ran out of space when adding even a single extension. - 400 is the same size as the max compressed cert format, which we use many places. The total increase is thus small, for a transient buffer. - ReadDerLength was private and insufficiently tested. Some issues were found by @bluebin14. Testing done: - Existing test coverage passes - Added exhaustive unit tests for ReadDerLength, covering all failing cases, and expanding it usability beyond 8 bits of range. * Restyled by clang-format * Replace Max CSR size with a Min CSR size. - Works in every situation. - Keeps existing constant as deprecated since main legacy usage will still work. - Remove a needless verification of size that was the root cause of issues with the constant before. * Update src/crypto/CHIPCryptoPAL.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Restyled by clang-format --------- Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
HunsupJung
pushed a commit
to HunsupJung/connectedhomeip
that referenced
this pull request
Oct 23, 2023
…t-chip#28911) * Fix corner cases of handling of Common Name fallback encoding Problem: - Appearance of a Mpid:/Mvid: in a DAC/PAI/PAA DN was deemed OK by previous code, but this caused a critical ambiguity in PAIs which would possibly cause fall-back to non-PID-scoped PAI interpretation. - Related to CHIP-Specifications/connectedhomeip-spec#7470 - Fixes project-chip#28898 This PR: - Replaces the logic for fallback encodign conversion to take the first legitimate fully matching case for Mvid: and Mpid: and detect errors where either of these is present but without a following Mpid/Mvid. - Updates unit tests to improve coverage and to properly mark as invalid some cases marked invalid in spec which where deemed valid by prior code by mistake Testing done: - Integration tests still pass (relater to Commissioner DUT). - Test vectors updated. - New unit tests added. * Restyled by clang-format * Restyled by prettier-json * Address review comments by revamping algorithm * Fix leftover comment follow-ups from @bzbarsky-apple from project-chip#28899 * Restyled by clang-format * Add more comments and fix clang-tidy * Address more review comments --------- Co-authored-by: tennessee.carmelveilleux@gmail.com <tennessee@google.com> Co-authored-by: Restyled.io <commits@restyled.io>
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.
Testing done: