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

Openvpn authentication with otp as suffix #8058

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Openvpn authentication with otp as suffix #8058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4eaefa0
Handle Authentication with TOTP in reverse
Nov 13, 2024
90168b2
Merge branch 'opnsense:master' into openvpn-auth-otp-as-suffix
tsdogs Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/opnsense/mvc/app/library/OPNsense/Auth/TOTP.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ trait TOTP
/**
* @var bool token after password
*/
private $passwordFirst = false;
public $passwordFirst = false;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the problem. This looks like a workaround for a different issue.

Copy link
Author

Choose a reason for hiding this comment

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

This is needed to know if the authenticator has the pin as suffix.
There's a readonly option but then it needs to be initialized in the creator of the class.


/**
* use graceperiod and timeWindow to calculate which moments in time we should check
Expand Down
19 changes: 13 additions & 6 deletions src/opnsense/scripts/openvpn/user_pass_verify.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,16 +80,15 @@ function do_auth($common_name, $serverid, $method, $auth_file)
if (empty($username) || empty($password)) {
return "username or password missing ({$method} - {$auth_file})";
}
$pinpassword = false;
$pin = false;
if (strpos($password, 'SCRV1:') === 0) {
// static-challenge https://github.com/OpenVPN/openvpn/blob/v2.4.7/doc/management-notes.txt#L1146
// validate and concat password into our default pin+password
// validate and split password into pin and password (used below based on authenticato settings)
$tmp = explode(':', $password);
if (count($tmp) == 3) {
$pass = base64_decode($tmp[1]);
$pinpassword = base64_decode($tmp[1]);
Copy link
Member

Choose a reason for hiding this comment

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

all of this just obscures the intention...

Copy link
Author

Choose a reason for hiding this comment

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

We might move this also inside the authenticator loop for clarity, just it didn't seem programmatically correct to me.

$pin = base64_decode($tmp[2]);
if ($pass !== false && $pin !== false) {
$password = $pin . $pass;
}
}
}
$a_server = $serverid !== null ? (new OPNsense\OpenVPN\OpenVPN())->getInstanceById($serverid, 'server') : null;
Expand Down Expand Up @@ -121,7 +120,15 @@ function do_auth($common_name, $serverid, $method, $auth_file)
foreach (explode(',', $a_server['authmode']) as $authName) {
$authenticator = $authFactory->get($authName);
if ($authenticator) {
if ($authenticator->authenticate($username, $password)) {
$checkpassword = $password;
if ($pin!=false) {
if (property_exists($authenticator,'passwordFirst') && $authenticator->passwordFirst) {
$checkpassword = $pinpassword . $pin;
} else {
$checkpassword = $pin . $pinpassword;
}
}
if ($authenticator->authenticate($username, $checkpassword)) {
Copy link
Member

@fichtner fichtner Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

if we had a third argument for the pin this might be possible to handle gracefully inside the authenticator but I am not sure of the benefit of all of this CC @AdSchellevis

Copy link
Member

Choose a reason for hiding this comment

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

we shouldn't do this, you can configure openvpn to ask for the pin separately, see also #3290

Copy link
Author

Choose a reason for hiding this comment

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

I thought also about that, but it would impact more code. If needed I might just change the Auth backends to accept the pin.

Copy link
Author

Choose a reason for hiding this comment

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

About #3290 that's exactly the point. Enabling the pin request does not handle the reverse option enabled.

Copy link
Author

Choose a reason for hiding this comment

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

Should we maybe move the "SCRV1:" handling code inside the authenticator itself?

Copy link
Member

Choose a reason for hiding this comment

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

easiest option is just to add 2 authenticators if you need both orders, use one for openvpn, the other for the ui. I don't think we need more glue to be honest, a note in the documentation about limitations should be more than enough.

// fetch or create client specific override
$common_name = empty($a_server['cso_login_matching']) ? $common_name : $username;
syslog(
Expand Down