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(basic-auth) accept passwords containing ':' #3014

Closed
wants to merge 2 commits into from
Closed

fix(basic-auth) accept passwords containing ':' #3014

wants to merge 2 commits into from

Conversation

nico-acidtango
Copy link
Contributor

Summary

When a password contains one or more ':' characters, the password is
truncated up to the first ':' character. This is due to an error
while splitting the user:password string.

Full changelog

  • Modify split in basic-auth/access
  • Add test to check a password with ':' is accepted

Issues resolved

Fix #2986

@thibaultcha
Copy link
Member

Hi,

Thank you for the patch! We will try to review it as soon as we can manage some time aside. As a side thought: maybe this could land in the master branch instead? Or do you see any breaking behavior compared to the current behavior of the basic-auth plugin?

@nico-acidtango
Copy link
Contributor Author

@thibaultcha there is no breaking behaviour, so it may perfectly be moved to master. How should I proceed?

@Tieske Tieske changed the base branch from next to master November 9, 2017 10:53
@Tieske
Copy link
Member

Tieske commented Nov 9, 2017

@nico-acidtango you can rebase your branch on master using the git command line: git rebase master

I already changed this PR to target master. As you can see above, a lot of unrelated commits (from next) are now in here. After your rebase those should be gone.

When a password contains one or more ':' characters, the password is
truncated up to the first ':' character. This is due to an error
while splitting the user:password string.

Fix #2986
@nico-acidtango
Copy link
Contributor Author

@Tieske thanks for the advise, its done now

@@ -39,7 +39,7 @@ local function retrieve_credentials(request, header_name, conf)
if m and m[1] then
local decoded_basic = ngx.decode_base64(m[1])
if decoded_basic then
local basic_parts = utils.split(decoded_basic, ":")
local basic_parts = utils.split(decoded_basic, ":", 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

A thought while we're visiting this code: this pure Lua split function from penlight is not JITable, which is a (small) concern here on a hot path. Should we consider migrating this to a JITable ngx.re.match implementation?

Copy link
Member

Choose a reason for hiding this comment

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

@p0pr0ck5 good catch! that would definitely be an improvement to make here while we're at it.

Copy link
Member

Choose a reason for hiding this comment

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

Thought as much, but not really the scope of this PR, as we try to generally enforce. I'd personally advise making that a subsequent perf contribution. Unless @nico-acidtango feels comfortable making further updates, this fulfills the premise of the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. My thought was we would end up doing this ourselves anyway, so we could encourage the community to give it a go (in a separate commit in this PR). But I do agree in its current form this is 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem guys, I'll change it to use ngx.re.match

local basic_parts = utils.split(decoded_basic, ":")
username = basic_parts[1]
password = basic_parts[2]
local basic_parts, err = ngx.re.match(decoded_basic, "([^:]+):(.+)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks! A few brief thoughts: we may want to localize the "ngx.re.match" function, and we will want to use the "oj" flags as the third param in this call to leverage PCRE JITing and cache the compiled expression. Thanks again!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I'll apply those changes too

Better performance in username:password string splitting by using
JITable ngx.re.match implementation instead of pure Lua split function
from penlight
@p0pr0ck5
Copy link
Contributor

p0pr0ck5 commented Dec 5, 2017

Apart from a few pedantic things that can be fixed at merge this LGTM. @thibaultcha @Tieske any objections to merging?

@thibaultcha
Copy link
Member

Sorry for the delay, merged to master with some minor edits!

@nico-acidtango Thank you for the patch!

@thibaultcha thibaultcha closed this Dec 5, 2017
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.

Error in basic-auth authorization
4 participants