Skip to content

2FA authentication - #322

Open
alexandregz wants to merge 6 commits into
postfixadmin:masterfrom
alexandregz:master
Open

2FA authentication#322
alexandregz wants to merge 6 commits into
postfixadmin:masterfrom
alexandregz:master

Conversation

@alexandregz

Copy link
Copy Markdown
Contributor

2-factor authentication, only for admins.

$struct['x_2fa_qrcode'] = pacol(0, 0, 1, 'html', '2FA_qr_code', '2FA_qr_code_onthefly', '', array(),
/*not_in_db*/ 0,
/*dont_write_to_db*/ 0,
'IF(x_2fa_secret > "", CONCAT("<img src=\"'.$urlQRCode[0].'", REPLACE(username, "@", "%40"), "'.$urlQRCode[1].'", x_2fa_secret, "'.$urlQRCode[2].$urlQRCode[3].'\">"), "") AS x_2fa_qrcode'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This leaks the quite a lot of information to a seemingly random third party (the QR code API provider):

  • the email address of the user
  • the 2FA secret of the user
  • the location of the postfixadmin instance through the HTTP Refer[r]er header (depending on the referrer policy)

In addition, it allows the third party (assuming they became malicious) to deliver back arbitrary content. Think some of the weird things you can do if you return SVGs instead of PNGs.

Even if it turns out that this case is immune to such attacks, this simply makes the site admin a prime phishing target (even admins aren't 100% immune to social engineering)

And even if this were not an issue either, this introduces an implicit dependency on a third party service the user of the software is not even necessarily aware of.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

To be a bit more constructive: I'd also like to see 2FA, but maybe it'd be better to generate the QR code locally using something like phpqrcode or qrcode.js

(PS: I'm not a postfixadmin maintainer, just a user who's excited about the option to get 2FA in postfixadmin)

Comment thread public/2fa.php
if($ga->verifyCode( $row['x_2fa_secret'], $fCode, 2)) { // 2 = 2*30sec clock tolerance

// ---- ToDo need some implementation in functions.php and login.php :-(
// // set cookie don't ask for 2fa next 30 days

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this implies I won't be asked for 2fa if I set a cookie? Doesn't this feel a little insecure?

ideally wouldn't you want something on the user db record (e.g. a timestamp field which if not-null is the time of next checking?)

Comment thread templates/2fa.tpl
<!--
<tr>
<td class="label"><label>{$PALANG.2FA_form_30days_authentication}:</label></td>
<td><input type="checkbox" value="1" name="f30DaysRemember"/</td>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

your html is missing a > here

@svenseeberg

Copy link
Copy Markdown
Contributor

There is now another PR that aims at providing similar (and more) functionality: #753

@Neustradamus

Copy link
Copy Markdown

To follow

@winicius87

Copy link
Copy Markdown

2FA with TOTP should be easier to implement with one POST request that has the fields username, password, and totp.

After receiving the username and password, hide the element and show the totp input form. This way it looks like the user is sending two POST requests to the server.

@Neustradamus

Copy link
Copy Markdown

@DavidGoodwin: Why have you closed it?

@DavidGoodwin

Copy link
Copy Markdown
Member

Sorry, I think I made a mistake.

@DavidGoodwin DavidGoodwin reopened this Jul 29, 2024
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.

6 participants