Test (missing) RSA key support in Secure Session #710

iamnotacake · 2020-08-18T16:04:51Z

Add few specific tests to Go, NodeJS, Python, Ruby, C++ wrappers. These tests make sure the wrapper API gives error (exception) when we try to create Secure Session with a key different from EC private one.

While adding these tests, few bugs were discovered in Python and Ruby wrappers. To be precise, it was wrong way of checking whether secure_session_create() returned NULL. The tests were not failing as expected. This PR also brings fixes for them.

Speaking about other wrappers, some of them seem to handle this kind of errors as well: Java (JNI), PHP, another PHP, JS (WASM)
And others may be missing the check: Objective C

I've also updated .gitignore to include some stuff like generated Ruby packages, some Python package metadata, another Rust target directory.

Checklist

Change is covered by automated tests
The coding guidelines are followed
Changelog is updated

Add tests to make sure Python wrapper rejects any keys except EC private ones when creating SSession. Fix SSession and SComparator (the comparison of returned pointer was wrong). Here's example: ```python >>> import ctypes >>> null = ctypes.POINTER(ctypes.c_int)() >>> not_null = ctypes.POINTER(ctypes.c_int)(ctypes.c_int(1)) >>> # bool(x) -- good way to compare, returns False for NULL >>> bool(null), bool(not_null) (False, True) >>> # not x -- also works >>> not null, not not_null (True, False) >>> # ctypes.POINTER is never None >>> null is None, not_null is None (False, False) >>> null == None, not_null == None (False, False) ``` For more info, see https://docs.python.org/3/library/ctypes.html

Add tests to make sure Ssession creating fails when wrong/unsupported key was provided. Also, fix FFI usage where comparing things with NULL was done in wrong way and in case of NULL, no expected exception was raised.

Do the tests with valid peer ID so the only way for constructor to fail will be passing wrong key. The third arg may still be an empty function since it won't be called before doing handshake.

ilammy · 2020-08-18T16:35:50Z

And others may be missing the check: Objective C

Yep, it definitely is missing the check. The correct way will be to check for self.session value and return nil out of the initializer.

Please keep a note of which wrappers still need checks and tests.

ilammy

Overall, I like it! 👍

Are more wrappers expected in this PR of this is all for now?

gothemis/session/session_test.go

src/wrappers/themis/jsthemis/test/test.js

tests/pythemis/test_ssession.py

src/wrappers/themis/ruby/lib/rbthemis.rb

tests/rbthemis/ssession_test.rb

ilammy · 2020-08-18T16:54:59Z

tests/themispp/secure_session_test.hpp

-static const std::vector<uint8_t> client_public_key = as_bytes(client_pub);
+/* Test keys can be found in secure_session_test_keys.hpp */


Thanks ❤️

Verify that Secure Session has been constructed successfully. This should catch RSA keys being passed to the initializer earlier. Unfortunately, Secure Session is not tested in ObjCThemis at all at the moment so no tests verify this behavior.

ilammy · 2020-08-18T17:15:09Z

@iamnotacake, currently I don't have my macBook available so I'm in the same boat as you: I can only make blind changes and test them by submitting it to CI 😁 Since you have been kind enough to enable the Allow edits by maintainers check box, I have simply pushed the commit into this branch (please git pull it on your machine). Unfortunately, Secure Session is not tested at all in ObjCThemis so no tests at the moment.

ilammy · 2020-08-18T17:21:35Z

Also, since this PR does change the behavior of some wrappers, I believe it should update the changelog for relevant languages.

iamnotacake · 2020-08-18T17:56:44Z

Also, since this PR does change the behavior of some wrappers, I believe it should update the changelog for relevant languages.

Does this look good?

- **Python**

  - Fixed bug where `SSession` constructor did not throw an exception if `secure_session_create()` returned `NULL` ([#710](https://github.com/cossacklabs/themis/pull/710)).

- **Ruby**

  - Fixed bug where `Ssession` constructor did not throw an exception if `secure_session_create()` returned `NULL` ([#710](https://github.com/cossacklabs/themis/pull/710)).

BTW, should we put news in changelog in alphabetical order or append them (in the same _Code:_ section) or it doesn't matter?
And if we add these things to changelog, I guess #698 should be mentioned too?

UPD: Maybe you mean mentioning that sec session wrappers don't allow RSA keys anymore? Or both this and bugfixes? Now I'm a lil bit confised

vixentael

Thank you! These are good changes.

I suggest to add test to check EC private keys as well (test should pass), because right now we have tests for RSA pub/priv and EC pub, which looks like EC priv are missing.

src/wrappers/themis/jsthemis/test/test.js

vixentael · 2020-08-18T17:46:52Z

src/wrappers/themis/python/pythemis/scomparator.py

@@ -42,7 +42,7 @@ class SComparator(object):
    def __init__(self, shared_secret):
        self.session_ctx = ctypes.POINTER(ctypes.c_int)
        self.comparator_ctx = scomparator_create()
-        if self.comparator_ctx is None:
+        if not self.comparator_ctx:


I'd ask @Lagovas to take a look here

it's okay here. secure_comparator_create will return NULL (0 or None as python types) which will mean False in if condition

vixentael · 2020-08-18T17:59:47Z

src/wrappers/themis/python/pythemis/ssession.py

@@ -143,7 +143,7 @@ def __init__(self, user_id, sign_key, transport):
                ctypes.byref(ctypes.create_string_buffer(sign_key)),
                len(sign_key),
                ctypes.byref(self.transport_))
-        if self.session_ctx is None:
+        if not self.session_ctx:


and here @Lagovas

tests/themispp/secure_session_test.hpp

ilammy · 2020-08-18T18:07:56Z

@vixentael,

I suggest to add test to check EC private keys as well (test should pass)

Aren't EC private keys – the happy path – tested in other tests that verify that Secure Session works as expected? Or you suggest just a complementary test for the constructor alone to complete the puzzle?

vixentael · 2020-08-18T18:10:35Z

you suggest just a complementary test for the constructor alone to complete the puzzle?

yes, I suggest to complete the puzzle, because future us will totally forget why one case is not tested (which looks like we've missed that case)

ilammy · 2020-08-18T18:13:56Z

@iamnotacake,

Does this look good?
[...]
Maybe you mean mentioning that sec session wrappers don't allow RSA keys anymore? Or both this and bugfixes? Now I'm a lil bit confised

The changelog is mostly for the users and they probably don't (and shouldn't) know about Themis Core functions. So I'd write there something like "${Secure Session constructor name} now (throws an exception | returns null | does a dance) when given incorrect keys", or maybe RSA keys specifically.

The idea is to describe the expected behavior to the user. With this change, the constructors may start failing due to incorrect keys. This changelog entry might help debugging.

BTW, should we put news in changelog in alphabetical order or append them (in the same Code: section) or it doesn't matter?

I'd ask to keep them alphabetical. That's one predictable order to look for a language.

And if we add these things to changelog, I guess #698 should be mentioned too?

Yeah, makes sense. Please do.

* Update changelog, mention fixes of NodeJS, Python, Ruby wrappers * Add tests to ensure EC private keys are accepted by Secure Session (JS, Go, Python, Ruby, C++) * Add comment to JS tests aboud why huge base64-encoded key is inside the tests code * Small refactor in Python tests and Ruby wrapper

gothemis/session/session_test.go

CHANGELOG.md

tests/themispp/secure_session_test_keys.hpp

* Mention about TSSession fixes in ObjC and Swift in changelog * Small refactor of Go tests no not only check error, but also check whether result is as expected (nil result and not nil error or vice-versa) * Small refactor of Ruby tests, rewrite them in a better-looking way * Fix typos in C++ tests

ilammy

Yup. Nicely done, good to merge! 👍

iamnotacake added 7 commits August 18, 2020 17:12

Add Go tests

63e3302

Add JS tests

05f3732

Add Ruby tests, fix Ruby wrapper

7fd71c5

Add tests to make sure Ssession creating fails when wrong/unsupported key was provided. Also, fix FFI usage where comparing things with NULL was done in wrong way and in case of NULL, no expected exception was raised.

Add C++ tests

fc6d8c4

Update .gitignore

3c74146

Fixup for JS tests

fcced76

Do the tests with valid peer ID so the only way for constructor to fail will be passing wrong key. The third arg may still be an empty function since it won't be called before doing handshake.

iamnotacake added W-PyThemis 🐍 Wrapper: PyThemis, Python API W-RbThemis ♦️ Wrapper: RbThemis, Ruby API, Ruby gems W-GoThemis 🐹 Wrapper: GoThemis, Go API tests Themis test suite W-JsThemis 🍭 Wrapper: JsThemis for Node.js, JavaScript API, npm packages labels Aug 18, 2020

iamnotacake requested review from ilammy and vixentael August 18, 2020 16:04

ilammy changed the title ~~Add more tests to wrappers~~ Test (missing) RSA key support in Secure Session Aug 18, 2020

ilammy reviewed Aug 18, 2020

View reviewed changes

iamnotacake added the W-ThemisPP ⚔️ Wrapper: ThemisPP, C++ API label Aug 18, 2020

vixentael reviewed Aug 18, 2020

View reviewed changes

tests/themispp/secure_session_test.hpp Outdated Show resolved Hide resolved

iamnotacake added 2 commits August 19, 2020 12:44

Alphabetically sort code-related news in changelog

c7b1b2f

Lagovas reviewed Aug 19, 2020

View reviewed changes

gothemis/session/session_test.go Outdated Show resolved Hide resolved

gothemis/session/session_test.go Outdated Show resolved Hide resolved

ilammy reviewed Aug 19, 2020

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

vixentael reviewed Aug 19, 2020

View reviewed changes

iamnotacake added 2 commits August 19, 2020 19:12

Merge branch 'master' into anatolii-T1738-add-more-tests-to-wrappers

8c26ba1

vixentael approved these changes Aug 19, 2020

View reviewed changes

vixentael requested review from ilammy and Lagovas August 19, 2020 16:59

ilammy approved these changes Aug 19, 2020

View reviewed changes

iamnotacake merged commit 858f23f into cossacklabs:master Aug 19, 2020

iamnotacake deleted the anatolii-T1738-add-more-tests-to-wrappers branch August 20, 2020 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test (missing) RSA key support in Secure Session #710

Test (missing) RSA key support in Secure Session #710

iamnotacake commented Aug 18, 2020 •

edited by ilammy

Loading

ilammy commented Aug 18, 2020

ilammy left a comment

ilammy Aug 18, 2020

ilammy commented Aug 18, 2020

ilammy commented Aug 18, 2020

iamnotacake commented Aug 18, 2020 •

edited

Loading

vixentael left a comment

vixentael Aug 18, 2020

Lagovas Aug 19, 2020

vixentael Aug 18, 2020

Lagovas Aug 19, 2020 •

edited

Loading

ilammy commented Aug 18, 2020

vixentael commented Aug 18, 2020

ilammy commented Aug 18, 2020

ilammy left a comment

		static const std::vector<uint8_t> client_public_key = as_bytes(client_pub);
		/* Test keys can be found in secure_session_test_keys.hpp */

Test (missing) RSA key support in Secure Session #710

Test (missing) RSA key support in Secure Session #710

Conversation

iamnotacake commented Aug 18, 2020 • edited by ilammy Loading

Checklist

ilammy commented Aug 18, 2020

ilammy left a comment

Choose a reason for hiding this comment

ilammy Aug 18, 2020

Choose a reason for hiding this comment

ilammy commented Aug 18, 2020

ilammy commented Aug 18, 2020

iamnotacake commented Aug 18, 2020 • edited Loading

vixentael left a comment

Choose a reason for hiding this comment

vixentael Aug 18, 2020

Choose a reason for hiding this comment

Lagovas Aug 19, 2020

Choose a reason for hiding this comment

vixentael Aug 18, 2020

Choose a reason for hiding this comment

Lagovas Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

ilammy commented Aug 18, 2020

vixentael commented Aug 18, 2020

ilammy commented Aug 18, 2020

ilammy left a comment

Choose a reason for hiding this comment

iamnotacake commented Aug 18, 2020 •

edited by ilammy

Loading

iamnotacake commented Aug 18, 2020 •

edited

Loading

Lagovas Aug 19, 2020 •

edited

Loading