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(key-auth): validate the configured headernames #2142

Closed
wants to merge 3 commits into from

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Feb 28, 2017

adds validation of header names (was completely absent)
due to nginx/openresty config the '_' is also considered an invalid
character.

Issues resolved

Fixes #2013

fixes #2013 adds validation of header names (was completely absent)
due to nginx/openresty config the '_' is also considered an invalid
character.
@thibaultcha
Copy link
Member

Really don't like the actual implementation and dumping it in utils, but I guess it'll do the job.

@Tieske
Copy link
Member Author

Tieske commented Mar 1, 2017

@thibaultcha

utils is the place for re-usable utility functions no? where else to put it?

What specifically don't you like about the implementation?

@thibaultcha
Copy link
Member

thibaultcha commented Mar 1, 2017

So far this code is not used anywhere but in the key-auth schema validation. Therefore, it has no reason for landing in utils by default.

The implementation, aside from not adhering to the conventional code style discussed many times, uses string.match instead of the PCRE ngx_lua API.

Finally, I don't think the pattern is even relevant. Here is a quote from the Nginx documentation:

Valid names are composed of English letters, digits, hyphens, and possibly underscores (as controlled by the underscores_in_headers directive).

letter + digits + hyphens + [underscores]. If we want "invalid" header names, we should then tweak the ignore_invalid_headers directive, which would make this PR irrelevant. If we don't want to (and we probably don't), then this pattern - or ideally, regex - should comply to it.

We're not writing PUC Lua. We're writing LuaJIT in Nginx. I think we should apply ngx_lua idioms and not PUC Lua ones.

@Tieske
Copy link
Member Author

Tieske commented Mar 1, 2017

So far this code is not used anywhere but in the key-auth schema validation. Therefore, it has no reason for landing in utils by default.

If it does not land in utils then no one will find it the next time it is needed, and hence will reinvent the wheel. The pure generic-header-validation logic nature makes it a utility function by default. Hence utils seems the better fit imo.

The implementation, aside from not adhering to the conventional code style discussed many times, uses string.match instead of the PCRE ngx_lua API.

Will update. But it is like this in many places where performance is non-critical. So update all matching to PCRE everywhere? through leave-behind-better-than-found policy?

Finally, I don't think the pattern is even relevant. Here is a quote from the Nginx documentation:

Valid names are composed of English letters, digits, hyphens, and possibly underscores (as controlled by the underscores_in_headers directive).

letter + digits + hyphens + [underscores]. If we want "invalid" header names, we should then tweak the ignore_invalid_headers directive, which would make this PR irrelevant. If we don't want to (and we probably don't), then this pattern - or ideally, regex - should comply to it.

hmmm, didn't look there, just implemented the rfc7230 with the additional _ exception.

But I agree to the more restrictive nginx implementation over tweaking the directive.

* switched to nginx validation instead of rfc
* using PCRE instead of build in matching
* added whitespace
@Tieske
Copy link
Member Author

Tieske commented Mar 2, 2017

@thibaultcha updated style, PCRE and validation restrictions

@Tieske Tieske added this to the 0.10 RC milestone Mar 2, 2017
@thibaultcha
Copy link
Member

Manually merged with the correct commit grouping, naming and some style modifications.

@thibaultcha thibaultcha closed this Mar 6, 2017
@thibaultcha thibaultcha deleted the fix/header-name branch March 6, 2017 19:28
thibaultcha added a commit that referenced this pull request Apr 10, 2017
There is no incentive for Kong to not allow a header name containing an
underscore. This reverts the change in behavior introduced in #2142
and 73437ef.

The `underscores_in_headers` directive seems to already be enabled.
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.

2 participants