Introduce ASN.1 SEQUENCE traversal API #263
No reviewers
Labels
No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-ARMmbed/mbed-crypto!263
Loading…
Reference in a new issue
No description provided.
Delete branch "asn1_traversal_api"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary: This PR is a spin-out from the X.509 on demand parsing work https://github.com/ARMmbed/mbedtls/pull/2478, one of the main drivers of which is to avoid dynamically allocated presentations of ASN.1 SEQUENCEs by traversing the raw ASN.1 in-place whenever needed. To this end, this PR introduces the ASN.1 API
mbedtls_asn1_traverse_sequence_of()which can be used to traverse an ASN.1 SEQUENCE container, triggering a user-specified callback for each entry found.The PR also introduces two helper macros
MBEDTLS_ASN1_IS_STRING_TAGandMBEDTLS_OID_CMP_RAW. Those are technically unrelated to the SEQUENCE traversal API and are included here solely to keep the number of dependencies for the on demand parsing PR low.https://github.com/ARMmbed/mbed-crypto/pull/75 fixes some edge cases in the ASN.1 library and adds tests. Can you please make sure that your X.509 code and tests are compatible with these fixes? This PR doesn't have tests; I'm willing to accept it without tests but the lack of tests needs to be filed as a separate bug.
@gilles-peskine-arm It's not fully satisfactory, but because the PR reimplements
mbedtls_asn1_get_sequence_of()in terms of the new traversal API, any tests for the former will also test the latter.But of course, it'd be better to have standalone tests, too.
@ -371,0 +423,4 @@** For example, to validate that the input is a SEQUENCE of `tag1` and call* `cb` on each element, use* ```Why should this be experimental? When can it be non-experimental? Could we make asn1_internal.h and place it there if it's only for internal use for now?
@ -371,0 +477,4 @@* parameters:* - \p ctx.* - The tag of the current element.* - A pointer to the start of the current element'sCan the
unsigned charbeconst unsigned char? I don't think we'd be modifying**p, onlypor*p.@ -371,0 +482,4 @@* - The length of the content of the current element.* If the callback returns a non-zero value,* the function stops immediately,* forwarding the callback's return value.Can
startbe made const?Ah, too bad this is not const. I guess we can't have our callback with
pconst because of this.I rebased this pull request on top of the current base branch. The previous content by Hanno is at https://github.com/gilles-peskine-arm/mbed-crypto/tree/asn1_traversal_api-1.
Compared with the previous content:
@ -371,0 +423,4 @@** For example, to validate that the input is a SEQUENCE of `tag1` and call* `cb` on each element, use* ```It isn't internal since it will be used by Mbed TLS.
@ -371,0 +477,4 @@* parameters:* - \p ctx.* - The tag of the current element.* - A pointer to the start of the current element'sconst unsigned char **andunsigned char **do not mix. If this function used a different type, it couldn't be used with the other asn1 parsing functions.@ -371,0 +482,4 @@* - The length of the content of the current element.* If the callback returns a non-zero value,* the function stops immediately,* forwarding the callback's return value.I'm wondering about that. In 99% of the cases it would be const: parsing a read-only string. But it may be useful occasionally to modify an input in place.
New CI run now that the problems with the Cypress boards are fixed: https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-263-merge/20/ → PASS
@ -371,0 +423,4 @@** For example, to validate that the input is a SEQUENCE of `tag1` and call* `cb` on each element, use* ```@gilles-peskine-arm @Patater What's your view on this? This function must be public, as @gilles-peskine-arm said, so what remains is to decide whether we want to declare it as experimental or not. I'm open either way. If you think the API is good as-is, I'm happy to remove the qualification.
@ -371,0 +423,4 @@** For example, to validate that the input is a SEQUENCE of `tag1` and call* `cb` on each element, use* ```My preference is what you wrote: declare this function as experimental. We don't care about this function except as an internal detail of the implementation an X509 feature that we do care about. It has a complex API that I would prefer not to make official, but that I'm ok to include with the understanding that we're likely to modify it in the future.
@ -371,0 +477,4 @@* parameters:* - \p ctx.* - The tag of the current element.* - A pointer to the start of the current element'sI agree with @Patater in that in principle the type of
pshould beconst unsigned char **here. However, the rest of the ASN.1 parsing library unfortunately also usesunsigned char **, which we can't change and which forces the use ofunsigned char **here, too (not just for consistency, but simply because this function calls e.g.mbedtls_asn1_get_tag()which has aunsigned char **argument). An alternative would be to extend the documentation of all ASN.1 parsing functions to explicitly mention that, despite the missingconstqualification, the**pis never written to, and that it is hence safe to use the functions in aconst unsigned char **context. If we did that, we could start improving the API by usingconst unsigned char **at least for this new API function, and doing a const-cast when calling the other parsing functions.@ -371,0 +477,4 @@* parameters:* - \p ctx.* - The tag of the current element.* - A pointer to the start of the current element'sWe can tweak the API in Mbed TLS 3. Until we get there, let's keep the API consistent.
@ -371,0 +482,4 @@* - The length of the content of the current element.* If the callback returns a non-zero value,* the function stops immediately,* forwarding the callback's return value.I agree that it should be
const. However, if we want to call other ASN.1 parsing functions from within the callback, then we can't haveconsthere because those parsing functions don't haveconstqualification, as mentioned above.@ -371,0 +477,4 @@* parameters:* - \p ctx.* - The tag of the current element.* - A pointer to the start of the current element'sOk, no change here, then.
This way it would work, but what doesn't work is calling parsing functions from within
mbedtls_asn1_traverse_sequence_of().Rebased on top of
development. The only conflict stemmed from @yanesca's introduction of default error codes and was easy to fix.The CI found a problem, in
test-ref-configs, withconfig-symmetric-only.h, the first error is:and the other errors look similar. I suggest including
stdint.hdirectly inasn1.h- it probably happens to be included indirectly in some configurations including the default one, but apparently not all of them.Thank you @mpg, I'll fix that.
@mpg @gilles-peskine-arm Fixed the issue by replacing
uint8_tbyunsigned char.@ -371,0 +423,4 @@** For example, to validate that the input is a SEQUENCE of `tag1` and call* `cb` on each element, use* ```The situation should be temporary, since I think we're aiming at making the whole
asn1.hprivate in Mbed TLS 3.0 (or some point in the future at least) anyway.LGTM
CI passes except for the Mbed OS tests which are failing for unrelated reasons.