Symmetric keygen: Themis Core #560

ilammy · 2019-12-04T11:03:33Z

Implement symmetric key generation utilities described in RFC 1 (not available publicly at the moment). This is new API.

We are going to need a way to let our users generate strong symmetric keys for use with Secure Cell. This core function is the implementation of key generation. High-level wrappers will be using it to implement key generation.

The API has the usual “check – allocate – output” style found in other places in Themis:

themis_status_t themis_gen_sym_key(uint8_t* key, size_t* key_length);

The users are expected to first pass NULL for key to learn the default length, then allocate a suitable buffer and call the function again to receive key bytes. After that the key can be immediately used with Secure Cell. (Tests and examples for Secure Cell will be updated in next pull requests.)

Checklist

Change is covered by automated tests
~~Benchmark results are attached~~ (not interesting)
The coding guidelines are followed
Public API has proper documentation (hell yeah)
~~Example projects and code samples are updated~~ (will do later)
Changelog is updated

Properly group and document all members of this file. The definition of themis_key_kind_t is tweaked a bit so that Doxygen does not generate two entries for both the enum and the typedef. However, we still keep the "enum themis_key_kind" tag available.

Implement themis_gen_sym_key() function in Themis Core. Add tests. It's pretty much documented in doxygen comments. This is the actual key generation implementation which will be used by all wrappers.

Lagovas · 2019-12-04T11:11:13Z

src/themis/secure_keygen.h

    THEMIS_KEY_INVALID,
+    /** Private RSA key. */


for what these comment lines which duplicate enum values?

for what these comment lines which duplicate enum values?

Doxygen output looks better this way. If we describe enumerators then the enumeration gets a 'detailed description' section like all other declarations.

Though yeah, it does feel like infamous Java getter docs:

/** * Returns success value. * * Use this method to get success value as boolean. * * @return boolean value `true` if successful, `false` if not successful. */ public boolean getSuccess() { return succes; }

🙄

Lagovas · 2019-12-04T11:15:21Z

tests/themis/themis_secure_cell.c

+
+    res = themis_gen_sym_key(master_key, &master_key_length);
+    testsuite_fail_unless(res == THEMIS_SUCCESS, "keygen: generate new key");
+    testsuite_fail_unless(master_key_length > 0, "keygen: key length returned");


will be better to more precisely compare result key length and check that master_key_length == MAX_KEY_SIZE

plus in tests of key generation, we should check that our generation function did something. for example compare that values of an array were changed after generation and not equal to previous values. plus we should check that each call of function return new random values and user can call this function as many times as he want to get new key, without calling somehing like RAND_seed(...) in openssl. So we should check at least 2 calls and verify that it return different values

we should check that each call of function return new random values

Sounds fair. It seems a good idea to test whether the cryptographic backend is sane. Standby for more tests.

Verify that we can generate big keys too, up to MAX_KEY_SIZE. Verify that key generation routines actually fill in the key buffer when they return THEMIS_SUCCESS, and that every invocation returns a different key.

Lagovas · 2019-12-04T13:44:09Z

tests/themis/themis_secure_cell.c

+
+    res = themis_gen_sym_key(NULL, &master_key_1_length);
+    testsuite_fail_unless(res == THEMIS_BUFFER_TOO_SMALL, "keygen: query key size");
+    testsuite_fail_unless(master_key_1_length > 0, "keygen: recommends non-empty key");


maybe will be better to check that result length is THEMIS_SYM_KEY_LENGTH. with this check we will be sure that our generation function use this definition, not some hardcoded values or took elsewhere.

This constant is not exported by design so that the users won't think that keys always have some fixed length, won't start preallocating fixed buffers, etc. We don't export key length constants for asymmetric keys too. The users just need to know that key length is some positive number. Currently we allow anything other than zero.

This check ensures that we never return zero length as it fails both key generation (in its current implementation) and future use of invalid key with Secure Cell (which does not allow zero-sized keys). I don't consider this a particular magic number.

If anything, I'd probably check that it's >= 32 — so that we never shorten the keys, only expand them — but that will be a magic number for real. If we export and use a constant directly then this check will allow us to redefine it to 16 without breaking tests.

Thus... I'd leave it as is.

we don't export but we should be sure that this define used there and works as expected. it is not integration tests, it is unit test that verifies that function works as expected. we as authors expect that the key should be generated with predefined default value and check that it is expected value imho

We don't really have a suitable header to put it into, like <themis/secure_keygen_t.h> and I don't really want to introduce a new one just for this constant. Putting it into some random private header like <themis/secure_session_t.h> is also not nice.

I'd like to have this constant not visible when including <themis/themis.h> so that we don't have to maintain it as a part of public API. If it's visible then someone will notice it and use it, they could very well put a uint8_t themis_key[THEMIS_SYM_KEY_LENGTH] somewhere into their data structures, then write them to disk, and now we cannot ever change this length to 64 because this breaks binary compatibility for the users.

If anything, I'd probably go with a copy for tests:

#define DEFAULT_SYM_KEY_LENGTH 32

then check that we return this value when the buffer is too small. It's not the same macro constant, but it has the same value, so if anyone changes THEMIS_SYM_KEY_LENGTH then they'll have to change it in the other place as well to fix the test. It's a bit more strict than checking for > 0 and avoids unnecessary exports.

Any second opinion? @vixentael, @ignatk, @storojs72?

anyway, we will describe this in our docs, that default behavior of our function for key generation is to use random key with length=32. imho it's not some private value. it's the same as we describe that we use aes with 256 block size.
and yes, more opinion will be great to choose the final decision)

ignatk · 2019-12-04T13:51:22Z

src/themis/secure_keygen.c

+        return THEMIS_INVALID_PARAMETER;
+    }
+    if (key != NULL && *key_length == 0) {
+        return THEMIS_INVALID_PARAMETER;


shouldn't this be "buffer too small" as well?

shouldn't this be "buffer too small" as well?

Mmm... Yeah, it probably should.

My reasoning was that requesting zero-length key is an error, but it can be equally understood as "zero-length buffer is too small, please expand".

I've checked asymmetric key generation API and it returns THEMIS_BUFFER_TOO_SMALL when given non-NULL buffer and zero length. It also outputs proper length into the provided pointer (like when buffer is NULL). So I guess we'll use the same behavior:

- if (key != NULL && *key_length == 0) { - return THEMIS_INVALID_PARAMETER; - } - - if (key == NULL) { + if (key == NULL || *key_length == 0) { *key_length = THEMIS_SYM_KEY_LENGTH; return THEMIS_BUFFER_TOO_SMALL; }

Thanks for noticing!

Asymmetric key generation API considers zero length "too small", not an error, and outputs a proper length while returning approriate error code. Update symmetric key generation behavior for consistency.

After fierce discussion we have reached a compromise: we should do a more strict check than "returns length greater that zero", but we do not want to export THEMIS_SYM_KEY_LENGTH constant. Copying its value for tests is acceptable.

Lagovas

lgtm

vixentael · 2019-12-05T11:53:12Z

Fantastic work! Merge 'em all! 👾

ilammy added 2 commits December 4, 2019 07:38

Symmetric keygen: Themis Core

34cd83a

Implement themis_gen_sym_key() function in Themis Core. Add tests. It's pretty much documented in doxygen comments. This is the actual key generation implementation which will be used by all wrappers.

ilammy added the core Themis Core written in C, its packages label Dec 4, 2019

ilammy requested review from Lagovas, storojs72 and vixentael as code owners December 4, 2019 11:03

Lagovas requested changes Dec 4, 2019

View reviewed changes

Additional tests with sanity checks

2f82040

Verify that we can generate big keys too, up to MAX_KEY_SIZE. Verify that key generation routines actually fill in the key buffer when they return THEMIS_SUCCESS, and that every invocation returns a different key.

ilammy requested a review from Lagovas December 4, 2019 12:11

Lagovas reviewed Dec 4, 2019

View reviewed changes

ignatk reviewed Dec 4, 2019

View reviewed changes

ilammy added 2 commits December 4, 2019 16:35

Handle zero length consistently

99a2b82

Asymmetric key generation API considers zero length "too small", not an error, and outputs a proper length while returning approriate error code. Update symmetric key generation behavior for consistency.

More strict check for default key length

5262257

After fierce discussion we have reached a compromise: we should do a more strict check than "returns length greater that zero", but we do not want to export THEMIS_SYM_KEY_LENGTH constant. Copying its value for tests is acceptable.

This was referenced Dec 4, 2019

Symmetric keygen: high-level wrappers #561

Merged

Symmetric keygen: JsThemis #562

Merged

Symmetric keygen: JavaThemis #565

Merged

Lagovas approved these changes Dec 4, 2019

View reviewed changes

vixentael approved these changes Dec 5, 2019

View reviewed changes

ilammy merged commit 06d27e6 into cossacklabs:master Dec 6, 2019

ilammy deleted the themis_gen_sym_key/core branch December 6, 2019 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Symmetric keygen: Themis Core #560

Symmetric keygen: Themis Core #560

ilammy commented Dec 4, 2019

Lagovas Dec 4, 2019

ilammy Dec 4, 2019

Lagovas Dec 4, 2019

ilammy Dec 4, 2019

Lagovas Dec 4, 2019

ilammy Dec 4, 2019

Lagovas Dec 4, 2019

ilammy Dec 4, 2019

Lagovas Dec 4, 2019

ignatk Dec 4, 2019

ilammy Dec 4, 2019

Lagovas left a comment

vixentael commented Dec 5, 2019

Symmetric keygen: Themis Core #560

Symmetric keygen: Themis Core #560

Conversation

ilammy commented Dec 4, 2019

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lagovas left a comment

Choose a reason for hiding this comment

vixentael commented Dec 5, 2019