feature: support load private key with password #973

detailyang · 2017-02-02T15:13:45Z

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

@agentzh This feature is separated at #957 . Now It adds the FFI API as the following:

int
ngx_http_lua_ffi_priv_key_pem_to_der_with_password(const u_char *pem,
    size_t pem_len, const u_char *pwd, size_t pwd_len, u_char *der, char **err);

void *
ngx_http_lua_ffi_parse_pem_priv_key_with_password(const u_char *pem,
    size_t pem_len, const u_char *pwd, size_t pwd_len, char **err);

When we think it can be merged to master, I will create another PR for lua-resty-core to implment load private with password.

agentzh · 2017-02-02T19:14:46Z

src/ngx_http_lua_ssl_certby.c

+{
+    BIO         *in;
+    EVP_PKEY    *pkey;
+    ngx_str_t   password;


Coding style issue. Please use the ngx-releng script to check your code. Thank you.

stupid error:(.

agentzh · 2017-02-02T19:15:35Z

t/140-ssl-c-api.t

+
+
+
+=== TEST 6: simple cert + private key with password


Better create a new .t file to avoid growing this test file from forever :)

Also, we can simply test the Lua API here instead of the plain C API since lua-resty-core is already visible to this module's test suite.

Finally, I think you can use the new add_block_preprocessor feature of Test::Nginx::Socket to avoid the massive code duplication in these SSL related tests. See

http://search.cpan.org/dist/Test-Nginx/lib/Test/Nginx/Socket.pm#add_block_preprocessor

FWIW, the pattern i've been using is testing the C FFI function in lua-nginx-module and test the Lua function in lua-resty-core. Otheriwse you create a co-dependency on both repos that makes the tests fail for both lua-nginx-module and lua-resty-core PRs until you merge both. (Assuming this is what you meant in the first paragraph).

@agentzh Thanks for you suggestion, I will take a look at add_block_preprocessor.

@ghedo ~~what's your meaning? My plan is to add the new api at first. When we let it merge to master then creating another PR to lua-resty-core. Is that wrong?~~

Now I know what's yours meaning, So we just simply test the C API in lua-nginx-module and test the Lua API in lua-resty-core :D

thibaultcha · 2017-02-02T19:40:17Z

src/ngx_http_lua_ssl_certby.c

+    int          len;
+    BIO         *in;
+    EVP_PKEY    *pkey;
+    ngx_str_t   password;


idem (missing space)

stupid error:(.

ghedo · 2017-02-02T22:40:20Z

src/ngx_http_lua_ssl.c

@@ -34,4 +34,35 @@ ngx_http_lua_ssl_init(ngx_log_t *log)
 }


+int
+ngx_http_lua_ssl_password_callback(char *buf, int size, int rwflag,


This should probably be moved to ngx_http_lua_ssl_certby.c and declared static since it's only used there.

maybe ngx_http_lua_ssl_password_callback is a useful function ? I mean that it will be used in other scene like cosocket in future.

@detailyang I personally prefer @ghedo 's suggestion.
When it will be used in other place, we can move it out in the future :)

detailyang · 2017-02-03T14:52:00Z

@agentzh @ghedo @thibaultcha

Will you please look at the new codebase, Many thanks :D

doujiang24 · 2017-02-05T11:01:27Z

src/ngx_http_lua_ssl.c

@@ -34,4 +34,35 @@ ngx_http_lua_ssl_init(ngx_log_t *log)
 }


+int
+ngx_http_lua_ssl_password_callback(char *buf, int size, int rwflag,


@detailyang I personally prefer @ghedo 's suggestion.
When it will be used in other place, we can move it out in the future :)

doujiang24 · 2017-02-05T11:04:15Z

src/ngx_http_lua_ssl_certby.c

@@ -982,6 +982,51 @@ ngx_http_lua_ffi_priv_key_pem_to_der(const u_char *pem, size_t pem_len,
 }


+int
+ngx_http_lua_ffi_priv_key_pem_to_der_with_password(const u_char *pem,
+    size_t pem_len, const u_char *pwd, size_t pwd_len, u_char *der, char **err)


How about reuse the current ngx_http_lua_ffi_priv_key_pem and pwd == NULL means without password?
So that we can reuse most of the code.

@doujiang24 I have considered the ngx_http_lua_ffi_priv_key_pem. But there are some existing tests which are rely on it. I'm not sure it's worth to change the ngx_http_lua_ffi_priv_key_pem. So just add the new api ngx_http_lua_ffi_priv_key_pem_to_der_with_password, then reimplement the Lua FFI API ngx.ssl.priv_key_pem_to_der to support password

So if we almost agree to reimplement the ngx_http_lua_ffi_priv_key_pem, I will try to do :D

doujiang24 · 2017-02-06T02:13:56Z

t/141-ssl-c-api-password.t

@@ -0,0 +1,584 @@
+# vim:set ft= ts=4 sw=4 et fdm=marker:


I think it's fine to use t/151-ssl-c-api-password.t, so that we don't need to rename t/141-luajit.t to t/151-luajit.t.
The num order is not important here:)

detailyang · 2017-02-06T05:58:33Z

@doujiang24 @ghedo @agentzh

Now the only question is that we can choose to reimplement ngx_http_lua_ffi_priv_key_pem_to_der and ngx_http_lua_ffi_parse_pem_priv_key to adding the password param but may introduce the risk because of doing more change for some tests and lua-resty-core or adding the new API ngx_http_lua_ffi_priv_key_pem_to_der_with_password and ngx_http_lua_ffi_parse_pem_priv_key_with_password

What's your guys suggestion?

guanglinlv · 2017-03-03T09:06:01Z

@detailyang , see #750 if you want to reuse ngx_http_lua_ffi_priv_key_pem_to_der.

agentzh · 2017-03-03T20:26:51Z

@detailyang Huh, that is another reason why we shouldn't call into C functions directly through FFI in lua-nginx-module's test suite. We should really only test the lua-resty-core Lua API instead ;)

detailyang · 2017-03-04T07:21:02Z

@agentzh Thanks.
Finally I do understand what's the meaning for 'simply test' when you are always teaching me 'simply test' in some code reviews :D

zhuizhuhaomeng · 2022-01-23T06:40:01Z

This PR implements the same function #1991
We have merged that.

feature: support load private key with password

2dc8e74

agentzh requested changes Feb 2, 2017

View reviewed changes

thibaultcha reviewed Feb 2, 2017

View reviewed changes

ghedo reviewed Feb 2, 2017

View reviewed changes

detailyang added 6 commits February 3, 2017 22:14

style: align

e520238

tests: move password test to new .t

c3b9a38

revert t/140-ssl-c-api.t to old version

1a1162f

style: remove unused whitespace

5c60231

style: add blank line for .t file

ab2d04f

tests: optimize add_block_preprocessor

a42f837

doujiang24 reviewed Feb 6, 2017

View reviewed changes

detailyang added 2 commits February 6, 2017 13:05

rename 151-luajit.t and 141-ssl-c-api-password.t

cacdcfa

set ngx_http_lua_ssl_password_callback is static

66d45b8

thibaultcha force-pushed the master branch from a79e9aa to 315330a Compare August 27, 2019 21:35

zhuizhuhaomeng force-pushed the master branch from f924579 to fef2581 Compare September 20, 2021 11:01

zhuizhuhaomeng closed this Jan 23, 2022

feature: support load private key with password #973

feature: support load private key with password #973

Uh oh!

Conversation

detailyang commented Feb 2, 2017

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!

detailyang Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

detailyang commented Feb 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

detailyang Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

detailyang commented Feb 6, 2017

Uh oh!

guanglinlv commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agentzh commented Mar 3, 2017

Uh oh!

detailyang commented Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhuizhuhaomeng commented Jan 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

detailyang Feb 3, 2017 •

edited

Loading

detailyang Feb 6, 2017 •

edited

Loading

guanglinlv commented Mar 3, 2017 •

edited

Loading

detailyang commented Mar 4, 2017 •

edited

Loading

zhuizhuhaomeng commented Jan 23, 2022 •

edited

Loading