Refactor ec key import #229
Conversation
| python3 -c "from urllib.request import urlretrieve; urlretrieve('${{ env.BUILDER_HOST }}/${{ env.BUILDER_SOURCE }}/${{ env.BUILDER_VERSION }}/builder.pyz?run=${{ env.RUN }}', 'builder')" | ||
| chmod a+x builder | ||
| ./builder build -p ${{ env.PACKAGE_NAME }} --compiler=gcc --cmake-extra=-DASSERT_LOCK_HELD=ON --coverage |
There was a problem hiding this comment.
> aws --region us-east-1 secretsmanager get-secret-value --secret-id codecov-token
No token found for aws-c-cal, check https://app.codecov.io/github/awslabs/aws-c-cal/settings for token and add it to codecov-token in secret-manager.
This actually failed, but I didn't have the non-zero exit code in the builder...
Should be fixed by awslabs/aws-crt-builder#340
Also, I put the token in the secrete manager now, so it should work now.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
source/ecc.c
Outdated
|
|
||
| /* tag 0 is optional params */ | ||
| if (aws_der_decoder_tlv_type(decoder) == AWS_DER_CONTEXT_SPECIFIC_TAG0) { | ||
| aws_der_decoder_next(decoder); |
There was a problem hiding this comment.
check for reaching the end or not after _next
There was a problem hiding this comment.
this was somewhat on purpose to account for cases where there is 2 optional fields back to back. anyways rewrote this to make it more clear.
source/ecc.c
Outdated
| if (aws_der_decoder_tlv_type(decoder) == AWS_DER_CONTEXT_SPECIFIC_TAG0) { | ||
| aws_der_decoder_next(decoder); | ||
|
|
||
| if (aws_der_decoder_tlv_type(decoder) == AWS_DER_OBJECT_IDENTIFIER) { |
There was a problem hiding this comment.
lets just hard fail. its not a valid asn if its not object identifier
| struct aws_der_decoder *aws_der_decoder_nested_tlv_decoder(struct aws_der_decoder *decoder) { | ||
| struct aws_byte_cursor cursor; | ||
| AWS_ZERO_STRUCT(cursor); | ||
| if (aws_der_decoder_tlv_string(decoder, &cursor)) { |
There was a problem hiding this comment.
I am not familiar with the der decoder, but looks like this is only used once, and seems like a very specific case the nested der is a string type?
There was a problem hiding this comment.
Yeah, this is a bit of unusual case.
basically instead of inlining the fields of the nested structure, it is turned into octet string.
we need to parse the nested structure, so we need to read the string and start a new decoder.
there are legacy reasons for why its done this way, but they are kinda boring.
this is a helper to initialize a new decoder from a string at current location. we need it once now. there are more keys in the wild that have similar structure, so it might be helpful in general.
im not tied to this, it was just simpler without having to expose allocator for decoder.
source/unix/opensslcrypto_ecc.c
Outdated
| return AWS_OP_SUCCESS; | ||
|
|
||
| on_error: | ||
| EC_POINT_free(point); |
There was a problem hiding this comment.
Loos like our previous logic has a null protection, but not here.
According to Google AI summary:
Safe to use: You can pass NULL to BN_free() without any risk. It is a safe operation, [says the Ubuntu Manpage](https://manpages.ubuntu.com/manpages/noble/man3/BN_clear_free.3ssl.html).
Wow, the doc it linked seems legit.
There was a problem hiding this comment.
openssl is kinda inconsistent about those. those 3 frees are fine and can handle null (ex. https://github.com/aws/aws-lc/blob/0fed4dbc456406bdaf7158fd55d05a52d9998ce4/crypto/fipsmodule/ec/ec.c#L595 and similar in openssl).
but let me add null check just for consistency cause we usually check.
Co-authored-by: Dengke Tang <[email protected]>
3 participants
Issue #, if available:
Description of changes:
Replace the logic of iterating through the asn1 structure looking for fields that seem to match expected ec key, with approach of looking for several concrete key structures.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.