Add method for encoding a public key as in SEC1

[sirk390]

Hi Anton, what do you think about adding something like this? It is common to encode public keys in SEC1 format as openssl does + compressed format is useful as it takes less space. You didn't have any method for it so I added this.
I needed it in my project while implementing a proto ECIES implementation.

[AntonKueltz]

All the encoding logic should probably have its own modules. The Point class is itself agnostic to how it is encoded in that all it should know is how to do group operations for elliptic curve points. May be worth looking into making a subpackage for binary encodings e.g. this, ASN1 / DER encoding.

[sirk390]

Yes sure I didn't know exactly where to put this in the current structure. If it is in the asn1 module (which I did first but didn't like due to the already similar named encoding public key) maybe it is required to renamed the other encoding methods to something more specific (including RFC name or other)

[AntonKueltz]

Merging this to it's own branch for now, haven't had much time to think about how to best organize the encoding files / methods.

[AntonKueltz]

fastecdsa.encoding was added in commit e312d5f on the sec1-encoding branch. I'm still thinking about how to define the interfaces in fastecdsa.encoding.__init__.py but I think the general approach works, reduces clutter in files and (most importantly!) doesn't break any prior functionality.

The new functions that you added for DER and SEC1 encoding do have new locations now but I think the current breakdown helps a lot with clarity and clean code. I'm happy to discuss further if you have any feedback / comments. I'm still going to test this a bit more before merging into master.

Cheers,
Anton

[sirk390]

Hi, Yes, it looks better organized. A few comments : * I would put the methods "encode_private_key", "decode_private_key" of RFC5915, RFC5480, RFC2459 as @*staticmethod *to make them simpler to call (They shouldn't need a state, but grouping them by class is still nice because you can pass the class as argument) * I would rename rfc5915, rfc5480, rfc2459 modules and classes to something more clear, as most people (at least myselve) don't remember the RFC numbers. It is nice to have them, but an extra helper would be nice " *rfc2459_der*.py" , " *rfc5915_pem*.py" and the same for Classes inside. Otherwise, you spend a lot of time searching for some functionality. * The README / or doc should probably updated on the main page to explain this new functionality * I'm not sure "keys.export_key" "keys.import_key" are very useful (they don't really do anything except calling "encoder.X()" which you could already directly). It theory, they could be removed, but this might be an issue with compatibility. * The docstrings would needs to be updated for "keys.*import_key" and "keys.export_key" *(it speaks about PEM encoding + doesn't list the available Encoders which would be nicer so that people don't have to navigate the modules) * It might be interesting to consider adding a PublicKey and a PrivateKey class (other libraries have this) although it is not important to me. * It might be cleaner if "sec1" had a class as well to contain "encode" and "decode" functions. If you need any help don't hesitate! Cheers, Christian

…

On Wed, Jan 16, 2019 at 9:58 AM AntonKueltz ***@***.***> wrote: fastecdsa.encoding was added in commit e312d5f <e312d5f> on the sec1-encoding branch. I'm still thinking about how to define the interfaces in fastecdsa.encoding.__init__.py but I think the general approach works, reduces clutter in files and (most importantly!) doesn't break any prior functionality. The new functions that you added for DER and SEC1 encoding do have new locations now but I think the current breakdown helps a lot with clarity and clean code. I'm happy to discuss further if you have any feedback / comments. I'm still going to test this a bit more before merging into master. Cheers, Anton — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8CGzUEgouLMjXIJyAlqW5Hwcc3tmx5ks5vDumvgaJpZM4Zl2QR> .

[AntonKueltz]

Some thoughts -

I would put the methods "encode_private_key", "decode_private_key" of RFC5915, RFC5480, RFC2459 as @*staticmethod *to make them simpler to call (They shouldn't need a state, but grouping them by class is still nice because you can pass the class as argument)

I agree, but I don't know of a way to make their interface / abc enforce that the abstractmethods are also staticmethods (that is python2 compatible). The whole idea behind the KeyEncoder and SigEncoder interface is that you pass any derived class of those two to the import / export calls and it takes care of the decoding / encoding for you. Might be something that just needs to be documented for anyone implementing a new encoder.

I would rename rfc5915, rfc5480, rfc2459 modules and classes to something more clear, as most people (at least myselve) don't remember the RFC numbers. It is nice to have them, but an extra helper would be nice " rfc2459_der.py" , " rfc5915_pem.py" and the same for Classes inside. Otherwise, you spend a lot of time searching for some functionality.

I think this is a good idea, I think changing the class names to PemEncoder / DerEncoder might also make more sense, that way it's clear what does what when passing the encoders around to functions.

I'm not sure "keys.export_key" "keys.import_key" are very useful (they don't really do anything except calling "encoder.X()" which you could already directly). It theory, they could be removed, but this might be an issue with compatibility.

Yes the calls are now more for compatibility than anything else, and to provide some reasonable defaults for users who don't want to dive into the world of binary encodings and formats and just want to have a consistent way of exporting and importing keys.

It might be interesting to consider adding a PublicKey and a PrivateKey class (other libraries have this) although it is not important to me.

I always found using these class based approaches to keys a bit cumbersome and overly OOP-based. EC keys are pretty easy to represent and are just scalars and points on curves so representing them and using them as such seems straightforward to me (same with signatures just being two scalars and not a bloated class). This is more of a personal preference but I think it makes it more clear what things are and adds less OOP bloat. I did consider making a drop-in interface that replicates what ecdsa has but it seems like a lot of code for minimal value.

It might be cleaner if "sec1" had a class as well to contain "encode" and "decode" functions.

Agree. Feel free to change this however you see fit, I haven't worked much with SEC1 encoding so I leave the details to you. :)

[AntonKueltz]

I pushed the updates in f0854d6 and 600bbbd. The only thing of note is, in order for the SEC1 and PEM encoder classes to be consistent I changed the SEC1Encoder.decode_public_key parameter order to take the encoded key as the first param and the curve as the second param. Feel free to change this however you need to.

[AntonKueltz]

Once some more documentation is added to the README and everything looks OK to you I think we can merge to master.

[sirk390]

Hi Anton, I like the changes. I tried it quickly on my use-case and it works fine. It's also nice that you renamed functions like "_int_to_bytes" to "int_to_bytes" as they can be really useful outside the library. The new SEC1 parameter order is not a problem, and better for consistency. For me it is all ok. You can push it after adding some more doc to README.

[AntonKueltz]

Documented and merged in 5c7d02a.