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.

Sign up for GitHub

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

Jump to bottom

passwords for users created via graph is stored unhashed in idm #3778

Closed
rhafer opened this issue May 12, 2022 · 5 comments · Fixed by #4053
Closed

passwords for users created via graph is stored unhashed in idm #3778

rhafer opened this issue May 12, 2022 · 5 comments · Fixed by #4053
Assignees
@rhafer
Labels
Priority:p1-urgent Consider a hotfix release with only that fix Topic:Security Type:Bug

Comments

@rhafer
Copy link
Contributor

rhafer commented May 12, 2022

Describe the bug

Currently the user passwords for users created via graph are stored in cleartext in libregraph/idm (for other LDAP implementation this depends a bit on the specific configuration). We could either:

  • add (LDAP) client side hashing, this would mean we need know about the hashes supported on the client side
  • rely on server side hashing (teach IDM to hash passwords), and e.g. use the LDAP Modifiy Password extension when talking to servers that support it.

Note: the userpassword for the user admin and all service users are already hashed.
@C0rby
Copy link
Contributor

C0rby commented Jun 8, 2022

@micbar, IMO this should have a higher priority.

@micbar micbar added the Priority:p1-urgent Consider a hotfix release with only that fix label Jun 14, 2022
@rhafer rhafer self-assigned this Jun 15, 2022
@C0rby
Copy link
Contributor

C0rby commented Jun 15, 2022 •
edited
Loading

rely on server side hashing (teach IDM to hash passwords)

I think this would be the clean solution. Where would we add the hashing? To the ldapserver or to the handlers (boltdb, etc...)?
@rhafer

@rhafer
Copy link
Contributor Author

rhafer commented Jun 15, 2022

rely on server side hashing (teach IDM to hash passwords)

I think this would be the clean solution. Where would we add the hashing? To the ldapserver or to the handlers (boltdb, etc...)? @rhafer

If it just where about idm I would lean towards adding it to the ldapserver at the top-level (i.e. https://github.com/libregraph/idm/blob/master/pkg/ldapserver/add.go#L12 and https://github.com/libregraph/idm/blob/master/pkg/ldapserver/modify.go#L13)

It's more difficult however as we also want to support external LDAP servers. OpenLDAP e.g. does not hash the password when add or modified via the standard LDAP operations (mainly to stay compatible with the LDAP RFC). For OpenLDAP it would makes sense set the password via the LDAP Password Modify Extended Operation sigh. Other LDAP servers (e.g. 389DS) do hash the password received via ADD/MODIFY requests though.

The most compatible approach might be to add LDAP Password Modify Extended Operation to idm and always use that operation to set passwords. I haven't checked how hard this is yet. But I think it shouldn't be too much work (famous last words ...)

@C0rby
Copy link
Contributor

C0rby commented Jun 15, 2022

It's more difficult however as we also want to support external LDAP servers. OpenLDAP e.g. does not hash the password when add or modified via the standard LDAP operations (mainly to stay compatible with the LDAP RFC). For OpenLDAP it would makes sense set the password via the LDAP Password Modify Extended Operation sigh. Other LDAP servers (e.g. 389DS) do hash the password received via ADD/MODIFY requests though.

I see, that is news to me.

The most compatible approach might be to add LDAP Password Modify Extended Operation to idm and always use that operation to set passwords. I haven't checked how hard this is yet. But I think it shouldn't be too much work (famous last words ...)

Yes, this makes sense. I can take a look into that next week.

@rhafer
Copy link
Contributor Author

rhafer commented Jun 16, 2022

Yes, this makes sense. I can take a look into that next week.

Oh. I already started some ground work on that. (Sorry, I saw this a bit too late. Didn't want to step on your toes). See https://github.com/rhafer/idm/tree/pwexop

If you want we can pair on this on monday.

rhafer added a commit to rhafer/ocis that referenced this issue Jun 28, 2022
@rhafer
Use LDAP Modify Password ExtOp for updating passwords
1c7dcf3 
By default the graph API will now use the LDAP Password Modify Extended
Operation for setting user passwords. By this we make sure that the
LDAP server can e.g. properly hash the password with and algorithm that
it supports.

This can be reverted to the old behaviour (using "normal" LDAP modify
requests) by setting GRAPH_LDAP_SERVER_USE_PASSWORD_MODIFY_EXOP=false

Fixes: owncloud#3778
@rhafer rhafer mentioned this issue Jun 28, 2022
Merged
rhafer added a commit to rhafer/ocis that referenced this issue Jun 28, 2022
@rhafer
Use LDAP Modify Password ExtOp for updating passwords
38c6ec2 
By default the graph API will now use the LDAP Password Modify Extended
Operation for setting user passwords. By this we make sure that the
LDAP server can e.g. properly hash the password with and algorithm that
it supports.

This can be reverted to the old behaviour (using "normal" LDAP modify
requests) by setting GRAPH_LDAP_SERVER_USE_PASSWORD_MODIFY_EXOP=false

Fixes: owncloud#3778
rhafer added a commit that referenced this issue Jun 30, 2022
@rhafer
Use LDAP Modify Password ExtOp for updating passwords
2cabf5b 
By default the graph API will now use the LDAP Password Modify Extended
Operation for setting user passwords. By this we make sure that the
LDAP server can e.g. properly hash the password with and algorithm that
it supports.

This can be reverted to the old behaviour (using "normal" LDAP modify
requests) by setting GRAPH_LDAP_SERVER_USE_PASSWORD_MODIFY_EXOP=false

Fixes: #3778
@micbar micbar mentioned this issue Jul 19, 2022
Closed
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rhafer rhafer

Labels
Priority:p1-urgent Consider a hotfix release with only that fix Topic:Security Type:Bug
Projects
Infinite Scale Team Board
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use LDAP Modify Password ExtOp for updating passwords rhafer/ocis
3 participants
@rhafer @micbar @C0rby