support for encrypted PEM keys #128

kunkku · 2018-05-03T17:52:31Z

No description provided.

src/openssl.c

kunkku · 2018-07-02T19:06:50Z

Please let me know if further changes are required to this patch set.

daurnimator

A couple of technical issues spotted; I still need to do a more thorough review.

src/openssl.c

daurnimator · 2018-07-03T01:09:28Z

src/openssl.c

@@ -3674,6 +3687,8 @@ static int pk_new(lua_State *L) {
 			}
 		}

+		ud = prepsimple(L, PKEY_CLASS);


This can throw and leak bio (which is why it was previously done before allocating bio)

src/openssl.c

ncopa · 2018-07-25T09:54:45Z

src/openssl.c

+	char *pass = (char *) u;
+	strncpy(buf, pass, size);
+	return MIN(strlen(pass), (unsigned int) size);
+} /* pem_password_cb() */


I guess there is a typo in the comment. Should be /* pem_pw_cb() */

This still needs fixing

daurnimator · 2018-08-13T02:34:36Z

src/openssl.c

+	lua_settop(L, 3);
+
+	if (cname) {
+		cipher = EVP_get_cipherbyname(cname);


This gives a warning:

src/openssl.c: In function ‘pk_getPrivateKey’: src/openssl.c:4132:10: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] cipher = EVP_get_cipherbyname(cname); ^

src/openssl.c

daurnimator · 2018-08-13T02:55:48Z

So this PR works, but I've found it very odd to use.

e.g. here is a valid invocation:

pk=require "openssl.pkey"
a=pk.new()
k=a:getPrivateKey("aes-256-cbc", "bar")
b = pk.new(k, "PEM", "private", "bar")

But the following invocations fail (with mostly hard to understand/debug error messages)

Passing "public" rather than "private":

$ lua -e 'pk=require "openssl.pkey";  a=pk.new(); k=a:getPrivateKey("aes-256-cbc", "bar") pk.new(k, "PEM", "public", "bar")'
lua: pkey.new: pem_lib.c:691:error:0906D06C:PEM routines:PEM_read_bio:no start line
stack traceback:
	[C]: in function 'openssl.pkey.new'
	(command line):1: in main chunk
	[C]: in ?

Passing wrong password (this is probably fine):

$ lua -e 'pk=require "openssl.pkey";  a=pk.new(); k=a:getPrivateKey("aes-256-cbc", "bar") pk.new(k, "PEM", "private", "foo")'
lua: pkey.new: evp_enc.c:536:error:06065064:digital envelope routines:EVP_DecryptFinal_ex:bad decrypt
stack traceback:
	[C]: in function 'openssl.pkey.new'
	(command line):1: in main chunk
	[C]: in ?

Passing wrong password and also no public vs private:

$ lua -e 'pk=require "openssl.pkey";  a=pk.new(); k=a:getPrivateKey("aes-256-cbc", "bar") pk.new(k, "PEM", nil, "foo")'
lua: pkey.new: pem_lib.c:691:error:0906D06C:PEM routines:PEM_read_bio:no start line
stack traceback:
	[C]: in function 'openssl.pkey.new'
	(command line):1: in main chunk
	[C]: in ?

Passing wrong password and no "PEM" choice:

$ lua -e 'pk=require "openssl.pkey";  a=pk.new(); k=a:getPrivateKey("aes-256-cbc", "bar") pk.new(k, nil, "private", "foo")'
lua: pkey.new: tasn_dec.c:1129:error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag
stack traceback:
	[C]: in function 'openssl.pkey.new'
	(command line):1: in main chunk
	[C]: in ?

Otherwise, I found it troublesome that not passing a cipher would result in no password being used.
Perhaps throw an error if a password is specified and a cipher isn't?

kunkku · 2018-09-02T09:49:59Z

I was thinking if the API could be something like this:

pk = require "openssl.pkey"
a = pk.new()
k = a:toPEM{type="private", cipher="aes-256-cbc", password="bar"}
b = pk.new(k, {format="PEM", type="private", password="bar"})

This would make the code more readable and there would be no need for a separate getPrivateKey method.

kunkku · 2018-09-29T14:15:56Z

The new patch set implements passing options via a table as suggested above, avoiding the need for a new getPrivateKey method.

vinayakhulawale · 2019-12-26T22:25:03Z

@kunkku - Is this patch safe to use ? are there any further changes expected ?

kunkku · 2019-12-27T00:46:35Z

Honestly, I do not know. I would like to get feedback from the maintainers.

daurnimator · 2019-12-30T15:49:31Z

src/openssl.c

+	char *pass = (char *) u;
+	strncpy(buf, pass, size);
+	return MIN(strlen(pass), (unsigned int) size);
+} /* pem_password_cb() */


This still needs fixing

daurnimator · 2019-12-30T15:52:33Z

src/openssl.c

+	lua_State *L = (lua_State *) u;
+
+	if (lua_isnil(L, -1) || (lua_isfunction(L, -1) && lua_pcall(L, 0, 1, 0)))
+		return 0;


This doesn't keep the stack balanced: if the field is nil then nothing is pushed. If the pcall results in an error then the error will be left on the stack.

I suppose this should work?

diff --git a/src/openssl.c b/src/openssl.c index 35183ab..868a6d3 100644 --- a/src/openssl.c +++ b/src/openssl.c @@ -4073,9 +4073,14 @@ static BIO *getbio(lua_State *L) { static int pem_pw_cb(char *buf, int size, int rwflag, void *u) { lua_State *L = (lua_State *) u; - if (lua_isnil(L, -1) || (lua_isfunction(L, -1) && lua_pcall(L, 0, 1, 0))) + if (lua_isnil(L, -1)) return 0; + if (lua_isfunction(L, -1) && lua_pcall(L, 0, 1, 0)) { + lua_pop(L, 1); + return 0; + } + const char *pass = lua_tostring(L, -1); if (!pass) return 0;

Please check the current implementation. In case of error, the error is replaced by nil to make the callback return directly if called again. The callback does not change the size of the stack on any execution path.

wahern/luaossl#128 backwards incompatible

daurnimator · 2020-06-14T22:15:38Z

src/openssl.c

+			lua_getfield(L, 2, "type");
+			lua_getfield(L, 2, "password");
+		} else
+			lua_pushnil(L);


This branch only pushes one item vs the other branch's 3

Now the net amount of pushed items is 1 in both branches.

daurnimator · 2020-06-14T22:21:53Z

src/openssl.c

@@ -4062,10 +4070,31 @@ static BIO *getbio(lua_State *L) {
 } /* getbio() */


+static int pem_pw_cb(char *buf, int size, int rwflag, void *u) {


Maybe worth adding a comment of "expects nil/function/string on top of the stack" and "leaves one item on the top of the stack".

"errors from the function are not reported"

perhaps it should leave nothing on the stack?

Requested comments added.

daurnimator · 2020-06-14T22:24:00Z

src/openssl.c

+					return luaL_error(L, "pkey:toPEM: password not defined");
+			}
+
+			if (!PEM_write_bio_PrivateKey(bio, key, cipher, NULL, 0, pem_pw_cb, L))


if there is no cname on the stack then what will be at top of stack?

Added an else branch which pushes a nil value. The value will not be used because openssl will not invoke the callback when cipher equals NULL.

daurnimator · 2020-06-19T11:33:49Z

src/openssl.c

 		size_t len;
 		BIO *bio;
 		EVP_PKEY *pub = NULL, *prvt = NULL;
 		int goterr = 0;

+		if (lua_istable(L, 2)) {
+			lua_pop(L, 1);


this could be popping the table itself; or the 3rd argument.

It is always the 3rd argument because stack size is set to 3 on line 4101. Of course, this can be changed to lua_remove if you prefer it that way.

daurnimator · 2020-06-19T11:48:07Z

src/openssl.c

+
+		/* #1 key, #2 format, #3 type, #4 password or callback */
+
+		data = luaL_checklstring(L, 1, &len);


the error message for this will have the wrong index

What should it be then? This was just moved from line 4321 to process the arguments in ascending order. The purpose was to improve readability.

kunkku force-pushed the password branch from b5cfce2 to e60b60a Compare May 3, 2018 19:17

daurnimator requested changes May 4, 2018

View reviewed changes

src/openssl.c Outdated Show resolved Hide resolved

src/openssl.c Outdated Show resolved Hide resolved

src/openssl.c Outdated Show resolved Hide resolved

src/openssl.c Outdated Show resolved Hide resolved

kunkku force-pushed the password branch 2 times, most recently from 70d1c68 to 0a358f8 Compare June 7, 2018 12:22

kunkku force-pushed the password branch from 0a358f8 to 42319d0 Compare July 1, 2018 14:48

daurnimator requested changes Jul 3, 2018

View reviewed changes

kunkku force-pushed the password branch from 42319d0 to 2cea827 Compare July 10, 2018 15:06

daurnimator requested changes Jul 12, 2018

View reviewed changes

src/openssl.c Outdated Show resolved Hide resolved

kunkku force-pushed the password branch from 2cea827 to cc6f021 Compare July 12, 2018 16:51

daurnimator requested changes Jul 13, 2018

View reviewed changes

src/openssl.c Show resolved Hide resolved

ncopa reviewed Jul 25, 2018

View reviewed changes

kunkku force-pushed the password branch from cc6f021 to b8c6bb0 Compare July 26, 2018 09:33

daurnimator requested changes Aug 13, 2018

View reviewed changes

kunkku force-pushed the password branch from b8c6bb0 to eaa5795 Compare September 14, 2018 15:56

kunkku requested a review from daurnimator December 27, 2019 00:45

daurnimator reviewed Dec 30, 2019

View reviewed changes

kunkku force-pushed the password branch from eaa5795 to cf65579 Compare January 12, 2020 17:20

algitbot pushed a commit to alpinelinux/aports that referenced this pull request Jan 12, 2020

main/lua-ossl: align with latest PEM encryption patch set

b9dd8d8

wahern/luaossl#128 backwards incompatible

daurnimator requested changes Jun 14, 2020

View reviewed changes

kunkku added 5 commits June 19, 2020 12:21

pkey.toPEM: opts: rename const array

db281ae

pkey.toPEM: accept table arguments

24ff6da

pkey.toPEM: private key encryption

2f246cc

pkey.new: type: rename variable

312269d

pkey.new: opt: rename variable

1d6f159

kunkku added 3 commits June 19, 2020 13:38

pkey.new: accept option table

53fd95f

pkey.new: decryption

89f26bb

pkey.toPEM: use password callback

4f41ae9

kunkku force-pushed the password branch 2 times, most recently from dd01937 to 005b8bf Compare June 19, 2020 11:10

pkey: PEM password callback

652de88

kunkku force-pushed the password branch from 005b8bf to 652de88 Compare June 19, 2020 11:18

daurnimator reviewed Jun 19, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for encrypted PEM keys #128

support for encrypted PEM keys #128

kunkku commented May 3, 2018

kunkku commented Jul 2, 2018

daurnimator left a comment

daurnimator Jul 3, 2018

ncopa Jul 25, 2018

daurnimator Dec 30, 2019

daurnimator Aug 13, 2018

daurnimator commented Aug 13, 2018

kunkku commented Sep 2, 2018

kunkku commented Sep 29, 2018

vinayakhulawale commented Dec 26, 2019

kunkku commented Dec 27, 2019

daurnimator Dec 30, 2019

daurnimator Dec 30, 2019

ncopa Jan 9, 2020

kunkku Jan 12, 2020

daurnimator Jun 14, 2020

kunkku Jun 19, 2020

daurnimator Jun 14, 2020

daurnimator Jun 14, 2020

kunkku Jun 19, 2020

daurnimator Jun 14, 2020

kunkku Jun 19, 2020

daurnimator Jun 19, 2020

kunkku Jun 19, 2020

daurnimator Jun 19, 2020

kunkku Jun 19, 2020

		@@ -4062,10 +4070,31 @@ static BIO getbio(lua_State L) {
		} /* getbio() */


		static int pem_pw_cb(char buf, int size, int rwflag, void u) {


		/* #1 key, #2 format, #3 type, #4 password or callback */

		data = luaL_checklstring(L, 1, &len);

support for encrypted PEM keys #128

Are you sure you want to change the base?

support for encrypted PEM keys #128

Conversation

kunkku commented May 3, 2018

kunkku commented Jul 2, 2018

daurnimator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daurnimator commented Aug 13, 2018

kunkku commented Sep 2, 2018

kunkku commented Sep 29, 2018

vinayakhulawale commented Dec 26, 2019

kunkku commented Dec 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment