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

fix: check decrypt key to prevent lua thread aborted #2815

Merged
merged 1 commit into from
Nov 24, 2020

Conversation

starsz
Copy link
Contributor

@starsz starsz commented Nov 22, 2020

What this PR does / why we need it:

Hi, I found that the decrypt function in aes.lua will cause lua thread aborted if the key is nil.

fix: #2791

image

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible?

@tokers
Copy link
Contributor

tokers commented Nov 22, 2020

@starsz Your git commit message should be "semantic": https://github.com/zeke/semantic-pull-requests

@starsz starsz force-pushed the fix_2791 branch 3 times, most recently from 9e700e3 to afacc0e Compare November 22, 2020 13:36
@starsz starsz changed the title WIP: fix: check decrypt key to prevent lua thread aborted (#2791) fix: check decrypt key to prevent lua thread aborted (#2791) Nov 22, 2020
@starsz
Copy link
Contributor Author

starsz commented Nov 22, 2020

@starsz Your git commit message should be "semantic": https://github.com/zeke/semantic-pull-requests

Because the title is started with "WIP". Now it's fixed.

@starsz starsz changed the title fix: check decrypt key to prevent lua thread aborted (#2791) fix: check decrypt key to prevent lua thread aborted Nov 22, 2020
local core = require("apisix.core")
local t = require("lib.test_admin")

local ssl_cert = t.read_file("t/certs/miss_head.crt")
Copy link
Contributor

@tokers tokers Nov 23, 2020

Choose a reason for hiding this comment

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

The ssl creation will be failed if #2816 is solved. You have to solve this case that time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or add a TODO comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add TODO comment and I will solve this case in SSL certificate check PR.

@@ -0,0 +1,25 @@
MIIEojCCAwqgAwIBAgIJAK253pMhgCkxMA0GCSqGSIb3DQEBCwUAMFYxCzAJBgNV
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to introduce yet another bad cert and key just for mimicking bad base64 decoded strings. Do it by literal strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

return decrypted
local decoded_key = ngx_decode_base64(key)
if not decoded_key then
core.log.error("base64 decode ssl key failed. key[", key, "] ")
Copy link
Member

Choose a reason for hiding this comment

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

Better to mention that we will skip the bad key in the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

core.log.error("decrypt ssl key failed. key[", key, "] ")
local decrypted = iv:decrypt(decoded_key)
if not decrypted then
core.log.error("decrypt ssl key failed. key[", key, "] ")
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

@starsz starsz force-pushed the fix_2791 branch 2 times, most recently from 05378b3 to 56aa1d6 Compare November 23, 2020 12:55
@spacewander spacewander merged commit 95226d9 into apache:master Nov 24, 2020
@moonming moonming added this to the 2.1 milestone Nov 25, 2020
@starsz starsz deleted the fix_2791 branch October 18, 2023 07:11
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.

request help: After put my SSL certificate to apisix ,I curl https and then meet some problem
6 participants