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

Link libthemis_jni.so dynamically #552

Merged
merged 3 commits into from
Nov 8, 2019
Merged

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Nov 4, 2019

Do not embed Themis, Soter, and cryptographic backend (if present) into libthemis_jni shared object. Instead, link it dynamically against Themis like a proper library.

It has been a nice experiment to provide self-contained library for Java. However, we are going to distribute the library as a proper system package and for that we will need to have it properly linked against its dependencies. Themis Core will be installed by the package manager so all dependencies are going to be satisfied.

Checklist

  • Change is covered by automated tests
  • Benchmark results are attached (if applicable)
  • The coding guidelines are followed
  • Public API has proper documentation
  • Example projects and code samples are updated if needed (in case of API changes)
  • Changelog is updated if needed minor change, will be included into packaging PR

Do not embed Themis, Soter, and cryptographic backend (if present)
into libthemis_jni shared object. Instead, link it dynamically
against Themis like a proper library.

It has been a nice experiment to provide self-contained library for
Java. However, we are going to distribute the library as a proper system
package and for that we will need to have it properly linked against its
dependencies. Themis Core will be installed by the package manager so
all dependencies are going to be satisfied.
These are currently used by Themin JNI wrapper in order to operate
secure_session_load() correctly. Strictly speaking, we should not
be using these functions, but currently there is no way to allocate
Secure Sesssion instance suitable for secure_session_load() without
using private header <themis/secure_sesssion_t.h>. These functions
are exported from the shared library, but we make no commitment to
keeping them exported. The should be hidden back once
secure_session_load() gets a better API.
@ilammy ilammy added the W-JavaThemis ☕ Wrapper: Java, Java and Kotlin API label Nov 4, 2019
@ilammy
Copy link
Collaborator Author

ilammy commented Nov 4, 2019

Also, export some private functions to make dynamic linkage possible. These are currently used by Themis JNI wrapper in order to operate secure_session_load() correctly. Strictly speaking, we should not be using these functions, but currently there is no way to allocate Secure Session instance suitable for secure_session_load() without using private header <themis/secure_session_t.h>. These functions are exported from the shared library, but we make no commitment to keeping them exported. They should be hidden back once secure_session_load() gets a better API.

@vixentael
Copy link
Contributor

Changes LGTM, but need to test that building process works on different machines.

On the other hand, why not? It's not that hard to implement and
this *is* a wart in the API. Let's mark these private functions
as such so that there is no confusion.
@Lagovas
Copy link
Collaborator

Lagovas commented Nov 8, 2019

Strictly speaking, we should not be using these functions, but currently there is no way to allocate Secure Session instance suitable for secure_session_load() without using private header <themis/secure_session_t.h>.

can you explain why we need to export secure_session_<init|cleanup> to use secure_session_load? I don't see where these functions used in load/save implementations: https://github.com/cossacklabs/themis/blob/b9062e512aa39a284fa4177a6dc4ca9a34f72c83/src/themis/secure_session_serialize.c

@ilammy
Copy link
Collaborator Author

ilammy commented Nov 8, 2019

can you explain why we need to export secure_session_<init|cleanup> to use secure_session_load?

Sure.

They need to be exported because JNI library uses them here:

themis/jni/themis_session.c

Lines 533 to 538 in 0d97f5b

themis_status = secure_session_init(&(ctx->session),
id_buf,
id_length,
sign_key_buf,
sign_key_length,
&(ctx->callbacks));

and here:

themis/jni/themis_session.c

Lines 575 to 576 in 0d97f5b

secure_session_cleanup(&(ctx->session));
(*env)->SetLongField(env, thiz, session_field_id, 0);

Note that JNI library does not use secure_session_create() and secure_session_destroy(), and it stores secure_session_t directly, not a pointer to it like any other wrapper:

struct session_with_callbacks_type {
secure_session_t session;
secure_session_user_callbacks_t callbacks;
JNIEnv* env;
jobject thiz;
};

All of this is because secure_session_load() accepts a pointer to an allocated instance of secure_session_t. We need to allocate enough memory for Secure Session object in order to use that function, and currently the only way to do this is to include a private header <themis/secure_session_t.h> which defines the layout of secure_session_t. Allocating secure_session_t in that way requires secure_session_init() and secure_session_cleanup() to be used instead of secure_session_create() and secure_session_destroy().

This is problematic because:

  • we need to export functions that are considered private API
  • we need to expose implementation details of secure_session_t
  • we cannot change layout of secure_session_t in any way
  • we cannot compile JNI library and Themis Core separately

This PR exports the functions as an easy way out and maintains status quo. A proper solution would involve hiding the implementation details and reengineering secure_session_load() so that it does not depend on those details to be known by the users.

Copy link
Contributor

@shadinua shadinua left a comment

Choose a reason for hiding this comment

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

LGTM

@Lagovas
Copy link
Collaborator

Lagovas commented Nov 8, 2019

discussed with @ilammy that will be better if we will add a new function which will create secure session from the dumped buffer and return a pointer. after that, we will update jni to use this new function without usage of private api secure session to be consistent with other public api and deprecate old function.

@ilammy ilammy merged commit b8a16c5 into cossacklabs:master Nov 8, 2019
@ilammy ilammy deleted the jni-dynamic branch November 8, 2019 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
W-JavaThemis ☕ Wrapper: Java, Java and Kotlin API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants