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(env): support for uppercase reference to get ENV and SECRET #11172

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

LinkinStars
Copy link
Member

@LinkinStars LinkinStars commented Apr 19, 2024

Description

Fixes #11141

"cert": "$ENV://APISIX_ENV_CERT",
"key": "$ENV://APISIX_ENV_KEY",

Only lowercase is matched in the 'cert' and 'key' parameters. To maintain consistency, match the uppercase as well.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@shreemaan-abhishek
Copy link
Contributor

@LinkinStars please add test cases as well

@shreemaan-abhishek
Copy link
Contributor

also, please fix the code lint problem

@LinkinStars
Copy link
Member Author

also, please fix the code lint problem

I think it's probably not a code issue that's causing the lint to not pass.

image

FYI: https://github.com/orgs/community/discussions/120966

@shreemaan-abhishek
Copy link
Contributor

@LinkinStars, okay. Please rebase with master for the fix for CI failures.

@shreemaan-abhishek
Copy link
Contributor

@LinkinStars I think using lowercase "$env://" and uppercase "$SECRET://" might still not work as the code hardcodes the case for sanity checks:

Hardcoding for secret reference:
https://github.com/shreemaan-abhishek/apisix/blob/07b3cc3f63db8fc4ae3706baa324a76ee3049338/apisix/secret.lua#L34
https://github.com/shreemaan-abhishek/apisix/blob/07b3cc3f63db8fc4ae3706baa324a76ee3049338/apisix/secret.lua#L121

Hardcoding for env reference:
https://github.com/shreemaan-abhishek/apisix/blob/07b3cc3f63db8fc4ae3706baa324a76ee3049338/apisix/core/env.lua#L30
https://github.com/shreemaan-abhishek/apisix/blob/07b3cc3f63db8fc4ae3706baa324a76ee3049338/apisix/core/env.lua#L70

If you were to write test cases covering the above situation it would fail. What could be a good solution for this?

@LinkinStars
Copy link
Member Author

@shreemaan-abhishek

First, we discuss the issue of env.

After my testing, both the uppercase and lowercase should be supported after changes. There are two reasons for this

  1. unit test 18 passed https://github.com/shreemaan-abhishek/apisix/blob/07b3cc3f63db8fc4ae3706baa324a76ee3049338/t/router/radixtree-sni2.t#L700-L725
  2. user can use it after changing to lowercase bug: unable to setup ssls with env reference #11141 (comment)

After reading the code I found out why. For env, both checking and parsing converted the target to uppercase preferentially.

if string.has_prefix(upper(uri), core.env.PREFIX) then

Because when characters are cut, the character length is used. Both uppercase and lowercase lengths are the same, so there's no problem.

local path = sub(env_uri, #ENV_PREFIX + 1)


Secondly, let's discuss the secret.

Unfortunately, as you said, using the uppercase 'SECRET' is problematic.

I tried adding unit tests and found that they could not pass. The reason is quite simple: 'secret' is not converted to uppercase like 'env' before comparison and parsing.

Dig more. I find the git history. The 'secret' was previously modified by KMS, and there was uppercase conversion before.

1929250494

However, $SECRET has never been used. It's not even mentioned in the documentation.

So, in my opinion, I would not recommend supporting uppercase SECRET. I think the author who wrote the code at that time must have also considered that.

Of course, these are just my personal thoughts, if there is anything incorrect, please point it out.


All in all, there are two options.

  1. not support $SECRET
  2. suport $SECRET, just like the ENV. such asstring.has_prefix(upper(uri), secret.PREFIX)

@shreemaan-abhishek
Copy link
Contributor

let's only support uppercase for ENV and leave secret as it is. Really appreciate the detailed explanation 🙏🏼

Copy link
Contributor

@Neilblaze Neilblaze left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

bug: unable to setup ssls with env reference
3 participants