Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for HMAC-SHA256 and other SHA* algorithms #18

Closed
wants to merge 7 commits into from

Conversation

ddragosd
Copy link

I've added a wrapper on top of openssl libraries to bring support in ngx lua for HMAC-SHA* algorithms.
The supported algorithms are: sha1, sha224, sha256, sha384 and sha512.

HMAC-SHA256 is especially important for AWS V4 auth.

@agentzh
Copy link
Member

agentzh commented May 17, 2014

@ddragosd Thank you for working on this!

Several comments though:

  1. Please avoid the use of the module() function in your module definition, which has bad side-effects like creating globals. Also, avoid using metamethod for checking misuse of globals, because we have a better tool, lua-releng. See https://github.com/openresty/lua-nginx-module#lua-variable-scope for more details :)
  2. Please use ffi.typeof to create ctype objects on the toplevel of your module and use them directly in hot code paths like your funciton digest instead of letting LuaJIT to parse the ctype strings "int[?]" and "char[?]" upon every call. See http://luajit.org/ext_ffi_api.html#ffi_typeof

@ddragosd
Copy link
Author

Hi @agentzh thanks for feedback. I'm going to make the changes you have suggested early next week.

I agree with the first point; I was trying to follow the model of the existing code to be consistent, but I was on version 0.08 from openresty-1.5.11.1 . I'll update the code to be consistent with the current version and I'm updating to ffi.typeof as well. Thanks for the tip !

@ddragosd
Copy link
Author

Hi @agentzh . I've implemented your suggestions. On the second item I have also instantiated the hashing algorithms in the local variable available_algorithms, i.e.
sha256 = { alg = C.EVP_sha256(), length = 256/8 } (updated)
All tests are passing now. Let me know what you think.
Thanks !

@agentzh
Copy link
Member

agentzh commented May 22, 2014

@ddragosd Thank you for the fixes! I'll look into it when I have some spare time. Too busy with $work atm, sorry :(

@lorenanicole
Copy link

Will these be added soon?

@agentzh
Copy link
Member

agentzh commented Jul 1, 2014

@lorenanicole As soon as I can manage :) Sorry for the delay on my side.

@lorenanicole
Copy link

Thanks - looking to use this to generate some fun SHA256s here with secret keys galore 👍

-- @param msg The message to be signed
-- @param raw When true, it returns the binary format, else, the hex format is returned
--
function _M.digest(self, dtype, key, msg, raw)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a new context for every MAC which can get expensive if you're creating / verifying many MAC values. Also, it doesn't allow for a single MAC to be updated at a later time (e.g. for large values / stream values).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. Line 84 with C.HMAC method creates a new context each time indeed.
This worked perfectly fine for my use case, but I also see your use case as another nice-to-have.
Would adding 3 new methods similar to hmac.h : init, update and final and maybe a 4th reset cover all scenarios ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think adding update, final and reset would cover our needs (init could just be done inside new).
We actually just published a standalone module which includes exactly this functionality: https://github.com/jkeys089/lua-resty-hmac

Had I known someone else was working on this I would have collaborated with you directly.
Perhaps we could just contribute the code we've published back into the official lua-resty-string? I'd happy to prepare a PR if you think it makes sense.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkeys089 sounds great ! I'm all for collaboration also.
For some reason I'm thinking that the init method makes sense if we want to keep the exiting digest method, just to keep the API simple for the cases that don't need appending capabilities.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To use the existing digest method do we even need a new instance? I think it can just be called without reference to self and it will work in exactly the same way (i.e. remove self argument from the digest method and call it like hmac.digest("sha256", "sec-key", "some msg")) .

It doesn't make too much difference I think but it does keep the API a bit simpler and keeps the API similar other libs (e.g.

function _M.new(self, key, salt, _cipher, _hash, hash_rounds)
)

@zopieux
Copy link

zopieux commented Apr 10, 2015

Any progress on this PR? HMAC256 is quite widespread!

@agentzh
Copy link
Member

agentzh commented Apr 10, 2015

@zopieux Sorry for the delay on my side. Unfortunately I'm still swamped by other things atm. I'll look into this as soon as I can manage :)

@zopieux
Copy link

zopieux commented Apr 11, 2015

@agentzh No problem, thanks!

@xfalcox
Copy link

xfalcox commented Jun 30, 2015

Guys I need this ASAP to create a SSO endpoint for Discourse with OpenResty. Can someone point me a lib to use while this doesn't get merged? I need HMAC-SHA256.

@ddragosd
Copy link
Author

Hi @xfalcox, while waiting for the merge you can copy the hmac.lua file; you should also make sure you have OpenSSL installed.

@agentzh
Copy link
Member

agentzh commented Jul 1, 2015

Sorry for the long delay on my side. I'll try to find the time to review this :)

@xfalcox
Copy link

xfalcox commented Sep 1, 2015

Thanks @ddragosd !!! It works wonders!

@thibaultcha
Copy link
Member

👍

@dobesv
Copy link

dobesv commented Oct 27, 2015

Hi, we're hoping to use this in the Hawk auth support for Resty, for sha256 support ( golgote/lua-resty-hawk#5 ) but since it's not merged in it's a bit weird to have it as a requirement.

We've been locally patching lua-resty-string and lua-resty-hawk manually but it would be great if these were both merged upstream so we can go back to using the baseline.

The library is working for us, FWIW.

Any change of a merge? This won't break anything else if it's not in use, so worst case scenario it has a bug which is filed as an issue later on.

@ddragosd
Copy link
Author

If it helps, there is a module hosted here: https://github.com/adobe-apiplatform/api-gateway-hmac until this gets merged.

We needed it for the AWS V4 signature used in the AWS Lua SDK for Openresty

More than happy to go with lua-resty-string once merged.

@mynameiscfed
Copy link

@agentzh Is there anything blocking this from being merged?

@agentzh
Copy link
Member

agentzh commented Jan 26, 2016

I suggest creating a separate lua-resty-* library for this. I've never found the time to review and merge all such big PRs into the lua-resty-string library. Besides, lua-resty-string is already a mess and I don't want it to grow forever :)

@agentzh
Copy link
Member

agentzh commented Jan 26, 2016

Thanks for your understanding.

@bungle
Copy link
Member

bungle commented Jan 26, 2016

lua-resty-string is quite a mess in sense that it supplies multiple files under resty namespace. The name of the library is also a bit misleading. It has hashing, somewhat lacking symmetrical encryption, random generation, and to_hex and atoi.

I think this should probably be split to these:

  • lua-resty-hash
  • lua-resty-random
  • lua-resty-crypt
  • lua-resty-string

I think that resty-libraries should supply at most one file in resty namespace.

On a sidenote, I have build FFI bindings to GNU nettle. It has all these missing crypto primitives:
https://github.com/bungle/lua-resty-nettle

Just look inside this directory:
https://github.com/bungle/lua-resty-nettle/tree/master/lib/resty/nettle

@agentzh
Copy link
Member

agentzh commented Jan 26, 2016

@bungle Agreed.

@qbcs
Copy link

qbcs commented Jan 27, 2016

@ddragosd Thanks for your work. I'm using openresty to request AWS S3 service. AWS V4 Signature needs HMAC_SHA256.

@ddragosd
Copy link
Author

@biwing thanks for the feedback. Which AWS service are you using ? I'm asking as there's this AWS SDK : https://github.com/adobe-apiplatform/api-gateway-aws that leverages this HMAC impl.

@qbcs
Copy link

qbcs commented Jan 27, 2016

@ddragosd I'm using s3 storage service.

@xfalcox
Copy link

xfalcox commented Jul 25, 2016

Hey @ddragosd after a while using the sha256, the code begins to fail with the following:

2016/07/25 15:55:43 [error] 532#0: *146497668 lua entry thread aborted: runtime error: table overflow
stack traceback:
coroutine 0:
        [C]: in function 'cdef'
        /usr/local/openresty/nginx/lua/tests/test.lua:49: in function </usr/local/openresty/nginx/lua/tests/test.lua:1> while sending to client

Where line 49 is the last line here:

--
-- EVP_MD is defined in openssl/evp.h
-- HMAC is defined in openssl/hmac.h
--
ffi.cdef[[
typedef struct env_md_st EVP_MD;
typedef struct env_md_ctx_st EVP_MD_CTX;
unsigned char *HMAC(const EVP_MD *evp_md, const void *key, int key_len,
            const unsigned char *d, size_t n, unsigned char *md,
            unsigned int *md_len);
const EVP_MD *EVP_sha1(void);
const EVP_MD *EVP_sha224(void);
const EVP_MD *EVP_sha256(void);
const EVP_MD *EVP_sha384(void);
const EVP_MD *EVP_sha512(void);
]]

Maybe this is related to: https://github.com/openresty/lua-resty-string/pull/18/files#r15120852 ?

A restart solves this, and it takes a month for this to happen.

@agentzh
Copy link
Member

agentzh commented Jul 25, 2016

@xfalcox You should use require to load the Lua module so that the top-level code is run only once. Otherwise you are calling ffi.cdef upon every request, which will exhaust the internal LuaJIT table very quickly.

@andy-zhangtao
Copy link

@agentzh Hi agentzh, any progress on this PR? We also need this feature, so could you please merge it asap... :)


-- 64 is the max lenght and it covers up to sha512 algorithm
local digest_len = ffi_new("int[?]", 64)
local buf = ffi_new("char[?]", 64)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this declaring a single buffer instance that gets reused across all instances of hmac? I'm new to Lua, but if this is the case, that would be dangerous (wouldn't it?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljwagerfield This is a temporary buffer that is used inside a single Lua function that never yields. Since nginx uses single threaded processes and it does not yield for Lua light threads, it is safe.

@takakawa
Copy link

4 years passed,any progress?

@agentzh
Copy link
Member

agentzh commented Jan 17, 2018

@andy-zhangtao @takakawa As discussed above, such new new openssl related features should go to separate lua-resty-* libraries. This lua-resty-string library is already quite a mess.

@agentzh agentzh closed this Jan 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.