HPCC-31574 Add option in Dali LDAP support to ignore default file user #18503
Conversation
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-31574 Jirabot Action Result: |
dali/server/daldap.cpp
Outdated
| logNullUser(nullptr); | ||
| } | ||
|
|
||
| Owned<ISecUser> user = ldapsecurity->createUser(username); | ||
| user->setAuthenticateStatus(AS_AUTHENTICATED); | ||
| user->credentials().setPassword(password); | ||
| // user->setAuthenticateStatus(AS_AUTHENTICATED); let normal user authentication take place |
There was a problem hiding this comment.
I don't think this should be changed.
It was deliberate to treat calls to Dali getPermissions as from a trusted user.
See : https://hpccsystems.atlassian.net/browse/HPCC-22415
There was a problem hiding this comment.
I made an update that sets a passed in user as authenticated, but forces authentication for the default user. Otherwise any user/pw combination would have access in these cases.
There was a problem hiding this comment.
@kenrowland - looks good, but the user->setAuthenticateStatus(AS_AUTHENTICATED) was deliberate/shouldn't be changed in this PR.
dali/server/daldap.cpp
Outdated
| username.append(filesdefaultuser); | ||
| decrypt(password, filesdefaultpassword); | ||
| OWARNLOG("Missing credentials, injecting deprecated filesdefaultuser for request %s %s", key, nullText(obj)); | ||
| OWARNLOG("Missing credentials, injecting deprecated filesdefaultuser (%s) for request %s %s", filesdefaultuser.str(), key, | ||
| nullText(obj)); | ||
| logNullUser(nullptr); |
There was a problem hiding this comment.
minor: should this be above the if (disabledFileDefaultUser) so still useful when option on?
Also, I think it is already called before each call to this function in 1 context (in getPermissionsLDAP), that call should prob. be removed and just have 1 here. Not that it will make much difference as it only logs once per minute anyway.
There was a problem hiding this comment.
I moved the logNullUser above the disable check. It would continue to be a good idea to have the information while we are analyzing each case. I did see it's called before this method is called as well. I am going assess what logging is best and make appropriate changes. Right now, more is probably better, and at only once per minute we should be OK until those changes are made.
Added option to disable use of default user. Deny access if no user provided and default user is not defined. Do not automatically set user to authenticated. Signed-Off-By: Kenneth Rowland [email protected]
|
@kenrowland - please squash |
|
@ghalliday Squashed and ready for merge. |
| @@ -115,29 +117,36 @@ class CDaliLdapConnection: implements IDaliLdapConnection, public CInterface | |||
| return SecAccess_Full; | |||
|
|
|||
|
|
|||
| Owned<ISecUser> user; | |||
| StringBuffer username; | |||
| StringBuffer password; | |||
| if (udesc) | |||
| { | |||
| udesc->getUserName(username); | |||
| udesc->getPassword(password); | |||
There was a problem hiding this comment.
minor: password is not used by the rest of the function.
| OWARNLOG("Missing credentials, injecting deprecated filesdefaultuser (%s) for request %s %s", filesdefaultuser.str(), key, | ||
| nullText(obj)); | ||
| user.setown(ldapsecurity->createUser(username)); | ||
| user->credentials().setPassword(password); // Force authentication of default user when used |
There was a problem hiding this comment.
Is this really needed? What is it protecting against? The downside of using it is that the password needs to be included in the helm configuration - which is not secure.
There was a problem hiding this comment.
The default files user is not currently included in our helm charts, so both the username and password are empty.
|
This change has the implication of essentially forcing containerized deployments to always provide a user. Currently, the default user is not included in our helm charts which results in an empty username. Since that user for all intents and purposes has no permissions granted, access falls back to default file permissions. The TFE cluster was operating with a setting that allowed all access to any authenticated user. Since the user was marked as authenticated (the empty user) all scope requests were granted. IMO this is clearly not how it should operate. This change denies access if no user is provided and either no default user is defined or the flag disabling this support is set. W/o changes to fix all null users or adding the default user to the helm chart, once this is merged, access requests w/o a user will be denied. |
ghalliday
merged commit 21ba84c
into
hpcc-systems:candidate-9.6.x
Added option to disable use of default user.
Deny access if no user provided and default user is not defined.
Do not automatically set user to authenticated.
Signed-Off-By: Kenneth Rowland [email protected]
Type of change:
Checklist:
Smoketest:
Testing: