ECC-step3: extra import/export (ASN.1 related) #187

karel-m · 2017-04-04T08:59:41Z

This PR was extracted from #65

I have written this code in 2014 (just an excuse in case it starts to seem that I am not completely sure what these changes are good for).

The main reason for this patch was ASN.1 support for importing/exporting EC keys which are stored in ASN.1 structures using esoteric stuff like context specific tags and optional items in ASN.1 sequences.

You will notice on the first sight the ugly duplication in API that I have introduced (to keep backwards compatibility of "non *_ex" functions):

der_decode_subject_public_key_info
der_decode_subject_public_key_info_ex

and

der_length_sequence
der_length_sequence_ex

sjaeckel · 2017-04-04T09:29:04Z

src/pk/asn1/der/sequence/der_decode_sequence_ex.c

           case LTC_ASN1_BOOLEAN:
               z = inlen;
               if ((err = der_decode_boolean(in + x, z, ((int *)data))) != CRYPT_OK) {
+                   if (!ordered || list[i].optional) { continue; }


was this a bug before or this one now?

Well, I am not sure.

Looking at the code I believe that missing if (!ordered) { continue; } was a bug

sjaeckel · 2017-04-04T09:30:53Z

src/pk/asn1/der/sequence/der_encode_sequence_ex.c

-      goto LBL_ERR;
-   }
+   y = 0; z = 0;
+   if ((err = der_length_sequence_ex(list, inlen, &y, &z)) != CRYPT_OK) return CRYPT_INVALID_ARG;


👍 I like this change!

Can I extract this single change as a separate "small fix" PR?

sjaeckel · 2017-04-04T09:33:26Z

src/pk/asn1/der/sequence/der_encode_sequence_ex.c

           case LTC_ASN1_CONTEXT_SPECIFIC:
           case LTC_ASN1_EOL:
           case LTC_ASN1_TELETEX_STRING:
+           default:


#65 (comment)

coveralls · 2017-04-04T12:54:09Z

Coverage decreased (-0.2%) to 87.605% when pulling c666bc4 on pr/ecc-asn1-part into fbe7d22 on develop.

sjaeckel · 2017-04-04T13:55:04Z

IIUC this implements context-specific tag encoding differently than in the flexi decoder (it's stored in tag and we currently use the used field to store). Should we then also adjust the flexi decoder or is this not the same?

karel-m · 2017-04-04T18:24:37Z

Ad context-specific tag encoding - when I added tag I probably did not notice that used might already exist for the same reason.

I am in no way an expert at ASN.1, when I created the patch back in 2014 the only goal was to be able to load/save EC keys which I somehow managed. But some parts are rather hacks/workarounds.

If you want some testing keys, go to https://github.com/DCIT/perl-CryptX/tree/master/t/data and take any of ECC related *.der files.

sjaeckel · 2017-04-05T22:45:36Z

src/pk/asn1/der/sequence/der_encode_sequence_ex.c

+            tmptag[4] = (unsigned char)(z&255);
+            y = 5;
+         }
+         memmove(out + x + y, out + x, z);


XMEMMOVE()?

sjaeckel · 2017-04-05T22:53:54Z

Ad context-specific tag encoding - when I added tag I probably did not notice that used might already exist for the same reason.

As of the comment on the used field we could just re-use it instead of adding tag. Most likely it'd make sense to rename it to something else. Initially it was just kind of a hack so the struct definition won't change. As I don't see a way to somehow mangle the optional part without changing the struct I think a rename would be fine.

I am in no way an expert at ASN.1, when I created the patch back in 2014 the only goal was to be able to load/save EC keys which I somehow managed. But some parts are rather hacks/workarounds.

If you want some testing keys, go to https://github.com/DCIT/perl-CryptX/tree/master/t/data and take any of ECC related *.der files.

I'm also no expert at ASN.1 but I'll have a look when I have the time.

Would it probably make sense to split this PR up into two parts? One with the changes for tag and one for optional and 'the rest'?

sjaeckel · 2017-04-06T21:11:01Z

src/pk/asn1/der/sequence/der_length_sequence.c

          break;
       }

+       /* some items may be optional during import */


is this one correct? this looks fishy to me as the used field is only used when decoding but der_length_sequence_ex() is used when encoding

The der_length_sequence (which calls der_length_sequence_ex) is called during decoding - see der_decode_sequence_ex.c.

Possible theory: it seems that during decode operation I use optional to indicate that it is ok if the item is missing and used to indicate whether it was really decoded.

Yeah sure, I missed that. Sorry.

karel-m · 2017-06-21T21:01:02Z

Now rebased on top of ECC enhancements part 1 (curves y^2 = x^3 + ax + b) #236

karel-m · 2018-06-03T18:11:39Z

@sjaeckel we should rebase this one onto a develop

I have created a branch pr/ecc-asn1-part-BACKUP because AFAIK you have some uncommitted stuff related to this PR.

sjaeckel · 2018-06-03T18:16:48Z

IIUC that's only 1 commit which is left

and I've already rebased locally, so go ahead

karel-m · 2018-06-03T18:24:00Z

Don't you have some pkcs8 changes/improvements locally?

I have not done any coding in this PR/branche since you have asked me whether you can work on pkcs8. Therefore I think it would be better if you push --force your local changes.

sjaeckel · 2018-06-03T18:53:40Z

Don't you have some pkcs8 changes/improvements locally?

yeah, but nothing ready for publishing yet as I didn't really have the time ...

karel-m · 2018-06-03T20:13:48Z

OK, now rebased.

Is it worth splitting this PR into:

ecc_export_openssl + ecc_import_openssl + ecc_import_x509 (which is IMO ready for being merged into develop, except maybe a discussion whether the name _openssl is proper here or we can find something better)
ecc_import_pkcs8 (here are definitely some opportunities for improvement + I think the rsa_import_pkcs8 deserves a similar facelift as well)

?

karel-m · 2018-06-04T18:17:07Z

@sjaeckel could you please review: ecc_export_openssl + ecc_import_openssl + ecc_import_x509?

karel-m · 2018-06-07T20:02:53Z

@sjaeckel I have moved pkcs8 stuff to #403 - could you please review/approve what's left in this PR? (ecc_export_openssl + ecc_import_openssl + ecc_import_x509)

sjaeckel · 2018-06-10T11:51:11Z

src/pk/ecc/ecc_import_openssl.c

+
+   len_xy = sizeof(bin_xy);
+   len_oid = 16;
+   err = x509_decode_subject_public_key_info(in, inlen, PKA_EC, bin_xy, &len_xy,


What do you think about factoring-out those tries in their own functions?

_ecc_import_x509_with_oid() _ecc_import_x509_with_curve() ecc_import_subject_public_key_info() /* private API which calls the two above */ ecc_import_private_with_oid() /* private API */ ecc_import_private_with_curve() /* private API */

This would also allow to call ecc_import_subject_public_key_info() from ecc_import_x509() instead of this multi-purpose import function.

Frankly, it looks like too much work for more of less no gain. As it will not be part of public API we can change it any-time later.

My doubts here are more about whether ecc_export_openssl or ecc_export_<something_else> (and the same for ecc_import_openssl) would be better. Or generally whether the new API is fine as changing/renaming it later will be a pain.

As you can see from the code the available options for saving/loading an ecc key are pretty wide. For ecc_import_openssl I have chosen the approach "load whatever DER the openssl is able to save" therefore the name and therefore a single, maybe slightly overloaded, import function.

Frankly, it looks like too much work for more of less no gain

I think 30 minutes for a bit more readability is well invested.

Why I really did it was that I didn't like the fact that it would've been possible to import a private-key from a SubjectPublicKeyInfo.

👍 Looks good. Thank you.

... into several smaller functions

karel-m self-assigned this Apr 4, 2017

karel-m requested a review from sjaeckel April 4, 2017 09:25

sjaeckel reviewed Apr 4, 2017

View reviewed changes

sjaeckel reviewed Apr 5, 2017

View reviewed changes

sjaeckel reviewed Apr 6, 2017

View reviewed changes

sjaeckel modified the milestone: v2.0.0 Apr 7, 2017

karel-m force-pushed the pr/ecc-asn1-part branch 4 times, most recently from b12bef7 to eb3efc3 Compare May 3, 2017 15:28

karel-m force-pushed the pr/ecc-asn1-part branch from eb3efc3 to c258a84 Compare May 11, 2017 22:40

sjaeckel mentioned this pull request Jun 6, 2017

Add RSA key importing functionality for OpenSSL private keys #223

Closed

sjaeckel added the enhancement label Jun 7, 2017

sjaeckel modified the milestones: v1.18.0, next Jun 8, 2017

karel-m force-pushed the pr/ecc-asn1-part branch 2 times, most recently from cb50664 to c4024b6 Compare June 14, 2017 07:31

karel-m force-pushed the pr/ecc-asn1-part branch 2 times, most recently from a3114ec to f9f8364 Compare June 21, 2017 12:46

karel-m mentioned this pull request Jun 21, 2017

ECC-step2: curves y^2 = x^3 + ax + b #236

Merged

karel-m force-pushed the pr/ecc-asn1-part branch from 5c3a455 to 1162ac9 Compare June 21, 2017 20:58

karel-m changed the title ~~ASN.1 changes needed for ECC enhancements~~ ECC enhancements part 2 (ASN.1 changes) Jun 21, 2017

karel-m force-pushed the pr/ecc-non-asn1-part branch 2 times, most recently from 54ba133 to e0c7170 Compare May 4, 2018 08:30

karel-m force-pushed the pr/ecc-non-asn1-part branch from e0c7170 to 885f81d Compare May 5, 2018 16:03

karel-m force-pushed the pr/ecc-non-asn1-part branch 2 times, most recently from 0cd0205 to 362275e Compare May 13, 2018 09:33

karel-m force-pushed the pr/ecc-non-asn1-part branch from 362275e to 24c0eb8 Compare May 22, 2018 21:03

karel-m changed the base branch from pr/ecc-non-asn1-part to develop June 3, 2018 17:59

karel-m force-pushed the pr/ecc-asn1-part branch from 24c3dea to dc396e8 Compare June 3, 2018 20:06

karel-m force-pushed the pr/ecc-asn1-part branch 5 times, most recently from d9d203a to 50ffa85 Compare June 4, 2018 16:44

karel-m force-pushed the pr/ecc-asn1-part branch 2 times, most recently from 2a446c5 to 95fe5ea Compare June 7, 2018 19:41

karel-m mentioned this pull request Jun 7, 2018

Improved pkcs8 support (OBSOLETE) #403

Closed

sjaeckel reviewed Jun 10, 2018

View reviewed changes

karel-m and others added 3 commits June 11, 2018 10:00

Update makefiles

abedfa1

ECC improved import/export

05d397d

re-factor ecc_import_openssl()

4f36e03

... into several smaller functions

sjaeckel force-pushed the pr/ecc-asn1-part branch from 57ede5d to 4f36e03 Compare June 11, 2018 08:00

sjaeckel approved these changes Jun 11, 2018

View reviewed changes

sjaeckel merged commit 6238b63 into develop Jun 11, 2018

ECC-step3: extra import/export (ASN.1 related) #187

ECC-step3: extra import/export (ASN.1 related) #187

Uh oh!

Conversation

karel-m commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 4, 2017

Uh oh!

sjaeckel commented Apr 4, 2017

Uh oh!

karel-m commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjaeckel commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karel-m commented Jun 21, 2017

Uh oh!

karel-m commented Jun 3, 2018

Uh oh!

sjaeckel commented Jun 3, 2018

Uh oh!

karel-m commented Jun 3, 2018

Uh oh!

sjaeckel commented Jun 3, 2018

Uh oh!

karel-m commented Jun 3, 2018

Uh oh!

karel-m commented Jun 4, 2018

Uh oh!

karel-m commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sjaeckel commented Apr 5, 2017 •

edited

Loading