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.

Sign up for GitHub

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

Jump to bottom

fix poolname to include the digest of the cert for mTLS #307

Merged
merged 21 commits into from
Feb 29, 2024
Merged

fix poolname to include the digest of the cert for mTLS #307

merged 21 commits into from
Feb 29, 2024

Conversation

catbro666
Copy link
Contributor

Fix #306

Additionally, add the latest openresty versions in CI. OpenResty has already supported tcpsock:setclientcert from 1.21.4.2 so only skip the mtls tests when version < 1.21.4

@catbro666 catbro666 force-pushed the mtls-auth-pool-name-adds-cert-hash branch from deec9d3 to aa3c82a Compare January 10, 2024 06:06
@catbro666 catbro666 force-pushed the mtls-auth-pool-name-adds-cert-hash branch from 290cb84 to bc289e6 Compare January 10, 2024 06:46
@catbro666
Copy link
Contributor Author

Hi @pintsized could you review and merge this when you have time?

@catbro666
Copy link
Contributor Author

ping this

tzssangglass
tzssangglass approved these changes Jan 24, 2024
View reviewed changes
Copy link

@tzssangglass tzssangglass left a comment

Choose a reason for hiding this comment

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

LGTM

@catbro666
Copy link
Contributor Author

ping @pintsized This one has been open for a long time. Could you have a look?

@pintsized
Copy link
Member

ping @pintsized This one has been open for a long time. Could you have a look?

Sorry, I have no time to look at this at the moment. I'd appreciate some more eyes on it if anyone has time, perhaps some Kong folk? @Tieske @chronolaw

chronolaw
chronolaw reviewed Feb 26, 2024
View reviewed changes
lib/resty/http_connect.lua Outdated Show resolved Hide resolved
lib/resty/http_connect.lua Outdated Show resolved Hide resolved
lib/resty/http_connect.lua Outdated Show resolved Hide resolved
lib/resty/http_connect.lua Outdated Show resolved Hide resolved
Tieske
Tieske suggested changes Feb 27, 2024
View reviewed changes
lib/resty/http_connect.lua Outdated Show resolved Hide resolved
lib/resty/http_connect.lua Outdated Show resolved Hide resolved
@catbro666 catbro666 requested a review from Tieske February 27, 2024 14:54
@Tieske
Copy link
Contributor

Tieske commented Feb 27, 2024

@chronolaw can you have another look. lgtm now.

chronolaw
chronolaw approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@chronolaw chronolaw left a comment

Choose a reason for hiding this comment

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

LGTM

@chronolaw
Copy link
Contributor

I notice that there is some code (https://github.com/ledgetech/lua-resty-http/pull/307/files#diff-a717dcbee90c573feec0be98675c64ff06bf5bb3d8bb3d30b66b131952e22a3cR169),
it return three values, but the client only uses two (ok, err), is it a bug? should we fix it?

@catbro666
Copy link
Contributor Author

catbro666 commented Feb 28, 2024 •
edited
Loading

I notice that there is some code (https://github.com/ledgetech/lua-resty-http/pull/307/files#diff-a717dcbee90c573feec0be98675c64ff06bf5bb3d8bb3d30b66b131952e22a3cR169), it return three values, but the client only uses two (ok, err), is it a bug? should we fix it?

Yeah, I think so. Fixed them.

@Tieske
Copy link
Contributor

Tieske commented Feb 28, 2024

@pintsized I think this is good to go now.

@pintsized
Copy link
Member

Great work everyone, thank you!

@pintsized pintsized merged commit bf3e411 into ledgetech:master Feb 29, 2024
5 checks passed
@catbro666
Copy link
Contributor Author

catbro666 commented Feb 29, 2024 •
edited
Loading

@pintsized Could you bump a new version? Appreciate it.

@pintsized
Copy link
Member

@pintsized Could you bump a new version? Appreciate it.

0.17.2 is now on luarocks. I'm having some issues with opm, but it will be there once resolved.

@piotrp piotrp mentioned this pull request Sep 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chronolaw chronolaw chronolaw approved these changes

@tzssangglass tzssangglass tzssangglass approved these changes

@Tieske Tieske Tieske approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The poolname doesn't include the client cert info for mtls authentication
5 participants
@catbro666 @pintsized @Tieske @chronolaw @tzssangglass