Added CryptoMB private key provider. by ipuustin · Pull Request #17124 · envoyproxy/envoy

ipuustin · 2021-06-24T07:16:14Z

Commit Message:

Intel's IPP (Integrated Performance Primitives) crypto library has support for multi-buffer crypto operations (see https://github.com/intel/ipp-crypto/tree/develop/sources/ippcp/crypto_mb). Briefly, multi-buffer
cryptography is implemented with AVX-512 IFMA instructions using a SIMD (single instruction, multiple data) mechanism. Up to eight RSA or ECDSA operations are gathered together into a buffer and processed at the same time, providing potentially improved performance. The AVX-512 instructions are available on recently launched 3rd generation Xeon Scalable server processors (Ice Lake server) processors.

This commit adds a private key provider to accelerate RSA and ECDSA crypto operations on recent Intel Xeon processors. Every worker thread has a queue of up-to-eight crypto operations. When the queue is full or when the timer is triggered, the queue is processed and all the pending handshakes are notified.

The potential performance benefit depends on many factors: the size of the cpuset Envoy is running on, incoming traffic pattern, encryption type (RSA or ECDSA), and key size. In my own testing I saw the biggest performance increase when long RSA keys were used on an Envoy running in a fairly limited environment serving lots of new incoming TLS requests.

Additional Description:

One new dependency is introduced: Intel’s ipp-crypto library. The library has to be patched at the moment in order to make it compile against BoringSSL and Bazel build system. The ipp-crypto team has indicated that they will accept the patches and release them in future ipp-crypto versions.

Basic end-to-end tests are provided, and a fake library interface is included for testing on systems without the required AVX-512 instruction set.

Risk Level: Medium (TLS security feature, not enabled by default)
Testing: unit tests, integration tests
Docs Changes: API interface is documented
Release Notes: N/A
Platform Specific Features: Requires Intel 3rd generation Xeon Scalable server processor for the AVX-512 IFMA instruction set.
Fixes: #15871

Intel's IPP (Integrated Performance Primitives) crypto library has support for multi-buffer crypto operations. Briefly, multi-buffer cryptography is implemented with AVX-512 instructions using a SIMD (single instruction, multiple data) mechanism. Up to eight RSA or ECDSA operations are gathered together into a buffer and processed at the same time, providing potentially improved performance. The AVX-512 instructions are available on recently launched 3rd generation Xeon Scalable server processors (Ice Lake server) processors. This commit adds a private key provider to accelerate RSA and ECDSA crypto operations on recent Intel Xeon processors. Every worker thread has a queue of up-to-eight crypto operations. When the queue is full or when the timer is triggered, the queue is processed and all the pending handshakes are notified. The potential performance benefit depends on many factors: the size of the cpuset Envoy is running on, incoming traffic pattern, encryption type (RSA or ECDSA), and key size. In my own testing I saw the biggest performance increase when long RSA keys were used on an Envoy running in a fairly limited environment serving lots of new incoming TLS requests. One new dependency is introduced: Intel’s ipp-crypto library. The library has to be patched at the moment in order to make it compile against BoringSSL and Bazel build system. The ipp-crypto team has indicated that they will accept the patches and release them in future ipp-crypto versions. Basic end-to-end tests are provided, and a fake library interface is included for testing on systems without the required AVX512 instruction set. Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

repokitteh-read-only · 2021-06-24T07:16:22Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #17124 was opened by ipuustin.

see: more, trace.

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

moderation · 2021-06-24T16:23:42Z

+        project_url = "https://github.com/intel/ipp-crypto",
+        version = "2021.2",
+        sha256 = "d358e2665d100935f036d84eba70724a12b9e3e5b597ba850d79064a42e6ed5d",
+        strip_prefix = "ipp-crypto-ippcp_2021.2",


Remove hardcoded version strip_prefix = "ipp-crypto-ippcp_{version}",

moderation · 2021-06-24T16:24:21Z

+        version = "2021.2",
+        sha256 = "d358e2665d100935f036d84eba70724a12b9e3e5b597ba850d79064a42e6ed5d",
+        strip_prefix = "ipp-crypto-ippcp_2021.2",
+        urls = ["https://github.com/intel/ipp-crypto/archive/ippcp_2021.2.tar.gz"],


Remove hardcoded version urls = ["https://github.com/intel/ipp-crypto/archive/ippcp_{version}.tar.gz"],

moderation · 2021-06-24T17:01:22Z

Results from OSSF Scorecard. Scores need improving based on https://github.com/envoyproxy/envoy/blob/main/DEPENDENCY_POLICY.md#new-external-dependencies


RESULTS
-------
Repo: github.com/intel/ipp-crypto
Active: Fail 10
Automatic-Dependency-Update: Fail 3
Binary-Artifacts: Fail 10
Branch-Protection: Fail 10
CI-Tests: Fail 10
CII-Best-Practices: Fail 10
Code-Review: Pass 10
Contributors: Fail 10
Frozen-Deps: Fail 5
Fuzzing: Fail 10
Packaging: Fail 0
Pull-Requests: Fail 10
SAST: Fail 10
Security-Policy: Pass 10
Signed-Releases: Fail 0
Signed-Tags: Fail 10
Token-Permissions: Pass 10

lizan

api review

lizan · 2021-06-24T17:53:01Z

+  oneof key_type {
+    string private_key_file = 1;
+
+    string inline_private_key = 2;
+  }


use DataSource.

lizan · 2021-06-24T17:54:38Z

@@ -0,0 +1,74 @@
+From 0ec11191170a5298d2b4452a7b313195ca46b71d Mon Sep 17 00:00:00 2001


Any chance these patches can be in ipp-crypto repo directly instead of series of patch ended up here?

I'm quite uncomfortable adding these patches. We are really trying to push back against this, since it makes it very hard to bump in response to CVEs, in particular in security sensitive libraries like this. See further rationale in https://docs.google.com/presentation/d/14uotwhgyo5pb0AXJN_D3a69vchRtAjFRG01jHmdZ-G4/edit#slide=id.ga0aa5b53ee_0_1754.

Can we merge these patches upstream first?

I got commitment from the ipp-crypto team that the patches will be part of their next release. I'll ask if the patches could be made part of their development branch already, so that this PR could depend on that before the release is out.

I'd prefer to not merge this PR until these patches are committed to the upstream dependency, even if that means using a non-release-branch as the dependency temporarily.

Ok, ipp-crypto added the required changes to the development branch, and their plan is to include the patches in 2021.4 release of ipp-crypto. I made the PR point to the development branch and removed the custom patch files.

I did some work to improve the testing coverage, but it may be that the 96% line coverage target is difficult to reach, because tests use a custom software-only implementation of the relevant AVX-512 accelerated functions. See https://storage.googleapis.com/envoy-pr/13aeb26/coverage/source/extensions/private_key_providers/cryptomb/index.html for current coverage situation.

htuch · 2021-06-27T22:19:21Z

+  // is not filled before the delay has expired, the requests already in the
+  // queue are processed, even if the queue is not full. In effect, this value
+  // controls the balance between latency and throughput.
+  uint32 poll_delay = 3;


google.protobuf.Duration? If for some reason this doesn't work here, please name the field poll_delay_us or poll_delay_micros.

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

IPP-crypto supports Windows, could later why it doesn't compile. Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

The idea is that if the poll delay is 0, might as well process the request synchronously. This would not be used in the real world probably, but for testing synchronous processing makes things a lot easier. Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

snowp · 2021-07-14T14:18:24Z

Do we have an assigned sponsor for this extension? As per the extension policy these kind of extensions will need someone shepherding this (especially due to the complexity of this PR) PR

cc @ggreenway @lizan @htuch since ya'll have been somewhat involved in reviewing this already

Since there seems to be some identified delays here around making sure the necessary patches are upstreamed it might be that the contrib/ extension system will have landed by then, at which point we can be a bit more lax about sponsorship requirements

htuch · 2021-07-15T04:06:08Z

I don't have the cycles right now to be able to sponsor this (but super cool extension). @ipuustin what is your thought on a contrib/ vs. core Envoy extension here?

ggreenway · 2021-07-22T21:29:37Z

I think this is a good candidate for contrib, and if it receives significant interest/usage, I may sponsor moving it into core at that time.

htuch · 2021-07-23T01:19:09Z

@ipuustin should we go with contrib/ and close this PR?

ipuustin · 2021-07-27T08:36:32Z

@htuch Sorry for the delay answering this (I was travelling and am still out of office :-). I now read through the contrib proposal and I think it sounds a good match for CryptoMB private key provider. The extension is clearly relevant only to people who have access to supported hardware. I think that people with that hardware and who are also processing large amounts of TLS requests can find the extension in contrib and take it into use.

Do you know what's the timeline for contrib infrastructure work? And will it be possible to enable particular contrib extensions in Envoy builds just from bazel command line, or will it require file changes?

@ggreenway Thanks for considering to sponsor this extension in the future!

lizan

/lgtm api

api lgtm

htuch · 2021-07-29T11:54:20Z

@mattklein123 is working on contrib/ right now, tracked at #14078.

ggreenway · 2021-08-09T15:38:19Z

Waiting for /contrib

/wait

ipuustin · 2021-08-24T12:47:58Z

Let's continue in contrib: #17826

ipuustin requested review from asraa, ggreenway and lizan as code owners June 24, 2021 07:16

repokitteh-read-only Bot added api deps Approval required for changes to Envoy's external dependencies labels Jun 24, 2021

repokitteh-read-only Bot assigned lizan Jun 24, 2021

ipuustin added 4 commits June 24, 2021 00:30

Remove unused validate.proto includes.

8139c05

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Fix pre_check errors.

6d9978f

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Try reformatting Python file differently.

fdcc6f6

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Fix use_category.

113f676

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

moderation suggested changes Jun 24, 2021

View reviewed changes

lizan previously requested changes Jun 24, 2021

View reviewed changes

lizan added the waiting label Jun 25, 2021

htuch reviewed Jun 27, 2021

View reviewed changes

ipuustin added 2 commits June 28, 2021 03:23

Change category to match with the factory.

86b5f46

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Add CryptoMB API to the docs toctree.

22b4ad8

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

repokitteh-read-only Bot removed the waiting label Jun 28, 2021

ipuustin added 10 commits June 28, 2021 07:48

Update proto to DataSource and Duration.

9ba6773

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Fix documentation generation.

12a6e54

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Use version in repository location.

6057e68

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Fix a bunch of clang-tidy errors and a test memory leak.

270d57b

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Fix a memory leak.

4dbeba2

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Disable library build from other platforms than linux_x86.

a7d9b50

IPP-crypto supports Windows, could later why it doesn't compile. Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Add newlines.

f7534e8

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Fix clang-tidy issues.

6e8a6ee

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Move to TLS dispatcher to help with timer memory issue.

a2c7fb7

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

ipuustin added 13 commits July 4, 2021 23:37

Implement ECDSA FIPS check.

e6dafe6

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Fix a BIGNUM memory leak.

eddf925

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Try to skip build on Windows.

458dec1

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Add more tests.

5bb8504

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Timer tests and injecting library errors for tests.

749012e

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Merge remote-tracking branch 'upstream/main' into pr

0a0ff8d

Clang-tidy fixes.

e4bc4ab

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Better coverage for tests.

8e2aaa6

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Remove error debugging to get better test coverage.

1a9721a

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Refactor config tests.

1981410

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Test using RSA exponent 3.

36e3fe6

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

More coverage tests.

65f9ca6

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

Change ipp-crypto to development branch.

13aeb26

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>

lizan reviewed Jul 28, 2021

View reviewed changes

repokitteh-read-only Bot removed the api label Jul 28, 2021

repokitteh-read-only Bot added the waiting label Aug 9, 2021

ipuustin closed this Aug 24, 2021

		@@ -0,0 +1,74 @@
		From 0ec11191170a5298d2b4452a7b313195ca46b71d Mon Sep 17 00:00:00 2001

Conversation

ipuustin commented Jun 24, 2021

Uh oh!

repokitteh-read-only Bot commented Jun 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moderation commented Jun 24, 2021

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp commented Jul 14, 2021

Uh oh!

htuch commented Jul 15, 2021

Uh oh!

ggreenway commented Jul 22, 2021

Uh oh!

htuch commented Jul 23, 2021

Uh oh!

ipuustin commented Jul 27, 2021

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Jul 29, 2021

Uh oh!

ggreenway commented Aug 9, 2021

Uh oh!

ipuustin commented Aug 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants