Add apidsl file for toxencryptsave. #310

iphydf · 2016-12-10T11:37:10Z

This change is

GrayHatter · 2016-12-11T00:12:16Z

GrayHatter · 2016-12-11T00:12:30Z

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

zetok · 2016-12-13T01:56:24Z

toxencryptsave/toxencryptsave.h

    /**
-     * Some input data, or maybe the output pointer, was null.
+     * One of the arguments to the function was NULL when it was not expected.


When is NULL expected? Is it expected at all? Isn't this comment misleading?

Sometimes NULL is accepted. is_data_encrypted just returns false, because NULL is not encrypted. This comment is generated by apidsl. Feel free to file a bug or make a PR with different wording there: https://github.com/TokTok/apidsl/issues.

zetok · 2016-12-13T02:06:44Z

toxencryptsave/toxencryptsave.h

@@ -190,59 +222,66 @@ typedef struct {
 *
 * returns true on success
 */
-bool tox_derive_key_from_pass(const uint8_t *passphrase, size_t pplength, TOX_PASS_KEY *out_key,
-                              TOX_ERR_KEY_DERIVATION *error);
+bool tox_pass_key_derive_from_pass(struct Tox_Pass_Key *_key, const uint8_t *passphrase, size_t pplength,


tox_pass_key_derive_from_pass()

Redundant department of redundant redundancy is calling. This function doesn't make much sense as far as the name goes.

zetok · 2016-12-13T02:06:59Z

toxencryptsave/toxencryptsave.h

 * The salt must be TOX_PASS_SALT_LENGTH bytes in length.
 */
-bool tox_derive_key_with_salt(const uint8_t *passphrase, size_t pplength, const uint8_t *salt, TOX_PASS_KEY *out_key,
-                              TOX_ERR_KEY_DERIVATION *error);
+bool tox_pass_key_derive_with_salt(struct Tox_Pass_Key *_key, const uint8_t *passphrase, size_t pplength,


*passphrase

This is plain wrong.

zetok · 2016-12-13T02:13:18Z

toxencryptsave/toxencryptsave.h

+
+void tox_pass_key_free(struct Tox_Pass_Key *_key);
+
+void tox_pass_key_get_key(const struct Tox_Pass_Key *_key, uint8_t *key);


No errors are possible? Should this actually read TOX_PASS_KEY_LENGTH bytes from given place in memory?
Why there are no docs on how to use it?
If it's something that shouldn't be used, why there's an API for it?

Same goes for other functions.

iphydf · 2016-12-13T03:06:06Z

Added to v0.1.0. I'll push it to v0.2.0 if we can't get it reviewed on time.

zetok · 2016-12-13T03:48:10Z

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.

toxencryptsave/toxencryptsave.h, line 205 at r2 (raw file):

 */
bool tox_pass_encrypt(const uint8_t *plain, size_t plain_len, const uint8_t *passphrase, size_t passphrase_len,
                      uint8_t *cipher, TOX_ERR_ENCRYPTION *error);

cipher as a name for output data is confusing, perhaps it should be renamed to e.g. ciphertext?
While at renaming, plain isn't that good either. Not sure if it could be renamed, given that plaintext isn't much more friendly (perhaps a bit).

Same goes for naming in other functions.

Comments from Reviewable

iphydf · 2016-12-13T11:13:16Z

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.

toxencryptsave/toxencryptsave.h, line 205 at r2 (raw file):

Previously, zetok wrote…

cipher as a name for output data is confusing, perhaps it should be renamed to e.g. ciphertext?
While at renaming, plain isn't that good either. Not sure if it could be renamed, given that plaintext isn't much more friendly (perhaps a bit).

Same goes for naming in other functions.

Done.

Comments from Reviewable

robinlinden · 2016-12-13T14:05:26Z

Reviewed 2 of 6 files at r2, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

auto_tests/encryptsave_test.c, line 107 at r3 (raw file):

    Tox_Pass_Key *key = tox_pass_key_new();
    ck_assert_msg(key != NULL, "pass key allocation failure");
    memcpy((uint8_t *)key, test_salt, 32);

key->salt and key->key may be clearer. If you disagree, then maybe at least use TOX_PASS_SALT_LENGTH instead of 32 when comparing and using offsets.

Comments from Reviewable

This breaks the toxencryptsave API. It hides the Tox_Pass_Key struct definition.

robinlinden · 2016-12-13T14:30:48Z

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

iphydf · 2016-12-13T14:39:50Z

Review status: all files reviewed at latest revision, 2 unresolved discussions.

auto_tests/encryptsave_test.c, line 107 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

key->salt and key->key may be clearer. If you disagree, then maybe at least use TOX_PASS_SALT_LENGTH instead of 32 when comparing and using offsets.

Done.

Comments from Reviewable

zetok · 2016-12-13T14:51:37Z

Public API looks ok.

Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Equivalent to the change in TokTok/c-toxcore#310 where pass-key internals are no longer exposed publicly, since there is no need to expose them.

iphydf · 2016-12-13T20:20:45Z

@nurupo we can make modifications or undo this change if you have reasons for that later. The 0.1.0 release is still gated on your LGTM. I will merge the toxencryptsave API soon.

nurupo · 2016-12-13T20:25:39Z

Ok. I will have some reviewing time in 6 hours.

nurupo · 2016-12-14T08:51:55Z

Reviewed 1 of 6 files at r2, 3 of 4 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

toxencryptsave/toxencryptsave.h, line 51 at r4 (raw file):

<unresolved>

toxencryptsave/toxencryptsave.h, line 263 at r4 (raw file):

 */
void tox_pass_key_free(struct Tox_Pass_Key *_key);

Why _key?

Comments from Reviewable

GrayHatter · 2016-12-14T10:04:14Z

Reviewed 2 of 6 files at r2, 3 of 4 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

toxencryptsave/toxencryptsave.api.h, line 54 at r4 (raw file):

 * ${pass_Key.derive}, then use the derived key to encrypt the data.
 *
 * The encrypted data is prepended with a magic number, to aid validity

Is the term here not cookie?

A magic number is something that makes things work, where why could easily unknown.

Comments from Reviewable

nurupo · 2016-12-15T02:29:53Z

toxencryptsave/toxencryptsave.api.h, line 54 at r4 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Is the term here not cookie?

A magic number is something that makes things work, where why could easily unknown.

Magic number is the correct term here https://en.wikipedia.org/wiki/Magic_number_(programming).

Comments from Reviewable

iphydf added this to the v0.2.0 milestone Dec 10, 2016

iphydf mentioned this pull request Dec 10, 2016

Review TES and port it to APIDSL #308

Closed

GrayHatter self-assigned this Dec 11, 2016

iphydf force-pushed the toxes-api branch from ef3657e to ca7606a Compare December 12, 2016 22:01

iphydf assigned GrayHatter, nurupo and zetok and unassigned GrayHatter Dec 12, 2016

iphydf force-pushed the toxes-api branch 3 times, most recently from 09fd6fb to 3597599 Compare December 12, 2016 22:09

zetok reviewed Dec 13, 2016

View reviewed changes

iphydf force-pushed the toxes-api branch from 3597599 to 315fdd9 Compare December 13, 2016 02:06

zetok reviewed Dec 13, 2016

View reviewed changes

iphydf changed the title ~~Add apidsl file for toxencryptsave.~~ [WIP] Add apidsl file for toxencryptsave. Dec 13, 2016

iphydf force-pushed the toxes-api branch 3 times, most recently from 2729b4d to cad3eea Compare December 13, 2016 03:03

iphydf changed the title ~~[WIP] Add apidsl file for toxencryptsave.~~ Add apidsl file for toxencryptsave. Dec 13, 2016

iphydf modified the milestones: v0.1.0, v0.2.0 Dec 13, 2016

iphydf force-pushed the toxes-api branch 3 times, most recently from 8c8d655 to 6c1a5ab Compare December 13, 2016 03:34

iphydf force-pushed the toxes-api branch from 6c1a5ab to 6c800b6 Compare December 13, 2016 11:12

Add apidsl file for toxencryptsave.

4cf6999

This breaks the toxencryptsave API. It hides the Tox_Pass_Key struct definition.

iphydf force-pushed the toxes-api branch from 6c800b6 to 4cf6999 Compare December 13, 2016 14:25

iphydf assigned robinlinden Dec 13, 2016

zetok added a commit to zetok/tox that referenced this pull request Dec 13, 2016

fix(TES): remove from public API internals of PassKey

4d243c0

Equivalent to the change in TokTok/c-toxcore#310 where pass-key internals are no longer exposed publicly, since there is no need to expose them.

zetok mentioned this pull request Dec 13, 2016

fix(TES): remove from public API internals of PassKey zetok/tox#34

Merged

iphydf unassigned nurupo Dec 13, 2016

iphydf unassigned GrayHatter Dec 13, 2016

iphydf merged commit 4cf6999 into TokTok:master Dec 13, 2016

nurupo mentioned this pull request Dec 14, 2016

Release 0.1.0. #316

Merged

iphydf deleted the toxes-api branch December 23, 2016 02:30

nurupo mentioned this pull request Jan 9, 2018

Review all public API for 0.3.0 #655

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add apidsl file for toxencryptsave. #310

Add apidsl file for toxencryptsave. #310

iphydf commented Dec 10, 2016 •

edited

Loading

GrayHatter commented Dec 11, 2016 •

edited by iphydf

Loading

GrayHatter commented Dec 11, 2016

zetok Dec 13, 2016

iphydf Dec 13, 2016

zetok Dec 13, 2016

zetok Dec 13, 2016

zetok Dec 13, 2016

iphydf commented Dec 13, 2016

zetok commented Dec 13, 2016

iphydf commented Dec 13, 2016

robinlinden commented Dec 13, 2016

robinlinden commented Dec 13, 2016

iphydf commented Dec 13, 2016

zetok commented Dec 13, 2016

iphydf commented Dec 13, 2016

nurupo commented Dec 13, 2016

nurupo commented Dec 14, 2016

GrayHatter commented Dec 14, 2016

nurupo commented Dec 15, 2016


		void tox_pass_key_free(struct Tox_Pass_Key *_key);

		void tox_pass_key_get_key(const struct Tox_Pass_Key _key, uint8_t key);

Add apidsl file for toxencryptsave. #310

Add apidsl file for toxencryptsave. #310

Conversation

iphydf commented Dec 10, 2016 • edited Loading

GrayHatter commented Dec 11, 2016 • edited by iphydf Loading

GrayHatter commented Dec 11, 2016

zetok Dec 13, 2016

Choose a reason for hiding this comment

iphydf Dec 13, 2016

Choose a reason for hiding this comment

zetok Dec 13, 2016

Choose a reason for hiding this comment

zetok Dec 13, 2016

Choose a reason for hiding this comment

zetok Dec 13, 2016

Choose a reason for hiding this comment

iphydf commented Dec 13, 2016

zetok commented Dec 13, 2016

iphydf commented Dec 13, 2016

robinlinden commented Dec 13, 2016

robinlinden commented Dec 13, 2016

iphydf commented Dec 13, 2016

zetok commented Dec 13, 2016

iphydf commented Dec 13, 2016

nurupo commented Dec 13, 2016

nurupo commented Dec 14, 2016

GrayHatter commented Dec 14, 2016

nurupo commented Dec 15, 2016

iphydf commented Dec 10, 2016 •

edited

Loading

GrayHatter commented Dec 11, 2016 •

edited by iphydf

Loading