Skip to content
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.

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

soter: Support uncompressed EC keys #954

Merged
merged 6 commits into from Oct 9, 2022

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Oct 1, 2022

Themis can accept uncompressed public keys already. But it also performs paranoid checks that keys have exactly the size they should have, with all implementation choices and quirks. Newer implementations of Themis avoid those quirks and use uncompressed format for efficiency. Make sure such keys are accepted by this Themis.

Note: Themis still produces compressed keys, as before. However, it is now able to read uncompressed keys produced... elsewhere.

Checklist

  • Change is covered by automated tests
    • The tests actuall pass lol
  • Benchmark results are attached (not important)
  • The coding guidelines are followed
  • Changelog is updated (in case of notable or breaking changes)

I'm going to change things up soon, no longer possible to define
private key size as public key size directly.
Due to hysterical raisins, Themis has an extra zero byte in exported
private key representation. Currently, we enforce this and reject
keys which do not have that byte. However, newer versions will be
producing proper keys. Make sure those work, since those are good.
Similarly, Themis always produces compressed public keys, however
the underlying implementation for OpenSSL and BoringSSL can accept
uncompressed keys without any issues. Allow those as well.
Main user of EC keys are probably Secure Messages. Add a test there
to ensure that uncompressed keys keep working.

This brings the number of tests to 300.
@ilammy ilammy added the core Themis Core written in C, its packages label Oct 1, 2022
@ilammy ilammy marked this pull request as draft October 1, 2022 12:36
I guess Clang wants these rectangles to be as filled as possible?
@ilammy ilammy marked this pull request as ready for review October 3, 2022 10:52
Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

It doesn't break backwards-compatibility; it gives compatibility with elsewhere. I like it.

@vixentael vixentael merged commit dde159a into cossacklabs:master Oct 9, 2022
@vixentael vixentael deleted the uncompressed-ec-keys branch October 9, 2022 22:17
@Lagovas
Copy link
Collaborator

Lagovas commented Oct 12, 2022

yay, when I saw Support uncompressed EC keys subject, I was hoping that you added a directive that turns on using uncompressed keys in themis and it will generate ciphertexts and keys in uncompressed form)

@ilammy
Copy link
Collaborator Author

ilammy commented Oct 15, 2022

I was hoping that you added a directive that turns on using uncompressed keys in themis

That should be relatively easy to add as well. Update this line:

!= EC_POINT_point2oct(group,
Q,
POINT_CONVERSION_COMPRESSED,
(unsigned char*)(key + 1),
output_length - sizeof(soter_container_hdr_t),
NULL)) {

and a bunch of others for public key sizes, and the same thing for BoringSSL.

Though, I dunno whether it's worth doing, since downstream projects might be hardcoding key sizes and they'd be in for a surprise if the layout changes. Well, they'd have to deal with it when migrating to Themis NG, but that sounds more justified than having to deal with it during run-of-the-mill 0.14 → 0.15 upgrade (if it ever happens, haha).

I'm not particularly keen on redesigning key generation API to support this, but maybe some other option will do? Say, THEMIS_USE_UNCOMPRESSED_KEYS=1 environment variable? Some magic global state in the library? A blessed function in GoThemis to decompress a key?

How badly do you want time savings from not having to decompress keys?

I don't have numbers handy to copypaste them, but my benchmarks show that ECDH outstrips key decompression by quite a large margin. If anything, I'd rather fix Secure Message implementation to avoid doing ECDH twice, which would be much greater boost to performance than generating uncompressed keys – and with zero compatibility concerns.

Opinions, @Lagovas, @vixentael?

@Lagovas
Copy link
Collaborator

Lagovas commented Oct 17, 2022

It was just opinion. I remember, that it is easy to do (same as you described). But not an idea to do as next step.

Say, THEMIS_USE_UNCOMPRESSED_KEYS=1 environment variable?

Yeah, I thought about that approach because it save backward compatibility but allows to turn on manually if you know what to do and what you want.

How badly do you want time savings from not having to decompress keys?

As I remember, it saved ~30% of time in secure message logic. The most complicated operations in secure message algorithm is decompressing point because deals with big numbers to calculate Y. You do to computation of shared key that uses big nums too. And then using symmetric encryption after KDF to encrypt data using with a shared key. And point decompression looks like expensive operation which can be avoided by spending more storage space.

@vixentael
Copy link
Contributor

Say, THEMIS_USE_UNCOMPRESSED_KEYS=1 environment variable?

+1 to this approach. But I think that we should not change the default settings for the current Themis to avoid backwards compatibility issues, rather just to implement this option for our own needs & compatibility with Themis NG.

@ilammy ilammy added this to the 0.15.0 milestone Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Themis Core written in C, its packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants