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

Replace encryption algorithm #191

Merged
merged 5 commits into from
Oct 21, 2019
Merged

Replace encryption algorithm #191

merged 5 commits into from
Oct 21, 2019

Conversation

driftluo
Copy link
Collaborator

@driftluo driftluo commented Oct 15, 2019

relate #190

Due to the basic requirements of the industry standard for encryption algorithms, the replacement algorithm is libressl and boringssl bind.

  1. Remove support for twofish-ctr
  2. Added support for aes-gcm/chacha20poly1305
  3. Various hash and symmetric encryption algorithms are replaced with industry standard implementations
  4. Compatible with 0.1 in protocol layer save for aes-ctr communication
  5. The default symmetric encryption algorithm for 0.2 is aes-gcm

Note that aes-ctr is not the same encryption algorithm as aes-gcm/chacha20poly1305:

  • aes-ctr is just encryption, it needs hmac assistance for integrity verification
  • aes-gcm/chacha20poly1305 belongs to the aead algorithm, which itself has the function of encryption and integrity check, so it does not need the assistance of hmac, which is the only symmetric encryption algorithm supported by TLS 1.3.

The good news is that we have a side effect that has greatly improved performance. :P

On my machine:

$ cat /proc/cpuinfo | grep name | cut -f2 -d: | uniq -c
      8  Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz

before:

1kb_aes128              time:   [9.4003 ms 9.4581 ms 9.5252 ms]                        
                        change: [+4.0887% +5.0088% +5.9171%] (p = 0.00 < 0.05)
                        Performance has regressed.
1kb_aes256              time:   [11.567 ms 11.638 ms 11.712 ms]                        
                        change: [+0.8615% +1.9225% +2.8798%] (p = 0.00 < 0.05)
                        Change within noise threshold.
1mb_aes128              time:   [36.289 ms 36.395 ms 36.521 ms]                        
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild
1mb_aes256              time:   [46.033 ms 46.292 ms 46.606 ms]                        
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

after:

1kb_aes128ctr           time:   [1.5785 ms 1.5866 ms 1.5951 ms]                           
                        change: [-5.1104% -3.6428% -2.2569%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild
  2 (2.00%) high severe


1kb_aes256ctr           time:   [1.6384 ms 1.6554 ms 1.6756 ms]                           
                        change: [-2.2981% -0.6891% +1.2321%] (p = 0.45 > 0.05)
                        No change in performance detected.
Found 8 outliers among 100 measurements (8.00%)
  6 (6.00%) high mild
  2 (2.00%) high severe

1kb_aes128gcm           time:   [161.01 us 162.33 us 163.74 us]                          
                        change: [-2.5307% -1.8407% -1.1259%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

1kb_aes256gcm           time:   [201.21 us 202.64 us 204.26 us]                          
                        change: [-4.9827% -4.1794% -3.2904%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild

1kb_chacha20poly1305    time:   [335.52 us 337.61 us 339.91 us]                                 
                        change: [-1.4502% -0.6889% +0.0854%] (p = 0.08 > 0.05)
                        No change in performance detected.


1mb_aes128ctr           time:   [6.7473 ms 6.8568 ms 6.9673 ms]                           
                        change: [-3.3367% -2.0388% -0.6587%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild


1mb_aes256ctr           time:   [6.6972 ms 6.7755 ms 6.8614 ms]                           
                        change: [+0.4047% +1.4643% +2.5987%] (p = 0.01 < 0.05)
                        Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe

1mb_aes128gcm           time:   [654.97 us 658.75 us 662.99 us]                          
                        change: [-2.9265% -1.2787% +0.3783%] (p = 0.14 > 0.05)
                        No change in performance detected.
Found 8 outliers among 100 measurements (8.00%)
  2 (2.00%) high mild
  6 (6.00%) high severe

1mb_aes256gcm           time:   [835.03 us 841.81 us 850.27 us]                          
                        change: [-6.3051% -4.3540% -2.1946%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 14 outliers among 100 measurements (14.00%)
  8 (8.00%) high mild
  6 (6.00%) high severe


1mb_chacha20poly1305    time:   [1.3606 ms 1.3663 ms 1.3724 ms]                                  
                        change: [-1.6412% -0.6333% +0.4348%] (p = 0.23 > 0.05)
                        No change in performance detected.
Found 9 outliers among 100 measurements (9.00%)
  6 (6.00%) high mild
  3 (3.00%) high severe

The corresponding communication overhead has also dropped significantly(just in ci):

before:

task name: 10kb_benchmark_with_secio
cycles: 100
total cost: 3.133062369s
average: 31.330623ms
median: 43.93612ms
max: 46.227071ms
min: 527.707µs


task name: 10kb_benchmark_with_no_secio
cycles: 100
total cost: 2.815652608s
average: 28.156526ms
median: 43.912089ms
max: 44.124585ms
min: 72.834µs


task name: 10mb_benchmark_with_secio
cycles: 100
total cost: 50.080729558s
average: 500.807295ms
median: 500.415319ms
max: 529.83246ms
min: 487.35576ms


task name: 10mb_benchmark_with_no_secio
cycles: 100
total cost: 4.032045486s
average: 40.320454ms
median: 40.320799ms
max: 82.848557ms
min: 32.164175ms

after:

task name: 10kb_benchmark_with_secio
cycles: 100
total cost: 3.077164039s
average: 30.77164ms
median: 43.956092ms
max: 44.152848ms
min: 112.846µs


task name: 10kb_benchmark_with_no_secio
cycles: 100
total cost: 2.815918338s
average: 28.159183ms
median: 43.939645ms
max: 44.150048ms
min: 95.508µs


task name: 10mb_benchmark_with_secio
cycles: 100
total cost: 5.868204174s
average: 58.682041ms
median: 58.404746ms
max: 76.677994ms
min: 50.307476ms


task name: 10mb_benchmark_with_no_secio
cycles: 100
total cost: 4.310246341s
average: 43.102463ms
median: 43.411996ms
max: 49.381531ms
min: 33.707408ms

sorry,after testing, aes-ctr and openssl are compatible when nonce is in a certain range, but the nonce large probability generated during secio handshake is not in this range, so 0.1 and 0.2 are not compatible with high probability. Even so, I still insist on using openssl as the standard and think that the previous implementation is a bug.

@driftluo driftluo requested a review from a team October 15, 2019 03:08
@driftluo driftluo force-pushed the replace-encryption-algorithm branch 3 times, most recently from 4fb1c95 to 0f297ea Compare October 15, 2019 09:19
@doitian doitian added this to 👀 Awaiting review in CKB - Pull Requests Oct 16, 2019
@driftluo driftluo force-pushed the replace-encryption-algorithm branch 2 times, most recently from cfeefd1 to d7fe3bc Compare October 16, 2019 02:38
@driftluo
Copy link
Collaborator Author

Add a contrasting implementation of openssl and ring, and use the openssl implementation by default with native support

After testing, openssl implementation will be 5-20% faster than ring on my machine.

@doitian doitian moved this from 👀 Awaiting review to 🔴 High priority in CKB - Pull Requests Oct 18, 2019
@doitian doitian added this to the v0.24.0 milestone Oct 18, 2019
secio/src/crypto/mod.rs Outdated Show resolved Hide resolved
@driftluo
Copy link
Collaborator Author

driftluo commented Oct 21, 2019

@quake @TheWaWaR update, please review the latest commit

CKB - Pull Requests automation moved this from 🔴 High priority to ✅ Reviewer approved Oct 21, 2019
@driftluo driftluo merged commit bc9b8a2 into master Oct 21, 2019
CKB - Pull Requests automation moved this from ✅ Reviewer approved to Done Oct 21, 2019
@driftluo driftluo deleted the replace-encryption-algorithm branch October 21, 2019 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants