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

Rework encryption / openssl offloading #97

Merged
merged 17 commits into from Apr 5, 2016

Conversation

Projects
None yet
6 participants
@champtar
Contributor

champtar commented Mar 3, 2016

this need a rebuild of jnopenssl

@lyubomir lyubomir self-assigned this Mar 3, 2016

@jitsi-jenkins

This comment has been minimized.

Show comment
Hide comment
@jitsi-jenkins

jitsi-jenkins Mar 3, 2016

Can one of the admins verify this patch?

jitsi-jenkins commented Mar 3, 2016

Can one of the admins verify this patch?

@champtar

This comment has been minimized.

Show comment
Hide comment
@champtar

champtar Mar 4, 2016

Contributor

just added unit tests for F8 and CTR, you can review

Contributor

champtar commented Mar 4, 2016

just added unit tests for F8 and CTR, you can review

@lyubomir lyubomir removed their assignment Mar 4, 2016

@ibauersachs ibauersachs self-assigned this Mar 6, 2016

@ibauersachs

This comment has been minimized.

Show comment
Hide comment
@ibauersachs

ibauersachs Mar 6, 2016

Member

I need some more time to review this; in the meantime, could you please fix the formatting (e.g. curly braces on new lines)? Thanks.

Member

ibauersachs commented Mar 6, 2016

I need some more time to review this; in the meantime, could you please fix the formatting (e.g. curly braces on new lines)? Thanks.

champtar added some commits Mar 7, 2016

Don't export HMAC_CTX_cleanup() in jnopenssl lib
the only place where it make sense to use it is in HMAC_CTX_destroy()

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Improve SRTPCipherF8.processBlock a bit
in and out are the same

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Remove OpenSSLDigest
If OpenSSLHMAC doesn't work (JNI lib loading failure),
OpenSSLDigest will not work, so the hybrid fallback
(Bouncy Castle HMAc + OpenSSLDigest sha1) is useless

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Rework SRTPCipherF8 to hide the BlockCipher into it
Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Fix HMAC.c include
Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Fix OpenSSLHMAC with recent libcrypto
With recent (1.0.2 at least) libcrypto,
HMAC_Init_ex() call with null key and md on a new ctx is rejected

Caching the result of EVP_sha1() in a static variable is useless

Call to HMAC_CTX_cleanup() is useless and could have been harmfull
(OpenSSL docs says "HMAC_CTX_cleanup() erases the key and other data
from the HMAC_CTX and releases any associated resources.
It must be called when an HMAC_CTX is no longer required")

reset() must be called after init() now (call HMAC_Init_ex() only with the ctx)

We are now doing 4 JNI calls instead of 5 per HMAC:
HMAC_Update
HMAC_Update
HMAC_Final
HMAC_CTX_cleanup (removed)
HMAC_Init_ex

we could also merge HMAC_Final and HMAC_Init_ex in one JNI call

Run tested on Fedora 23 (libcrypto 1.0.2), Jitsi Meet still works
(and JVB is using OpenSSLHMAC)

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Add arguments checks to SRTPCipherF8
Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Rework SRTPCipherCTR to hide CipherBlock and getCipherStream
The SRTPCipherCTR interface is now greatly simplified, with just
constructor / init / process

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
@champtar

This comment has been minimized.

Show comment
Hide comment
@champtar

champtar Mar 7, 2016

Contributor

I think i've fixed all formating issues
I've added arguments checks (throwing IllegalArgumentException)

Contributor

champtar commented Mar 7, 2016

I think i've fixed all formating issues
I've added arguments checks (throwing IllegalArgumentException)

*/
f8Cipher.processBlock(iv, 0, f8ctx.ivAccent, 0);
cipher.init(true, new KeyParameter(encKey));

This comment has been minimized.

@ibauersachs

ibauersachs Mar 15, 2016

Member

Good catch!

@ibauersachs

ibauersachs Mar 15, 2016

Member

Good catch!

This comment has been minimized.

@champtar

champtar Mar 16, 2016

Contributor

using 1 cipher instead of 2 ?

@champtar

champtar Mar 16, 2016

Contributor

using 1 cipher instead of 2 ?

This comment has been minimized.

@ibauersachs

ibauersachs Mar 16, 2016

Member

Ah, now I see it. Was too late yesterday :-)

@ibauersachs

ibauersachs Mar 16, 2016

Member

Ah, now I see it. Was too late yesterday :-)

Show outdated Hide outdated src/org/jitsi/impl/neomedia/transform/srtp/SRTPCipherCTR.java
* @param data
* @param off
* @param len
* @param iv initial value of the counter (this value is modified)
*/

This comment has been minimized.

@ibauersachs

ibauersachs Mar 15, 2016

Member

Please document the parameters and include the reference to the RFC. Say something about the cipher stream being computed and used directly.

@ibauersachs

ibauersachs Mar 15, 2016

Member

Please document the parameters and include the reference to the RFC. Say something about the cipher stream being computed and used directly.

This comment has been minimized.

@ibauersachs

ibauersachs Mar 15, 2016

Member

Github says the diff is outdated, true, but: SRTPCipherCTRJava's class doc would be more appropriate on the (new) base class. Then say something about what SRTPCipherCTRJava does different/specialized from its base.

@ibauersachs

ibauersachs Mar 15, 2016

Member

Github says the diff is outdated, true, but: SRTPCipherCTRJava's class doc would be more appropriate on the (new) base class. Then say something about what SRTPCipherCTRJava does different/specialized from its base.

This comment has been minimized.

@champtar

champtar Mar 16, 2016

Contributor

that's what i was doing already (@inheritDoc)
I've updated the class JavaDoc

@champtar

champtar Mar 16, 2016

Contributor

that's what i was doing already (@inheritDoc)
I've updated the class JavaDoc

@@ -0,0 +1,92 @@
#include "SRTPCipherCTROpenSSL.h"

This comment has been minimized.

@ibauersachs

ibauersachs Mar 15, 2016

Member

The copyright header must be added here too.

@ibauersachs

ibauersachs Mar 15, 2016

Member

The copyright header must be added here too.

This comment has been minimized.

@champtar

champtar Mar 16, 2016

Contributor

done

@champtar

champtar Mar 16, 2016

Contributor

done

@ibauersachs

This comment has been minimized.

Show comment
Hide comment
@ibauersachs

ibauersachs Mar 15, 2016

Member

There's a few cosmetics left with Javadoc and copyrights, but otherwise this looks okay to me. There might be an incompatibility with OpenSSL < 1.0.1, but Ubuntu has 1.0.1something since at least 12.04 so that I wouldn't worry about it.

Member

ibauersachs commented Mar 15, 2016

There's a few cosmetics left with Javadoc and copyrights, but otherwise this looks okay to me. There might be an incompatibility with OpenSSL < 1.0.1, but Ubuntu has 1.0.1something since at least 12.04 so that I wouldn't worry about it.

champtar added some commits Mar 7, 2016

Rewite SRTPCipherCTR.process
This version is as fast as old implementation but uses less temp buffers
(around 2600 ns per 1000 bytes)

Version below is simpler but a bit slower (around 3200 ns)

public void process(byte[] data, int off, int len, byte[] iv)
{
    int end = offset + len;
    if (end > inOut.length)
        return;

    for (int off = offset; off < end; off += BLKLEN)
    {
        ciphertest.doFinal(iv, 0, BLKLEN, tmpCipherBlock);
        if(++iv[15] == 0) ++iv[14];
        for (int i = 0, j = off; i < BLKLEN && j < end; i++, j++)
            inOut[j] ^= tmpCipherBlock[i];
    }
}

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Add arguments checks to SRTPCipherCTR
Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Make SRTPCipherCTR an abstract class and
SRTPCipherCTRJava the first implementation

factor process() arguments checks into checkProcessArgs

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Update SRTPCipherCTRJava JavaDoc
SRTPCipherCTRJava is not specific AES,
and it's standard block cipher counter mode

Also remove @author mentions as authorship is in git,
and i've rewritten most of the code

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Introduce OpenSSLWrapperLoader and use it
OpenSSLWrapperLoader load the OpenSSL JNI lib wrapper
and initialise libcrypto
(OpenSSL_add_all_algorithms()/ERR_load_crypto_strings())

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Drop OpenSSLBlockCipher, we now have SRTPCipherCTROpenSSL
doing a JNI call to process 16 bytes at a time is inefficient
and slower than recent JCE

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Add unit tests for SRTPCipherF8
Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Add unit tests for SRTPCipherCTR
Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Add SRTPCipherCTROpenSSL, use it by default if available
Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
@champtar

This comment has been minimized.

Show comment
Hide comment
@champtar

champtar Mar 16, 2016

Contributor

we can support OpenSSL 1.0.0 if needed (EVP_aes_128_ctr is not there but the CTR impl is there if i remember right)
Don't forget to rebuild libjnopenssl if you merge

Contributor

champtar commented Mar 16, 2016

we can support OpenSSL 1.0.0 if needed (EVP_aes_128_ctr is not there but the CTR impl is there if i remember right)
Don't forget to rebuild libjnopenssl if you merge

@ibauersachs

This comment has been minimized.

Show comment
Hide comment
@ibauersachs

ibauersachs Mar 16, 2016

Member

I don't think OpenSSL 1.0.0 is needed. @lyubomir: can you merge and then build the JNI lib? I don't have access to an old Debian.

Member

ibauersachs commented Mar 16, 2016

I don't think OpenSSL 1.0.0 is needed. @lyubomir: can you merge and then build the JNI lib? I don't have access to an old Debian.

@lyubomir

This comment has been minimized.

Show comment
Hide comment
@lyubomir

lyubomir Mar 16, 2016

Member

@ibauersachs, thank you very much! I'll try to build the JNI lib.

Member

lyubomir commented Mar 16, 2016

@ibauersachs, thank you very much! I'll try to build the JNI lib.

@champtar

This comment has been minimized.

Show comment
Hide comment
@champtar

champtar Mar 23, 2016

Contributor

any updates?

Contributor

champtar commented Mar 23, 2016

any updates?

@lyubomir

This comment has been minimized.

Show comment
Hide comment
@lyubomir

lyubomir Mar 23, 2016

Member

@champtar, I have this in my TODO list. I'm afraid I'm very busy at work with a delivery due at the end of the month though.

Member

lyubomir commented Mar 23, 2016

@champtar, I have this in my TODO list. I'm afraid I'm very busy at work with a delivery due at the end of the month though.

@emcho

This comment has been minimized.

Show comment
Hide comment
@emcho

emcho Mar 23, 2016

Member

just to be clear: we are just waiting for someone to build the JNI? @damencho could you please do this?

Member

emcho commented Mar 23, 2016

just to be clear: we are just waiting for someone to build the JNI? @damencho could you please do this?

@champtar

This comment has been minimized.

Show comment
Hide comment
@champtar

champtar Mar 23, 2016

Contributor

for the guy that is going to rebuild the JNI, please don't forget to document everything (commands and soft version)

Contributor

champtar commented Mar 23, 2016

for the guy that is going to rebuild the JNI, please don't forget to document everything (commands and soft version)

@damencho

This comment has been minimized.

Show comment
Hide comment
@damencho

damencho Mar 23, 2016

Member

Will do.

Member

damencho commented Mar 23, 2016

Will do.

@ibauersachs ibauersachs assigned damencho and unassigned ibauersachs Apr 4, 2016

damencho added a commit that referenced this pull request Apr 4, 2016

@lyubomir

This comment has been minimized.

Show comment
Hide comment
@lyubomir

lyubomir Apr 4, 2016

Member

@damencho merged this PR into #127 (a branch of jitsi/libjitsi) along with build instructions and Linux shared object binaries so that (1) automatic tests may be run on it and (2) I can benchmark it and possibly create a before-and-after video and/or flame graphs. We'll close this PR afterwards.

Member

lyubomir commented Apr 4, 2016

@damencho merged this PR into #127 (a branch of jitsi/libjitsi) along with build instructions and Linux shared object binaries so that (1) automatic tests may be run on it and (2) I can benchmark it and possibly create a before-and-after video and/or flame graphs. We'll close this PR afterwards.

@ibauersachs ibauersachs merged commit 72a2e7d into jitsi:master Apr 5, 2016

1 check passed

default Build finished.
Details

@champtar champtar referenced this pull request Apr 5, 2016

Closed

AES Bench #80

@champtar champtar deleted the champtar:openssl2 branch Apr 5, 2016

champtar added a commit to champtar/libjitsi that referenced this pull request Apr 8, 2016

Remove reference to NIOBlockCipher in CryptoBenchmark
NIOBlockCipher as been removed in PR #97
commit 18c9516

This was missed because CryptoBenchmark.java was excluded
in maven compilation, so don't exclude it

This fixes #132 (Broken Ant build)

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>

champtar added a commit to champtar/libjitsi that referenced this pull request Apr 8, 2016

Make more accurate AES benchmark
packet size is around 1000 bytes and internet MTU is 1500 bytes
so use 1024bytes instead of 16384bytes

Timing only one encryption gives us "unstable" results,
so do it 10000 times, and do the benchmark less often (10 min)

On my computer (core i7) this gives (0,3s):
BouncyCastle 83394779, OpenSSL 70399746, SunJCE 38232257, SunPKCS11 134865361
Will employ AES implemented by SunJCE

Since #97 we now have SRTPCipherCTROpenSSL to replace OpenSSLBlockCipher

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment