[TwoFactorAuth] Issue second factor challenge depending on the channel of access

[GitHubUser4234]

Hello,

This is a small additional feature I would like to contribute, it is about Two-Factor-Authentication. The goal is to issue the second factor challenge depending on the channel of access. For example only users accessing Nextcloud through the internet should be asked to enter their OTP, while users from the intranet only need to enter the usual username/password to gain access. There are quite a number of environments conceivable where such a feature could prove useful.

For this to work, a common request header is used to determine whether the second factor challenge should be skipped. The header name is proposed to be NC_2FA_SKIP and the expected value would be "true".

I recently discussed this with @ChristophWurst who was kind to point me to a starting point in the OC\Authentication\TwoFactorAuth\Manager class.

Once I'll get the code ready, I'll open a PR.

[ChristophWurst]

For this to work, a common request header is used to determine whether the second factor challenge should be skipped. The header name is proposed to be NC_2FA_SKIP and the expected value would be "true".

Who sets that header? The client or a proxy?

cc @LukasReschke @nextcloud/security

[LukasReschke]

If misconfigured this will easily allow it to bypass the two-factor auth… Not really a fan of that.

IP based white-lists: Fair enough. Header? Not so much.

Also preferably this should have some GUI and store into the appconfig in the DB and not the config.php file

[rullzer]

Header sounds way to dangerous.

Also how much of an issue is this. With remember me functionality you should not need your second factor that often.

[boppy]

IP based white-lists: Fair enough.

I would consider IP based white- or blacklist as some form of 2nd factor with its own app (i.e. twofactor_ipbased)! The pitfall is that the user has to select "IP Based 2FA" from the "2FA Verification Screen". IProvider::getDescription() might provide info or IProvider::isTwoFactorAuthEnabledForUser() might check if IP matches and would only return true if it matches anyway... But the second click has to be done by the user (if I understoof that right).

Header-based

No way! It's just like disabling 2FA, since it's open source in here ;)

[GitHubUser4234]

Header sounds way to dangerous.

No way! It's just like disabling 2FA, since it's open source in here ;)

Not really, with the correct security measures on the web server like overriding it with an empty value where OTP should be used, this is pretty solid. But sure this feature should only be enabled if these measurements are in place.

IP based white-lists: Fair enough. Header? Not so much.

Also preferably this should have some GUI and store into the appconfig in the DB and not the config.php file

@LukasReschke I really like your suggestion, it sounds perfect! In which particular spot of the GUI do you see this feature?

With remember me functionality you should not need your second factor that often

Some more background info: To force users coming from the internet to use 2FA as a security measure, the opt-out mode of @boppy 's PR discussed in here must be used. And then when you have a big company with lots of staff who are using Nextcloud only in their job from the intranet (in addition to other staff that might come from the internet), there might be difficulties to force all of them to install an OTP app on their private mobile, go through the hassle that comes with it, provide the support etc....So yes, this feature can prove useful :)

[GitHubUser4234]

@LukasReschke : Unfortunately it turned out that with the IP suggestion, there would be quite a number of obstacles for example when reverse proxies are used. Some might want to filter by client IP, some by proxy IP and some even have nested proxies where the correct proxy level would have to be configured. Any thoughts?

[boppy]

That's an issue for an (upcoming?) IP-2FA app not a general one on 2FA. I think it's not the right place to discuss that here. Nevertheless it's a fact to keep an eye on.

[GitHubUser4234]

That's an issue for an (upcoming?) IP-2FA app not a general one on 2FA. I think it's not the right place to discuss that here. Nevertheless it's a fact to keep an eye on.

Uhm, not sure about you mean. If it would be done at an app level, the user would be asked to choose a 2FA provider right? Without modifying the core, the actual goal to just login and get straight access can unlikely be achieved.

[boppy]

Anything that adds a 2nd factor would be an app (in my eyes). So that IP-based auth might just return false from IProvider::isTwoFactorAuthEnabledForUser(user) if the IP doesn't match. How it will check it, what field to use, what kind of filter, etc. is to be implemented in that very Interface...

The server has nothing to do with the negotiation itself.

PS: I wouldn't see a "straight access". You'll be directed to the 2FA-Page where the IProvider::getDisplayName might return "One Click Login" or "Direct Login (IP based)" oder something whereas IProvider::verifyChallenge returns true all the time.

[GitHubUser4234]

PS: I wouldn't see a "straight access". You'll be directed to the 2FA-Page [..]

Yeah, but that is an issue. It would be confusing and at least ugly if the "Direct login" option was shown in a network where it actually couldn't be used. Maybe one solution would be to in fact move the IP logic to an app and in addition enhance the framework to skip the 2FA screen when all available providers reply true on a IProvider::skipChallenge() call.

Any more helpful comments are welcome :)

[jsalatiel]

This is what i have done for my home server: i edited lib/Service/Totp.php

        private function isPrivateNetwork() {
                return !(filter_var($_SERVER['REMOTE_ADDR'], FILTER_VALIDATE_IP, FILTER_FLAG_IPV4| FILTER_FLAG_IPV6 
                                                                        | FILTER_FLAG_NO_PRIV_RANGE |  FILTER_FLAG_NO_RES_RANGE));
        }

        public function hasSecret(IUser $user) {
                try {
                        if ($this->isPrivateNetwork()) {
                                return false;
                        }
                        $this->secretMapper->getSecret($user);
                } catch (DoesNotExistException $ex) {
                        return false;
                }
                return true;
        }

it appears to be working. It may have security implications, but for my home server is OK. I would appreciate opinions =)

Of course it would be great the user interface to add IPs and/or FQDN that could be resolved on the fly.

[GitHubUser4234]

Hi @jsalatiel great to see another buddy showing interest in such feature :)

Thanks for the info, yeah, it looks like there are more than one spot where such logic could be applied in a quick & dirty manner. Another example would be this method in lib\private\Authentication\TwoFactorAuth\Manager.php:

     /**
     * Determine whether the user must provide a second factor challenge
     *
     * @param IUser $user
     * @return boolean
     */
    public function isTwoFactorAuthenticated(IUser $user) {..}

[jsalatiel]

HI @GitHubUser4234 , i think that your method is the right one, i just didn't find it when i was looking where to change :)

[jsalatiel]

Using owncloud with external otp plugin i had these options:

checkbox for "Disable OTP for private IPs" textbox for "Skip otp for (FQDN)"

it would be nice to see something like this in nextcloud

[My1]

Not really, with the correct security measures on the web server like overriding it with an empty value where OTP should be used, this is pretty solid. But sure this feature should only be enabled if these measurements are in place.

so basically the header is a REQUEST to not do TOTP, which the server can say okay or not?

honestly this Idea could also be used to resolve #2906 although in a different way. for example with a JWT-style cookie (or local data or whatever) a token for "I dont need 2FA, the user authorised me already" which the server could check (obviously this thing should be signed to avoid any changes in data) and validate and then ignore 2FA or if the validation fails, ax the stored token and do 2FA again.

[PelleHanses]

If you want to specify which subnets do not need two-step authentication, this function also works (based on the above-mentioned proposal).

File: lib/private/Authentication/TwoFactorAuth/Manager.php

[..]
/**
* Determine whether the user must provide a second factor challenge
*
* @param IUser $user
* @return boolean
*/
private function ipMatch() {
    $ip = $_SERVER['REMOTE_ADDR'];
    $cidrs = array("192.168.119.0/23", "192.168.18.0/23"); // In CIDR notation
    foreach((array) $cidrs as $cidr) {
        list($subnet, $mask) = explode('/', $cidr);
        if(((ip2long($ip) & ($mask = ~ ((1 << (32 - $mask)) - 1))) == (ip2long($subnet) & $mask))) {
            return true;
        }
    }
    return false;
}

public function isTwoFactorAuthenticated(IUser $user): bool {
    // Check if IP in range
    if ($this->ipMatch()) { return false; }

    if ($this->mandatoryTwoFactor->isEnforcedFor($user)) {
            return true;
    }
[..]

[Koky999]

@PelleHanses

Hi, tried your method on 22.2.3 and it isn't work. Internal server error after editing lib/private/Authentication/TwoFactorAuth/Manager.php with my IP range.

[My1]

No way! It's just like disabling 2FA, since it's open source in here ;)

maybe a header could be possible but instead of some fairly dumb "skip2fa=true" header, it could contain a big random secret value only known to the proxy (and to NC as a hash) which obviously would be random for each installation, that way open source wouldnt be an issue.

[luckym-boop]

This method works fine. I changed the code a bit.

private function ipMatch() { if(!isset($_SERVER['HTTP_X_REAL_IP']) || empty($_SERVER['HTTP_X_REAL_IP'])) { $ip = $_SERVER['REMOTE_ADDR']; } else { $ip = $_SERVER['HTTP_X_REAL_IP']; } $cidrs = array("10.200.0.0/13"); // In CIDR notation foreach((array) $cidrs as $cidr) { list($subnet, $mask) = explode('/', $cidr); if(((ip2long($ip) & ($mask = ~ ((1 << (32 - $mask)) - 1))) == (ip2long($subnet) & $mask))) { return true; } } return false; }

public function isTwoFactorAuthenticated(IUser $user): bool { // Check if IP in range if ($this->ipMatch()) { return false; }

if ($this->mandatoryTwoFactor->isEnforcedFor($user)) {
        return true;
}

[Koky999]

If you want to specify which subnets do not need two-step authentication, this function also works (based on the above-mentioned proposal).

File: lib/private/Authentication/TwoFactorAuth/Manager.php

[..]
/**
* Determine whether the user must provide a second factor challenge
*
* @param IUser $user
* @return boolean
*/
private function ipMatch() {
    $ip = $_SERVER['REMOTE_ADDR'];
    $cidrs = array("192.168.119.0/23", "192.168.18.0/23"); // In CIDR notation
    foreach((array) $cidrs as $cidr) {
        list($subnet, $mask) = explode('/', $cidr);
        if(((ip2long($ip) & ($mask = ~ ((1 << (32 - $mask)) - 1))) == (ip2long($subnet) & $mask))) {
            return true;
        }
    }
    return false;
}

public function isTwoFactorAuthenticated(IUser $user): bool {
    // Check if IP in range
    if ($this->ipMatch()) { return false; }

    if ($this->mandatoryTwoFactor->isEnforcedFor($user)) {
            return true;
    }
}
[..]

Method I used, quoted up, is working, but is wrong. There is missing end curly bracket at the end ( } ). Now it is working like a sharm! :-)

[silvan78]

I have allowed myself to modify the code, so the list is controlled from nextcloud/config/config.php:

/var/www/html/nextcloud/config/config.php

  'twofactor_whitelist_ips' =>
  array(
    "10.10.0.0/16" // LAN
  ),

/var/www/html/nextcloud/lib/private/Authentication/TwoFactorAuth/Manager.php

        /**
         * Determine whether the user must provide a second factor challenge
         *
         * @param IUser $user
         * @return boolean
         */
        private function ipMatch() {
            $ip = $_SERVER['REMOTE_ADDR'];
            $cidrs=$this->config->getSystemValue('twofactor_whitelist_ips', []);
            foreach((array) $cidrs as $cidr) {
                list($subnet, $mask) = explode('/', $cidr);
                if(((ip2long($ip) & ($mask = ~ ((1 << (32 - $mask)) - 1))) == (ip2long($subnet) & $mask))) {
                    return true;
                }
            }
            return false;
        }

        public function isTwoFactorAuthenticated(IUser $user): bool {

                // Check if IP in range
                if ($this->ipMatch()) { return false; }

                if (isset($this->userIsTwoFactorAuthenticated[$user->getUID()])) {
                        return $this->userIsTwoFactorAuthenticated[$user->getUID()];
                }

                if ($this->mandatoryTwoFactor->isEnforcedFor($user)) {
                        return true;
                }

Other idea is to modify additionally Set/GetState functions and some gui.

[AndyXheli]

Hi @ChristophWurst I wanted to see if there's any process on this. Something like this would be really nice i just implanted this for our Office 365 users that they dont need to use 2FA with ip whitelist this would really be a nice feature to have since O365 has an option,.

[Dexus]

I have allowed myself to modify the code, so the list is controlled from nextcloud/config/config.php:

/var/www/html/nextcloud/config/config.php

  'twofactor_whitelist_ips' =>
  array(
    "10.10.0.0/16" // LAN
  ),

/var/www/html/nextcloud/lib/private/Authentication/TwoFactorAuth/Manager.php

        /**
         * Determine whether the user must provide a second factor challenge
         *
         * @param IUser $user
         * @return boolean
         */
        private function ipMatch() {
            $ip = $_SERVER['REMOTE_ADDR'];
            **$cidrs=$this->config->getSystemValue('twofactor_whitelist_ips', []);**
            foreach((array) $cidrs as $cidr) {
                list($subnet, $mask) = explode('/', $cidr);
                if(((ip2long($ip) & ($mask = ~ ((1 << (32 - $mask)) - 1))) == (ip2long($subnet) & $mask))) {
                    return true;
                }
            }
            return false;
        }

        public function isTwoFactorAuthenticated(IUser $user): bool {

                // Check if IP in range
                if ($this->ipMatch()) { return false; }

                if (isset($this->userIsTwoFactorAuthenticated[$user->getUID()])) {
                        return $this->userIsTwoFactorAuthenticated[$user->getUID()];
                }

                if ($this->mandatoryTwoFactor->isEnforcedFor($user)) {
                        return true;
                }

Other idea is to modify additionally Set/GetState functions and some gui.

does this work with the 2FA TOTP plugin?

[silvan78]

It's working with Two-Factor TOTP Provider by Christoph Wurst, version 6.4.1. I have not tested it on other plugins.

[AndyXheli]

@silvan78 can you please let me what what lines i need to modify

[AndyXheli]

When i remove and add this server errors out. I Think might be doing something wrong

[silvan78]

Apologies for late answer.

Your code look scrambled, lines 135-137 should not be there (as they're the same as line 148-150). Also, "bolding" code in line 126 is wrong, i have incorrectly added "**" bolding code to source code block.. and it was shown. Try using patch command on original file, unless You have made additional modifications.

The diff/patch file (2fa_whitelist.patch):

122a123,134
>         private function ipMatch() {
>             $ip = $_SERVER['REMOTE_ADDR'];
>             $cidrs=$this->config->getSystemValue('twofactor_whitelist_ips', []);
>             foreach((array) $cidrs as $cidr) {
>                 list($subnet, $mask) = explode('/', $cidr);
>                 if(((ip2long($ip) & ($mask = ~ ((1 << (32 - $mask)) - 1))) == (ip2long($subnet) & $mask))) {
>                    return true;
>                 }
>             }
>             return false;
>         }
>
123a136,138
>                 // Check if IP in range
>                 if ($this->ipMatch()) { return false; }
>

root@nextcloud:~# patch /var/www/html/nextcloud/lib/private/Authentication/TwoFactorAuth/Manager.php 2fa_whitelist.patch

I'm still using version 25, but it should be the same in 26.

Also, remember to modify the config file /var/www/html/nextcloud/config/config.php so that array twofactor_whitelist_ips is defined:

  'twofactor_whitelist_ips' =>
  array (
    0 => '10.0.0.0/8',
  ),

[AndyXheli]

@silvan78 I figure it out. Looks good :) just tested in on NC 26 and im not seeing any issues

[AndyXheli]

Here's what my config looks like.

    /**
     * Determine whether the user must provide a second factor challenge
     *
     * @param IUser $user
     * @return boolean
     */
    private function ipMatch() {
     $ip = $_SERVER['REMOTE_ADDR'];
     $cidrs=$this->config->getSystemValue('twofactor_whitelist_ips', []);
     foreach((array) $cidrs as $cidr) {
         list($subnet, $mask) = explode('/', $cidr);
         if(((ip2long($ip) & ($mask = ~ ((1 << (32 - $mask)) - 1))) == (ip2long($subnet) & $mask))) {
             return true;
          }
    }
    return false;
  }

  public function isTwoFactorAuthenticated(IUser $user): bool {
    // Check if IP in range
    if ($this->ipMatch()) { return false; }

    if ($this->mandatoryTwoFactor->isEnforcedFor($user)) {
            return true;
    }
  }

[d-sko]

I've also tested this and it works great! Users from the Internet have to do 2FA, but all internal users can use Nextcloud without. But my test revealed some general issue with this: If an user only uses his Nextcloud account from within the trusted internal network and never tries to login from the internet, he never gets asked to setup 2FA. So when an attacker somehow aquires the password of such an user, he not only can access the users files without 2FA but he also is the one who sets up the second factor.

[AndyXheli]

Running NC 26 server i have enforced 2fa via admin seetings. So created a new user that i know dose not have 2FA TOTP enabeld just yet. First i had to user login via internal network that was on the whitelist to not ask for 2FA and user logged in susefully. Then i had the same user login outside of the network and then the user was forced to setup 2fa totp. So this patch works as indented. Since the user was internal 2fa was not asked then once the user accesd the server externaly then they had to setup 2fa.

[AndyXheli]

I also did a test while having this patch on my server.

    /**
     * Determine whether the user must provide a second factor challenge
     *
     * @param IUser $user
     * @return boolean
     */
    private function ipMatch() {
     $ip = $_SERVER['REMOTE_ADDR'];
     $cidrs=$this->config->getSystemValue('twofactor_whitelist_ips', []);
     foreach((array) $cidrs as $cidr) {
         list($subnet, $mask) = explode('/', $cidr);
         if(((ip2long($ip) & ($mask = ~ ((1 << (32 - $mask)) - 1))) == (ip2long($subnet) & $mask))) {
             return true;
          }
    }
    return false;
  }

  public function isTwoFactorAuthenticated(IUser $user): bool {
    // Check if IP in range
    if ($this->ipMatch()) { return false; }

    if ($this->mandatoryTwoFactor->isEnforcedFor($user)) {
            return true;
    }
  }

I removed the below lines from the config.php and the server was working nornaly asking for 2fa and no errors under the server logs

  'twofactor_whitelist_ips' =>
  array (
    0 => '10.0.0.0/8',
  ),

Now my question is can we add the below line of code on the server Manager.php and then have documention saying if someone dose want to do a ip whitelist they would need to add the below line in the config file with the naseary ips that they want to whitelist ?

  'twofactor_whitelist_ips' =>
  array (
    0 => '10.0.0.0/8',
  ),

@LukasReschke @nextcloud/security @ChristophWurst what do you all think about this?

[AndyXheli]

Also tested on NC 27 RC1 and no issues

[silvan78]

@AndyXheli I would suggest avoiding adding configuration variables to engine code. Whitelisting can be a security problem, this having a default non-empty list is no-go for community-wide code. But I see no problem defining this list inside the function, but this is originally PelleHanses idea mentioned above.

Pull request and merge to make this feature/patch permanent. But I guess that would have to wait for some GUI for whitelist management.