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

Move to hashed passwords for the web interface #10157

Merged
merged 41 commits into from
Sep 17, 2021

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Mar 9, 2021

Short description

It is not very good practice to keep the password in plaintext in the configuration file as well as in memory at runtime.

Fixes #7937.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

Copy link
Member

@Habbie Habbie left a comment

Choose a reason for hiding this comment

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

One note, one text suggestion, definitely approved!

pdns/dnsdist-lua.cc Show resolved Hide resolved
pdns/dnsdistdist/docs/reference/config.rst Outdated Show resolved Hide resolved
@rgacogne
Copy link
Member Author

Otto, Peter, shall I try to extend that PR to also include the authoritative server and the recursor, at least when sodium support is available? I was thinking of adding password-hashing capabilities to pdnsutil and rec_control for that.

@Habbie
Copy link
Member

Habbie commented Mar 24, 2021

Yes please!

@omoerbeek
Copy link
Member

That would be wonderful!

@Habbie
Copy link
Member

Habbie commented Mar 24, 2021

rec_control

Bit weird to have a 'client side' function in rec_control, but the recent FD passing support for dump-* at least offers some precedent for rec_control being more than a simple socket command client :)

@rgacogne rgacogne changed the title dnsdist: Move to hashed passwords for the web interface Move to hashed passwords for the web interface Mar 31, 2021
@rgacogne
Copy link
Member Author

Rebased to fix a conflict plus:

  • automatically detect whether the password is hashed instead of using a separate parameter, moving to a separate class
  • use that class in the auth and rec
  • add hashing support to pdnsutil and rec_control
  • add unit and speed tests

The auth and rec parts need more testing, doing that now.

@rgacogne
Copy link
Member Author

We now also try to do credentials verification in constant time even without libsodium, by computing a random seed, hashing the credentials found in the configuration, then later hashing the credentials supplied by the user using the same seed, and comparing the password only if the hashes match.

@rgacogne
Copy link
Member Author

The auth and rec parts need more testing, doing that now.

Looks good!

Copy link
Contributor

@pieterlexis pieterlexis left a comment

Choose a reason for hiding this comment

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

It would be good to add libsodium to the auth and rec packages as well (adding a sodium BuildRequires for RPM and adding it in Build-Depends in the debbian control file should to the trick if configure checks for sodium).

@rgacogne
Copy link
Member Author

Don't we already build with libsodium for ed25519 DNSSEC support?

@Habbie
Copy link
Member

Habbie commented Mar 31, 2021

Don't we already build with libsodium for ed25519 DNSSEC support?

Yep, all our package builds already have libsodium!

@pieterlexis
Copy link
Contributor

Don't we already build with libsodium for ed25519 DNSSEC support?

we do, excellent :)

@rgacogne rgacogne force-pushed the ddist-hashed-passwords branch 2 times, most recently from 1555d8b to a8b7ee7 Compare March 31, 2021 11:43
@rgacogne
Copy link
Member Author

The test-auth-regress-gpgsql failure seems real (occurred twice already) but I don't see any useful output in the log. I'll have a look later.

@Habbie
Copy link
Member

Habbie commented Mar 31, 2021

I also saw a few (different) gsql failures today that I could not immediately explain..

@zeha
Copy link
Collaborator

zeha commented Mar 31, 2021

Q (feel free to ignore): is there a good reason to build without libsodium any longer?

@rgacogne
Copy link
Member Author

Q (feel free to ignore): is there a good reason to build without libsodium any longer?

Perhaps on some systems where it's not readily available? On the other hand it's packaged in most distros nowadays.

@mind04
Copy link
Contributor

mind04 commented Mar 31, 2021

el7 and el8 do not have libsodium out of the box

@pieterlexis
Copy link
Contributor

el7 and el8 do not have libsodium out of the box

fwiw: our package builds pull this in via EPEL.

@rgacogne
Copy link
Member Author

rgacogne commented Apr 6, 2021

Rebased to fix a conflict.

@Habbie Habbie self-requested a review April 8, 2021 10:19
Copy link
Member

@Habbie Habbie left a comment

Choose a reason for hiding this comment

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

Some comments, LGTM really!

pdns/dnsdistdist/Makefile.am Show resolved Hide resolved
pdns/dnsdistdist/Makefile.am Show resolved Hide resolved
pdns/dnsdist-lua.cc Outdated Show resolved Hide resolved
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
pdns/rec_control.cc Outdated Show resolved Hide resolved
@rgacogne
Copy link
Member Author

I'm merging this PR now even though a nice improvement was suggested in the comments, but the improvement would take a non-negligible amount of work and this PR already improves the situation enough that I would like to have it in dnsdist 1.7.0.

@rgacogne rgacogne merged commit c233a60 into PowerDNS:master Sep 17, 2021
@rgacogne rgacogne deleted the ddist-hashed-passwords branch September 17, 2021 08:03
@rgacogne rgacogne mentioned this pull request Sep 20, 2021
7 tasks
rgacogne added a commit to rgacogne/pdns that referenced this pull request Oct 7, 2021
… ChangeLog

As suggested by Denis Machard on the mailing-list (thanks!).
rgacogne added a commit that referenced this pull request Oct 13, 2021
…grade

dnsdist: Add #10157 to the upgrade guide and the 1.7.0-alpha1 ChangeLog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: store authentication verifier in hashed format
10 participants