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

A more secure and generalized approach for PDO Basic Auth Backend #1284

Merged

Conversation

lightbluetom
Copy link
Contributor

@lightbluetom lightbluetom commented Jul 15, 2020

Hello,

motivated by the currently insecure password hashing in sabre/dav (as discussed in Baikal #514), I developed this PR. It allows administrators to choose any password hashing function supported by password_verify() (among others, this includes the state of the art hashes bcrypt and Argon).

Furthermore this PR is about generalized approach on using the PDO Backend with Basic Authentication. The supplied Backend allows the customization of:

  • tableName : table in which the user information is stored,
  • digestColumn : table column in which the digest/passoword hash is stored,
  • uuidColumn : table column in which the Unique User Identifier is stored
  • digestPrefix : if your user management Backend prefixes your digests, you can specify it so it will be removed before verfiying it.

I think this covers a large part of use cases and would benefit a lot of sabre/dav users.

Best Regards.

@lightbluetom lightbluetom force-pushed the general-approach-for-pdo-basic-auth branch 2 times, most recently from 7e77997 to 07a4b68 Compare July 15, 2020 16:48
Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

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

Please add a unit test - THX

@lightbluetom
Copy link
Contributor Author

lightbluetom commented Jul 18, 2020

@DeepDiver1975 added the requested Unit Tests.

All but one test failed and i can not pinpoint why this happens, because i cannot recreate this failed test locally. https://travis-ci.org/github/sabre-io/dav/jobs/709463652

Do you have any idea why this might be happening?

@codecov
Copy link

codecov bot commented Jul 18, 2020

Codecov Report

Merging #1284 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1284      +/-   ##
============================================
+ Coverage     97.12%   97.15%   +0.02%     
- Complexity     2774     2801      +27     
============================================
  Files           174      175       +1     
  Lines          8028     8142     +114     
============================================
+ Hits           7797     7910     +113     
- Misses          231      232       +1     
Impacted Files Coverage Δ Complexity Δ
lib/DAV/Auth/Backend/PDOBasicAuth.php 100.00% <100.00%> (ø) 9.00 <9.00> (?)
lib/DAV/Server.php 96.13% <0.00%> (-0.18%) 192.00% <0.00%> (ø%)
lib/CardDAV/Card.php 100.00% <0.00%> (ø) 18.00% <0.00%> (ø%)
lib/DAVACL/FS/File.php 100.00% <0.00%> (ø) 3.00% <0.00%> (ø%)
lib/DAV/Sync/Plugin.php 100.00% <0.00%> (ø) 26.00% <0.00%> (ø%)
lib/DAVACL/Principal.php 100.00% <0.00%> (ø) 17.00% <0.00%> (ø%)
lib/CalDAV/CalendarHome.php 100.00% <0.00%> (ø) 44.00% <0.00%> (+1.00%)
lib/CalDAV/CalendarObject.php 100.00% <0.00%> (ø) 19.00% <0.00%> (ø%)
lib/CalDAV/Schedule/Inbox.php 100.00% <0.00%> (ø) 10.00% <0.00%> (ø%)
lib/CalDAV/Schedule/Outbox.php 100.00% <0.00%> (ø) 5.00% <0.00%> (ø%)
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7101bbe...590e9dc. Read the comment docs.

@lightbluetom
Copy link
Contributor Author

lightbluetom commented Jul 18, 2020

Unit Test are now passing.

@lightbluetom
Copy link
Contributor Author

Hey @DeepDiver1975,

i just wanted to check on the progress of this PR and if by any chance it will be merged soon.
Or if you are too busy, should i ask someone else to Review this?

Best regards.

@lightbluetom
Copy link
Contributor Author

@staabm, @ByteHamster, could any one of you guys review this PR or give me some feedback?

I think @DeepDiver1975 is a bit too busy atm.

@Offerel
Copy link

Offerel commented Dec 13, 2021

Are there any chance, to include this into the release?

@staabm
Copy link
Member

staabm commented Dec 13, 2021

@phil-davis @DeepDiver1975 any opinions?

@DeepDiver1975 DeepDiver1975 merged commit 7ecb81d into sabre-io:master Dec 13, 2021
/**
* Creates the backend object.
*
* If the filename argument is passed in, it will parse out the specified file fist.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment does not make sense to me. I will make a comments-only PR to sort it out.

@Offerel
Copy link

Offerel commented Dec 14, 2021

Nice work and many thx for the new release. Some hints for setting this up:

First you should define a $options array in your server.php file like:

$options = array(
    'digestColumn'  => 'digest',
	'uuidColumn'	=> 'username',
	'tableName'		=> 'users',
	'digestPrefix'	=> ''
);

After that, you can use it with $authBackend = new \Sabre\DAV\Auth\Backend\PDOBasicAuth($pdo, $options);

The $options assume, that you have a table 'users' with a unique 'username' column and a column 'digest'. In 'digest', you can save your hashed passords, generated by the php function passord_hash(). Optionally, you can use a salt to hash your passord and/or use a digestPrefix. I dont know if this should be go up sometime in a small documentation. If not, the info is here. Feel free to correct the info i have found.

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.

None yet

6 participants