Add a query to verify a user's password (fixes #6837)

[smunier01]

Required for all non-trivial PRs

• Rebased/mergable
• Tests pass
• CHANGELOG.md updated
• Sign CLA (if not already signed)

Required only if applicable

• InfluxQL Spec updated

As mentioned in #6837, I had already implemented something similar for my personal use, so if it can help, here is the pull request with my solution to the problem.

If you don't like the syntax, or think there is a better way to do this feature, feel free to tell me about it, I would happily try to do something better.

Syntax proposed

SHOW USER "user" WITH PASSWORD 'pwd' will return the user if the password is correct.

SHOW USER "user" will return the user. (might not be very useful, but I think it makes sense to have it

[e-dard]

Highlighting this for other members of the team to think about—should SHOW USERS be an admin-only feature?

[jsternberg]

SHOW USERS should probably be an admin-only feature. At best, SHOW USERS might be able to show yourself and you alone, but we don't currently have a way to do that.

I guess it's possible to try and make public and private users, but I'm not sure that's worth it.

[jsternberg]

This still needs to be restricted to admin-only.

[e-dard]

LGTM 👍 @smunier01 other people on the team will need to review this and agree that we should accept the feature. Thanks for the effort here!

[jsternberg]

I personally haven't figured out if I like this syntax or not, but I also haven't thought of an appropriate alternative syntax. So in the absence of an alternative syntax, I do have one comment about the proposed syntax.

SHOW USER <name> looks fine.

For SHOW USER <name> WITH PASSWORD <password> can you have it always return a value if the user exists? Include a third column for this one such as a password column (maybe a better name, but I'm drawing a blank) that says true or false depending on if the password was correct or not. This will allow an automation system to acknowledge whether it has to call SET PASSWORD or CREATE USER with only a single call.

Can you also add a test case to influxql/sanitize_test.go to ensure that the password within this command is sanitized and update the regexp name from sanitizeCreatePassword to sanitizeWithPassword?

[smunier01]

I also couldn't find anything I liked for the name of the third column. I settled on password_match. It's not pretty, but at least it's explicit.

[jsternberg]

It looks like there is a function called Authenticate which does the same thing as this. Will that work instead of adding a new function?

[smunier01]

At the time I was not sure if the new query should update the auth cache or not (like Authenticate does), but I don't see any reason why not now.

edit: Actually Authenticate doesn't return the user when the password is false. So unless you want to change the unit tests on Authenticate, I have to use a function that does (I put the code of Authenticate inside UserWithPassword and made it call it so that there is no code duplication).

[jsternberg]

I don't really want to add another method if possible, so I think it would be better to just call User and then try to call Authenticate with the username and check the result. That way we can avoid a second function that needs to be maintained. You can also do something like this:

u, err := e.MetaClient.Authenticate(user, password)
if err == ErrUserNotFound {
    return err
} else if u == nil {
    u, err = e.MetaClient.User(user)
    if err != nil {
        return err
   }
}
// do stuff with the user

[jsternberg]

This looks fine for the most part. I think if you just squash the commits with rebase we can probably get this merged.

@jwilder @e-dard any opinions about this?

[jwilder]

I have several concerns about this feature:

• There is a potential security issue that this could create that would allow any user to attempt to crack another users password simply by having query access.
• I think it's confusing to always return a user when with password does not match. The with password implies a constraint that needs to be matched.
• We have show users already and this adds a show user. Seems like they could be could be unified better.

I believe this functionality already exists via the create user foo with password 'password' statement. If you run that and the user and password already match, no error is returned. If the users exists, but with a different password, you get a ERR: user already exists

[jsternberg]

I think you're right that we can just use CREATE USER for this workflow, but I disagree with your first statement. We already have the ability for a person to attempt to crack another user's password simply by having query access. They can just try the username and password with a query and check to see if it returns a 401 or something else, so I don't think that's a major concern.

@smunier01 how would using CREATE USER WITH PASSWORD, checking the error condition, and then calling SET PASSWORD if that fails work for your use case? I think it should allow something like Ansible to set the password without having to add anything new to the query language.

[smunier01]

For my use case it would indeed be enough, but I can't speak for the person who did the initial feature request.

Although, if it was my software, I would dislike having functionalities covered by semantically incorrect ways (create user should only be used to create a user) and unless you document the use case properly, people are not going to know it is possible.

[jsternberg]

I'm going to close this then. @rkuchan can you verify that this usage of CREATE USER is documented by providing an example of verifying and updating a user's password in the documentation? Thanks.