Skip to content

Add ASN.1 ENUMERATED tag support #307

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

Jump to bottom
Merged
merged 2 commits into from
Nov 28, 2019
Merged

Add ASN.1 ENUMERATED tag support #307

merged 2 commits into from
Nov 28, 2019

Conversation

msopiha-linaro
Copy link
Contributor

Add support for ASN.1 ENUMERATED tag.

@gilles-peskine-arm
Copy link
Collaborator

Thank you for your contribution! Even if the code is very simple, can you please add a few test cases in tests/suites/test_suite_asn1parse? We do have a test coverage backlog there (ASN.1 used to be mostly tested via X.509, but nowadays X.509 lives in a separate library), but we prefer to at least test new features.

There's no code to parse ENUMERATED. Would you be able to add that as well? That's not a blocker (we don't require the feature set of asn1parse and asn1write to be identical), but it would be nice.

@msopiha-linaro
Copy link
Contributor Author

Thank you for your contribution! Even if the code is very simple, can you please add a few test cases in tests/suites/test_suite_asn1parse? We do have a test coverage backlog there (ASN.1 used to be mostly tested via X.509, but nowadays X.509 lives in a separate library), but we prefer to at least test new features.

There's no code to parse ENUMERATED. Would you be able to add that as well? That's not a blocker (we don't require the feature set of asn1parse and asn1write to be identical), but it would be nice.

Sure, I will add few tests. Do you want it in current PR, or it should be a new one?

@gilles-peskine-arm
Copy link
Collaborator

Please add the tests in the same PR.

@msopiha-linaro
Copy link
Contributor Author

msopiha-linaro commented Oct 31, 2019 •
edited
Loading

I have extended first commit by adding parser for ENUMERATED tag value.
I have also added test cases for writing/parsing enumerated values, based on ASN1_INTEGER test cases.

@msopiha-linaro msopiha-linaro force-pushed the development branch 2 times, most recently from a68fbbc to acab185 Compare October 31, 2019 14:13
gilles-peskine-arm
gilles-peskine-arm requested changes Oct 31, 2019
View reviewed changes
Copy link
Collaborator

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

I found a few documentation and style issues but otherwise the code looks good to me.

tests/scripts/recursion.pl library/asn1write.c is failing and that's a blocker. I don't know what it's complaining about.

@gilles-peskine-arm gilles-peskine-arm added enhancement New feature or request needs: work The pull request needs rework before it can be merged. labels Oct 31, 2019
@msopiha-linaro
Add ASN.1 ENUMERATED tag support
20180ca 
Add ASN.1 ENUMERATED [1] tag to supported tag list.

1. https://tools.ietf.org/html/rfc3641#page-8

Signed-off-by: Mykhailo Sopiha <[email protected]>
@msopiha-linaro
Add test cases for ASN.1 ENUMERATED tag
6af7bf9 
Add test cases for writing and parsing ASN.1 ENUMERATED
tag values.

Signed-off-by: Mykhailo Sopiha <[email protected]>
@msopiha-linaro
Copy link
Contributor Author

I see that some tests are failing, but I don't have access. Is there any info about that?

@gilles-peskine-arm
Copy link
Collaborator

All the test failures are due to a defective board on our CI. The rest passes and there's enough working coverage to validate this PR.

@gilles-peskine-arm gilles-peskine-arm added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Nov 4, 2019
@gilles-peskine-arm
Copy link
Collaborator

gilles-peskine-arm commented Nov 12, 2019 •
edited
Loading

New full CI run now that the boards are working again (internal link): https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-307-merge/9/ → PASS

@gilles-peskine-arm gilles-peskine-arm added needs: ci Needs a passing full CI run and removed needs: ci Needs a passing full CI run labels Nov 12, 2019
msopiha-linaro added a commit to msopiha-linaro/kmgk that referenced this pull request Nov 13, 2019
@msopiha-linaro
keymaster: refactor attestation routines
c67c01c 
Last major part of PTA refactoring. Remove PTA from
the process of attestation extension generation. Details:

- add keymaster_param_t parser
- add helper functions for mbedTLS asn1 parser
- remove asn1.[ch] files
- add asn1 sequence functions
- add asn1 multiple-octet tag functions
- squash RSA/EC attestation function to a single one
- remove ASN1 PTA part from attestation code

Note: this commit won't work without mbedtls support for
ASN1 ENUMERATED tag processing. Working on upstreaming [1]
meanwhile using local version. See my optee fork, branch
'km3support'[2].

1. ARMmbed/mbed-crypto#307
2. https://github.com/msopiha-linaro/optee_os/tree/km3support

Signed-off-by: Mykhailo Sopiha <[email protected]>
@msopiha-linaro
Copy link
Contributor Author

Is anything required from me on this PR?

@gilles-peskine-arm
Copy link
Collaborator

You don't need to do anything else. All pull requests need to be approved by two team members, so the next action on this PR is a second review. If that's an approval, we'll merge the PR.

msopiha-linaro added a commit to msopiha-linaro/kmgk that referenced this pull request Nov 18, 2019
@msopiha-linaro
keymaster: refactor attestation routines
b6adc23 
Last major part of PTA refactoring. Remove PTA from
the process of attestation extension generation. Details:

- add keymaster_param_t parser
- add helper functions for mbedTLS asn1 parser
- remove asn1.[ch] files
- add asn1 sequence functions
- add asn1 multiple-octet tag functions
- squash RSA/EC attestation function to a single one
- remove ASN1 PTA part from attestation code

Note: this commit won't work without mbedtls support for
ASN1 ENUMERATED tag processing. Working on upstreaming [1]
meanwhile using local version. See my optee fork, branch
'km3support'[2].

1. ARMmbed/mbed-crypto#307
2. https://github.com/msopiha-linaro/optee_os/tree/km3support

Signed-off-by: Mykhailo Sopiha <[email protected]>
@gilles-peskine-arm gilles-peskine-arm merged commit 7bb1a7e into ARMmbed:development Nov 28, 2019
@gilles-peskine-arm gilles-peskine-arm removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Nov 28, 2019
gilles-peskine-arm added a commit to gilles-peskine-arm/mbed-crypto that referenced this pull request Feb 3, 2020
@gilles-peskine-arm
Update crypto submodule
5af2941 
* ARMmbed#321: Replace config.pl by config.py
* ARMmbed#322: Update Mbed Crypto with latest Mbed TLS changes as of 2019-11-15
* ARMmbed#308: Small performance improvement of mbedtls_mpi_div_mpi()
* ARMmbed#324: test_psa_constant_names: support key agreement, better code structure
* ARMmbed#320: Link to the PSA crypto portal page from README.md
* ARMmbed#293: Always gather MBEDTLS_ENTROPY_BLOCK_SIZE bytes of entropy
* ARMmbed#310: Clarify test descriptions in test_suite_memory_buffer_alloc
* ARMmbed#307: Add ASN.1 ENUMERATED tag support
* ARMmbed#328: Remove dependency of crypto_values.h on crypto_extra.h
* ARMmbed#325: Rename psa_asymmetric_{sign_verify} to psa_{sign,verify}_hash

Missed listing in the previous submodule update:

* ARMmbed#304: Make sure Asan failures are detected in 'make test'
vchong pushed a commit to linaro-swg/kmgk that referenced this pull request Feb 10, 2020
@msopiha-linaro
keymaster: refactor attestation routines
0ed20d9 
Last major part of PTA refactoring. Remove PTA from
the process of attestation extension generation. Details:

- add keymaster_param_t parser
- add helper functions for mbedTLS asn1 parser
- remove asn1.[ch] files
- add asn1 sequence functions
- add asn1 multiple-octet tag functions
- squash RSA/EC attestation function to a single one
- remove ASN1 PTA part from attestation code

Note: this commit won't work without mbedtls support for
ASN1 ENUMERATED tag processing. Working on upstreaming [1]
meanwhile using local version. See my optee fork, branch
'km3support'[2].

1. ARMmbed/mbed-crypto#307
2. https://github.com/msopiha-linaro/optee_os/tree/km3support

Signed-off-by: Mykhailo Sopiha <[email protected]>
vchong pushed a commit to linaro-swg/kmgk that referenced this pull request Feb 10, 2020
@msopiha-linaro
keymaster: refactor attestation routines
de03424 
Last major part of PTA refactoring. Remove PTA from
the process of attestation extension generation. Details:

- add keymaster_param_t parser
- add helper functions for mbedTLS asn1 parser
- remove asn1.[ch] files
- add asn1 sequence functions
- add asn1 multiple-octet tag functions
- squash RSA/EC attestation function to a single one
- remove ASN1 PTA part from attestation code

Note: this commit won't work without mbedtls support for
ASN1 ENUMERATED tag processing. Working on upstreaming [1]
meanwhile using local version. See my optee fork, branch
'km3support'[2].

1. ARMmbed/mbed-crypto#307
2. https://github.com/msopiha-linaro/optee_os/tree/km3support

Signed-off-by: Mykhailo Sopiha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@dgreen-arm dgreen-arm dgreen-arm approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@msopiha-linaro @gilles-peskine-arm @dgreen-arm