kuzzleio / kuzzle-plugin-auth-passport-local

Provide local authentication with username/password for Kuzzle
Apache License 2.0
0 stars 2 forks source link

Implement search method #76

Closed MathieuVeber closed 3 years ago

MathieuVeber commented 3 years ago

Closes #75

What does this PR do?

Adds a method that can retrieve users from an ES query about their username. This method has to watch both input and output. On one hand the user should be able to search ONLY with username and on the other hand the user results are sensitive informations so this method will only return kuid.

How should this be manually tested?

Start a Kuzzle and use the action security:searchUsersByCredentials with local strategy.

Additional context

Check this PR which has an example, some documentation and more concerning this feature and the linked Kuzzle main action.

Additional safety considerations

This search method require more security than usual and needs at the same time a whitelist and a blacklist:

ES keywords are a pain point because we are dependent to what ES decide in the future so we need to keep an eye on them and update the blacklist if necessary. To decrease the security concerns, I have set up a second safety feature which has its limits but does better than nothing in case a new ES keyword comes out of nowhere.

Aschen commented 3 years ago

As discussed earlier, I was not sure the plugin should be adding query 'security' to the userRepository.search it doesn't look that solid. And upon further inspection, this actually exposed an underlying security issue that needs to be corrected.

IMHO specific logic used only in the case of a search on local credentials should be implemented here (protection again multi-match, ensure only username is used)

About the security issue, we should definitely address it directly in Kuzzle Core

MathieuVeber commented 3 years ago

Let's summarize: This search method require more security than usual. However, this has brought another issue to light which has to be fixed in the core (issue)

Considering the latter handled, this feature needs a whitelist and a blacklist to be safe:

ES keywords are a pain point because we are dependent to what ES decide in the future so we need to keep an eye on them and update the blacklist if necessary. To decrease the security concerns, I have set up a second safety feature which has its limits but does better than nothing in case a new ES keyword comes out of nowhere.