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

Add 2FA #416

Open
wants to merge 4 commits into
base: development
Choose a base branch
from
Open

Add 2FA #416

wants to merge 4 commits into from

Conversation

yubiuser
Copy link
Member

What does this PR aim to accomplish?:

Adds handling of 2 factor authentication to PADD


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

@yubiuser yubiuser mentioned this pull request Feb 13, 2025
1 task
@yubiuser yubiuser requested a review from a team February 13, 2025 21:41
@yubiuser
Copy link
Member Author

@PromoFaux
Copy link
Member

Do we need this? I thought the whole point of the app password was to bypass 2FA for API usage?

@yubiuser
Copy link
Member Author

Yes, the app password can bypass the 2FA, but users might not want to use it.

@mwoolweaver
Copy link

mwoolweaver commented Feb 14, 2025

this still asks for 2fa even if running PADD as sudo on pihole device directly. i was under the impression that #392 made it so that no password (2fa as well?) needs to be provided if running as root or as a user is a member the pihole group.

without --2fa

sudo ./padd.sh --server pi.hole

Screenshot From 2025-02-14 14-39-01

so --2fa becomes required (can pass any number, even if more than 6 digits long) regardless of who is running PADD

sudo ./padd.sh --server pi.hole --2fa 0

or from any device with app password from api

./padd.sh --server pi.hole --secret <app-password> --2fa 0

if using WebUI password, --2fa must be valid

@mwoolweaver mwoolweaver mentioned this pull request Feb 14, 2025
@yubiuser
Copy link
Member Author

I pushed a small change that skipps 2FA if a CLI password is read. Thanks for testing.

@mwoolweaver
Copy link

mwoolweaver commented Feb 15, 2025

tested with and without 2fa enabled on WebUI & running PADD on Pi-hole device and from other computers.

if 2FA is enabled, --2fa is only required when not using PADD from Pi-hole device as root, regardless of password that is provided as --secret.

if 2FA is not enabled on WebUI, PADD does not ask for second factor.

@yubiuser
Copy link
Member Author

I think this is fine now as it is. I don't want to make the code to complicate to check for the app password as well. If users know they use the app password, they can just enter any number in the 2FA field.

@mwoolweaver
Copy link

If users know they use the app password, they can just enter any number in the 2FA field.

should it be explained somewhere that when using the app password, the 2fa field can be any number?

or a better way to say it would be

--2fa only needs to be valid when using WebUI password.

@mwoolweaver mwoolweaver mentioned this pull request Feb 17, 2025
@yubiuser yubiuser mentioned this pull request Feb 18, 2025
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.

PADD does not properly reject failed authentication when 2FA is enabled on WebUI
3 participants