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

Update lua-resty-jwt for OpenSSL 1.1 #165

Merged
merged 2 commits into from
Jun 7, 2018

Conversation

cdbattags
Copy link
Contributor

@cdbattags cdbattags commented Jun 6, 2018

This should add support for OpenSSL 1.1 with the new fork of https://github.com/SkyLothar/lua-resty-jwt at https://github.com/cdbattags/lua-resty-jwt!

Still very confused that you're using two different versions of lua-resty-hmac with OPM version and LuaRocks...

@ChristianCiach
Copy link

I highly recommend to merge this, because the current dependency setup breaks new and updated installations of OpenResty with lua-resty-openidc

@cdbattags
Copy link
Contributor Author

cdbattags commented Jun 6, 2018

/agree with @ChristianCiach but I get that y'all want to take your time on reviewing everything I've done.

Maybe the fact that my module now being in the root manifest for LuaRocks will help assuage your worries? 😜

As seen: http://luarocks.org/modules/cdbattags/lua-resty-jwt

Update:

"All checks have passed" per https://travis-ci.org/zmartzone/lua-resty-openidc/builds/388932092?utm_source=github_status&utm_medium=notification

Please cut a new version for OPM and LuaRocks?

@ChristianCiach
Copy link

This is offtopic, but a good moment to ask about your opinion:

I know that OpenResty itself strongly discourages the use of LuaRocks in favor of OPM. What do you think about that? I have the feeling that LuaRocks is a lot more mature than OPM. And in this specific case, using LuaRocks would have fixed this issue a lot faster, because it would have pulled your fork of lua-resty-jwt.

@cdbattags
Copy link
Contributor Author

cdbattags commented Jun 6, 2018

OPM is extremely early on to be quite honest. The fact that I couldn't even delete from the registry is crazy to me. Only problem with this though is that lua-resty-hmac that's used in the OPM package is completely different than the one in the LuaRocks package... which is concerning.

So if we can get https://github.com/jkeys089/lua-resty-hmac on LuaRocks then we'd be set. Because in the current state on LuaRocks I still don't think this is fully OpenSSL 1.1 ready without the changes @jkeys089 has in his package.

Edit:

See jkeys089/lua-resty-hmac#9!

dist.ini Outdated
@@ -4,4 +4,4 @@ author = Hans Zandbelt (@zandbelt)
is_original = yes
license = apache2
repo_link = https://github.com/zmartzone/lua-resty-openidc
requires = openresty, pintsized/lua-resty-http >= 0.08, bungle/lua-resty-session >= 2.8, SkyLothar/lua-resty-jwt >= 0.1.5, jkeys089/lua-resty-hmac
requires = openresty, pintsized/lua-resty-http >= 0.08, bungle/lua-resty-session >= 2.8, cdbattags/lua-resty-jwt >= 0.2.0, jkeys089/lua-resty-hmac >= 0.2
Copy link
Contributor

Choose a reason for hiding this comment

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

so jkeys089/lua-resty-hmac >= 0.2 now but I only see 0.0.2 in the OPM repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, whoops. Updating now.

@@ -25,7 +25,7 @@ dependencies = {
"lua >= 5.1",
"lua-resty-http >= 0.08",
"lua-resty-session >= 2.8",
"lua-resty-jwt >= 0.1.5",
"lua-resty-jwt >= 0.2.0",
"lua-resty-hmac"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess because lua-resty-jwt comes with its own (/vendor) version of lua-resty-hmac, this dependency can be removed (Travis CI would verify this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thoughts exactly.

dist.ini Outdated
@@ -4,4 +4,4 @@ author = Hans Zandbelt (@zandbelt)
is_original = yes
license = apache2
repo_link = https://github.com/zmartzone/lua-resty-openidc
requires = openresty, pintsized/lua-resty-http >= 0.08, bungle/lua-resty-session >= 2.8, cdbattags/lua-resty-jwt >= 0.2.0, jkeys089/lua-resty-hmac >= 0.2
requires = openresty, pintsized/lua-resty-http >= 0.08, bungle/lua-resty-session >= 2.8, cdbattags/lua-resty-jwt >= 0.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I did not mean to remove it from the OPM dist.ini: AFAIK that format doesn't have the option to explicitly include dependencies

@cdbattags cdbattags force-pushed the update-lua-resty-jwt branch from e38b560 to f06ca70 Compare June 6, 2018 20:37
@cdbattags
Copy link
Contributor Author

cdbattags commented Jun 6, 2018

Sorry, this double package manager stuff is quite confusing... =P

Assuming this passes all tests we should be good to go!

@zandbelt
Copy link
Contributor

zandbelt commented Jun 6, 2018

Ok, I'm confused too :-)
I guess you we're right about removing the explicit lua-resty-hmac dependency from lua-resty-openidc OPM dist.ini file anyhow: since lua-resty-jwt takes care of its own dependencies (and you've specified this dependency in the dist.ini of your lua-resty-jwt.

But: that brings us back to this question #165 (comment) since it really is lua-resty-jwt that includes its own version of lua-resty-hmac in luarocks, but pulls jkeys089/lua-resty-hmac from the repo in OPM ...

@cdbattags
Copy link
Contributor Author

cdbattags commented Jun 6, 2018

So I guess let us worry about the dep then? Since you never explicitly call it anywhere in yours that makes the most sense, no?

This way OPM will be the most reasonable way to use this package but we will support on LuaRocks through the vendor directory until we get @jkeys089 to publish under some different name.

@zandbelt
Copy link
Contributor

zandbelt commented Jun 6, 2018

sounds good

@cdbattags
Copy link
Contributor Author

Ok, updating this one last time then without dist.ini dep and we wait for Travis and then go for it.

@cdbattags cdbattags force-pushed the update-lua-resty-jwt branch from f06ca70 to 3d3b39e Compare June 6, 2018 20:52
@cdbattags
Copy link
Contributor Author

Ok, should be good to go! Can we get this merged and fresh uploads to OPM and LuaRocks?

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.

3 participants