filter: exposed functions to Lua to verify digital signature #7050

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

source/common/crypto/utility.h

docs/root/intro/version_history.rst

Signed-off-by: Yan Xue <[email protected]>

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 <[email protected]>

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 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 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 <[email protected]>

Add release note and document

05854ec

Signed-off-by: Yan Xue <[email protected]>

refactor

dd3aa17

Signed-off-by: Yan Xue <[email protected]>

Revert unnecessary change

9de8afe

Signed-off-by: Yan Xue <[email protected]>

CR requested change

85cec32

Signed-off-by: Yan Xue <[email protected]>

Change comment

edeaae9

Signed-off-by: Yan Xue <[email protected]>

Replace auto with type

8d33bd5

Signed-off-by: Yan Xue <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filter: exposed functions to Lua to verify digital signature #7050

filter: exposed functions to Lua to verify digital signature #7050

yxue commented May 23, 2019

dio left a comment

htuch commented May 28, 2019

yxue commented May 29, 2019 •

edited

Loading

repokitteh-read-only bot commented May 29, 2019

lizan left a comment

htuch commented May 30, 2019

htuch left a comment

yxue commented Jun 4, 2019

dio left a comment

yxue commented Jun 5, 2019

lizan left a comment

lizan left a comment

yxue commented Jun 5, 2019

yxue commented Jun 5, 2019

repokitteh-read-only bot commented Jun 5, 2019

htuch left a comment

htuch left a comment

yxue commented Jun 5, 2019

dio left a comment

knrc commented Jun 7, 2019

filter: exposed functions to Lua to verify digital signature #7050

filter: exposed functions to Lua to verify digital signature #7050

Conversation

yxue commented May 23, 2019

dio left a comment

Choose a reason for hiding this comment

htuch commented May 28, 2019

yxue commented May 29, 2019 • edited Loading

repokitteh-read-only bot commented May 29, 2019

lizan left a comment

Choose a reason for hiding this comment

htuch commented May 30, 2019

htuch left a comment

Choose a reason for hiding this comment

yxue commented Jun 4, 2019

dio left a comment

Choose a reason for hiding this comment

yxue commented Jun 5, 2019

lizan left a comment

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

yxue commented Jun 5, 2019

yxue commented Jun 5, 2019

repokitteh-read-only bot commented Jun 5, 2019

htuch left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

yxue commented Jun 5, 2019

dio left a comment

Choose a reason for hiding this comment

knrc commented Jun 7, 2019

yxue commented May 29, 2019 •

edited

Loading