filter: exposed functions to Lua to verify digital signature by yxue · Pull Request #7050 · envoyproxy/envoy

yxue · 2019-05-23T01:36:15Z

Description: Expose functions for users to verify digital signature in Lua script

Risk Level: Low

Testing: unit test, integration

Docs Changes: https://github.com/envoyproxy/envoy/compare/master...crazyxy:feature/boringssllua?expand=1#diff-ddc999700fdafb43c2cdcafcc7a4b887

Release Notes: https://github.com/envoyproxy/envoy/compare/master...crazyxy:feature/boringssllua?expand=1#diff-d8d3e33358b55e6c0466dd1bb5f40c79

Fixes #Issue: #7009

dio

Looks good to me, some small styling comments. Summon @lizan to review on the code SSL related codes.

source/extensions/filters/http/lua/lua_filter.cc

source/extensions/filters/http/lua/lua_filter.h

source/extensions/filters/http/lua/lua_filter.cc

htuch · 2019-05-28T15:39:48Z

Please merge master to pick up #7090

yxue · 2019-05-29T22:18:30Z

@lizan Gentle ping. PTAL

repokitteh-read-only · 2019-05-29T22:18:34Z

crazyxy is not allowed to assign users.

🐱

Caused by: a #7050 (comment) was created by @crazyxy.

see: more, trace.

lizan

Generally LGTM though I'm a bit worried about this pulling in BoringSSL as a hard dependency of Lua filter, I think @bdecoste is working to make it possible to compile out BoringSSL completely. cc @bdecoste

source/extensions/filters/http/lua/lua_filter.cc

htuch · 2019-05-30T22:19:49Z

@crazyxy I pinged you internally, would like to understand the motivation for this PR a bit. Thanks.

htuch

Thanks @crazyxy. This looks good in principle to me; I do worry about the proliferation of LuaJIT bindings to random external dependencies, but on a concrete level this seems to be well motivated. Just a question on whether we can decouple the BoringSSL nitty gritty (and make it more testable) by splitting it out of the Lua bindings.
/wait

docs/root/configuration/http_filters/lua_filter.rst

source/extensions/filters/http/lua/lua_filter.cc

source/extensions/filters/http/lua/lua_filter.h

yxue · 2019-06-04T03:19:17Z

Thanks @crazyxy. This looks good in principle to me; I do worry about the proliferation of LuaJIT bindings to random external dependencies, but on a concrete level this seems to be well motivated. Just a question on whether we can decouple the BoringSSL nitty gritty (and make it more testable) by splitting it out of the Lua bindings.
/wait

@htuch Thanks for reviewing! Made the change as requested.

docs/root/configuration/http_filters/lua_filter.rst

source/common/crypto/utility.cc

docs/root/configuration/http_filters/lua_filter.rst

source/extensions/filters/http/lua/lua_filter.h

dio

Put more comments on preference using snake_case for local vars.

docs/root/configuration/http_filters/lua_filter.rst

source/common/crypto/utility.cc

source/extensions/filters/http/lua/lua_filter.cc

test/common/crypto/utility_test.cc

test/extensions/filters/http/lua/lua_filter_test.cc

test/extensions/filters/http/lua/lua_integration_test.cc

Signed-off-by: Yan Xue <yxyan@google.com>

yxue · 2019-06-05T00:57:36Z

@htuch @lizan PTAL. Thanks!

lizan

Still not convinced by the structure that import/release methods lives in StreamHandlerWrapper.

Also do not rebase + force push into existing PR b/c it messes up review history, git merge + regular push is preferred.

source/extensions/filters/http/lua/lua_filter.cc

Signed-off-by: Yan Xue <yxyan@google.com>

lizan

Looks much better, a few more comments. Thanks!

source/common/crypto/utility.cc

source/extensions/filters/http/lua/wrappers.h

test/common/crypto/utility_test.cc

Signed-off-by: Yan Xue <yxyan@google.com>

yxue · 2019-06-05T06:57:39Z

Looks much better, a few more comments. Thanks!

Thanks @lizan! Removed the pointer.

yxue · 2019-06-05T16:11:37Z

/retest

repokitteh-read-only · 2019-06-05T16:11:42Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #7050 (comment) was created by @crazyxy.

see: more, trace.

htuch

Nice, a few more general comments.
/wait

docs/root/intro/version_history.rst

source/common/crypto/utility.cc

source/common/crypto/utility.h

Signed-off-by: Yan Xue <yxyan@google.com>

source/common/crypto/utility.h

docs/root/intro/version_history.rst

Signed-off-by: Yan Xue <yxyan@google.com>

htuch

Thanks!

yxue · 2019-06-05T22:57:42Z

@dio Could you please take a look? Thanks!

dio

Cool. This is awesome. Thanks!

…oxy#7050) Description: Expose functions for users to verify digital signature in Lua script Risk Level: Low Testing: unit test, integration Docs Changes: https://github.com/envoyproxy/envoy/compare/master...crazyxy:feature/boringssllua?expand=1#diff-ddc999700fdafb43c2cdcafcc7a4b887 Release Notes: https://github.com/envoyproxy/envoy/compare/master...crazyxy:feature/boringssllua?expand=1#diff-d8d3e33358b55e6c0466dd1bb5f40c79 Fixes #Issue: envoyproxy#7009 Signed-off-by: Yan Xue <yxyan@google.com>

knrc · 2019-06-07T19:27:08Z

@bdecoste FYI this seems to be the commit which reintroduced boringssl into core

filter: exposed functions to Lua to verify digital signature (envoyproxy#7050)

cherry-pick into 1.2: filter: exposed functions to Lua to verify digital signature (envoyproxy#7050)

yxue mentioned this pull request May 23, 2019

Make BoringSSL available to Lua FFI istio/istio#13818

Closed

mattklein123 assigned dio May 23, 2019

dio suggested changes May 27, 2019

View reviewed changes

source/extensions/filters/http/lua/lua_filter.cc Outdated Show resolved Hide resolved

source/extensions/filters/http/lua/lua_filter.h Outdated Show resolved Hide resolved

source/extensions/filters/http/lua/lua_filter.cc Outdated Show resolved Hide resolved

yxue force-pushed the feature/boringssllua branch 2 times, most recently from 9f74306 to 4f2fdc1 Compare May 28, 2019 16:35

dio assigned lizan May 30, 2019

lizan suggested changes May 30, 2019

View reviewed changes

source/extensions/filters/http/lua/lua_filter.cc Outdated Show resolved Hide resolved

source/extensions/filters/http/lua/lua_filter.cc Outdated Show resolved Hide resolved

yxue force-pushed the feature/boringssllua branch 3 times, most recently from 0ffecb6 to 75b515b Compare May 31, 2019 16:15

htuch reviewed Jun 3, 2019

View reviewed changes

docs/root/configuration/http_filters/lua_filter.rst Outdated Show resolved Hide resolved

source/extensions/filters/http/lua/lua_filter.cc Outdated Show resolved Hide resolved

source/extensions/filters/http/lua/lua_filter.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jun 3, 2019

htuch reviewed Jun 3, 2019

View reviewed changes

source/extensions/filters/http/lua/lua_filter.h Outdated Show resolved Hide resolved

repokitteh-read-only bot removed the waiting label Jun 4, 2019

PiotrSikora reviewed Jun 4, 2019

View reviewed changes

dio suggested changes Jun 4, 2019

View reviewed changes

yxue force-pushed the feature/boringssllua branch 2 times, most recently from 61930c4 to 4be7d64 Compare June 4, 2019 22:26

yxue added 7 commits June 4, 2019 15:32

Expose functions for verifying digital signature

3bedd5e

Signed-off-by: Yan Xue <yxyan@google.com>

Add release note and document

05854ec

Signed-off-by: Yan Xue <yxyan@google.com>

refactor

dd3aa17

Signed-off-by: Yan Xue <yxyan@google.com>

Revert unnecessary change

9de8afe

Signed-off-by: Yan Xue <yxyan@google.com>

CR requested change

85cec32

Signed-off-by: Yan Xue <yxyan@google.com>

Change comment

edeaae9

Signed-off-by: Yan Xue <yxyan@google.com>

Replace auto with type

8d33bd5

Signed-off-by: Yan Xue <yxyan@google.com>

lizan suggested changes Jun 5, 2019

View reviewed changes

source/extensions/filters/http/lua/lua_filter.cc Outdated Show resolved Hide resolved

Add EVP_PKEY wrapper

14fed22

Signed-off-by: Yan Xue <yxyan@google.com>

lizan suggested changes Jun 5, 2019

View reviewed changes

source/common/crypto/utility.cc Outdated Show resolved Hide resolved

source/extensions/filters/http/lua/wrappers.h Outdated Show resolved Hide resolved

test/common/crypto/utility_test.cc Outdated Show resolved Hide resolved

Use unique_ptr

425f09e

Signed-off-by: Yan Xue <yxyan@google.com>

lizan previously approved these changes Jun 5, 2019

View reviewed changes

htuch suggested changes Jun 5, 2019

View reviewed changes

docs/root/intro/version_history.rst Outdated Show resolved Hide resolved

source/common/crypto/utility.cc Outdated Show resolved Hide resolved

source/common/crypto/utility.h Show resolved Hide resolved

source/common/crypto/utility.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jun 5, 2019

Resolve some comments

9d24d6d

Signed-off-by: Yan Xue <yxyan@google.com>

yxue dismissed lizan’s stale review via 9d24d6d June 5, 2019 18:59

repokitteh-read-only bot removed the waiting label Jun 5, 2019

htuch reviewed Jun 5, 2019

View reviewed changes

source/common/crypto/utility.h Outdated Show resolved Hide resolved

htuch reviewed Jun 5, 2019

View reviewed changes

docs/root/intro/version_history.rst Outdated Show resolved Hide resolved

More comments

26349b9

Signed-off-by: Yan Xue <yxyan@google.com>

htuch approved these changes Jun 5, 2019

View reviewed changes

dio approved these changes Jun 6, 2019

View reviewed changes

lizan merged commit 9749194 into envoyproxy:master Jun 6, 2019

lizan mentioned this pull request Jun 6, 2019

Make BoringSSL available to Lua FFI #7009

Closed

duderino pushed a commit to istio/envoy that referenced this pull request Jun 12, 2019

Merge pull request #79 from crazyxy/istio-release-1.1

8e54972

filter: exposed functions to Lua to verify digital signature (envoyproxy#7050)

duderino pushed a commit to istio/envoy that referenced this pull request Jun 12, 2019

Merge pull request #82 from duderino/jblatt_1_2_lua_symbols

4f5b5e1

cherry-pick into 1.2: filter: exposed functions to Lua to verify digital signature (envoyproxy#7050)

duderino pushed a commit to istio/proxy that referenced this pull request Jun 13, 2019

Update istio/envoy to pick up envoyproxy/envoy#7050 (#2274)

d26534e

yxue deleted the feature/boringssllua branch June 13, 2019 23:49

Conversation

yxue commented May 23, 2019

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch commented May 28, 2019

Uh oh!

yxue commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented May 29, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch commented May 30, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yxue commented Jun 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yxue commented Jun 5, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yxue commented Jun 5, 2019

Uh oh!

yxue commented Jun 5, 2019

Uh oh!

repokitteh-read-only bot commented Jun 5, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

yxue commented May 29, 2019 •

edited

Loading