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

Checking Windows machines domain membership #9

Open
greenjimll opened this issue Feb 5, 2024 · 7 comments
Open

Checking Windows machines domain membership #9

greenjimll opened this issue Feb 5, 2024 · 7 comments

Comments

@greenjimll
Copy link

I noticed in the documentation mention of the issue with Windows machines popping up NTLM authentication boxes, which rather negates the point of the Negotiate module. We hit this some years ago with earlier version of simpleSAMLphp, and we couldn't just limit it by subnet as our users use a mix of organisationally owned domain bound machines and their own devices when on site.

I wrote a little work round that uses an extra, optional string in the negotiate module configuration in authsources.php called checkDomain. This is the AD domain name that the machine should be in. I then added an extra routine to the src/Auth/Source/Negotiate.php code to check this, with a call just after the subnet mask checks. It uses /usr/bin/nmblookup to search for the Windows machine, and ignores browsers that don't include "Windows NT" in their user agent string (so Linux boxes, Macs, etc).

In case it is of use, here's a diff of the changes against a recent version of the negotiate module:

$ diff Negotiate.php.dist Negotiate.php
58d57
< 
82a82
> 	$this->check_domain = $cfg->getOptionalString('checkDomain', NULL);
131a132,139
>         // Check if client is in allowed AD domain
>         $domainOK = $this->checkDomain();
>         if (!$domainOK) {
>             Logger::debug('Negotiate - Not a matching domain. falling back');
>             $this->fallBack($state);
> 	    return;
>         }
> 
226a235,267
>        /**
>          * checkDomain() looks up the domain that the client is bound into
>          *               and limits clients to the specified domain.
>          *
>          * Will return TRUE if no domain restriction option is configured.
>          *
>          * @return boolean
>          */
>         public function checkDomain(): bool {
>                 // No domain means all clients are accepted.
>                 if ($this->check_domain === NULL)
>                         return true;
> 
>                 // Only do this check for Windoze machines.
>                 $ua = $_SERVER['HTTP_USER_AGENT'];
>                 if (!preg_match("/Windows NT/", $ua)) {
>                    return true;           
>                 }
> 
>                 $ip = $_SERVER['REMOTE_ADDR'];
>                 $cmd = "/usr/bin/timeout 1 /usr/bin/nmblookup -A $ip 2>&1";
>                 $shellOutput = shell_exec($cmd);
>                 error_log($shellOutput,0);
>                 $lines = explode(PHP_EOL, $shellOutput);
>                 $myDomain = $this->check_domain;
>                 foreach ($lines as $line) {
>                   if(preg_match("/$myDomain\s+\<00\>\s\-\s\<GROUP\>.+ACTIVE/",
>                                 $line)) {
>                     return true;
>                   }
>                 }
>                 return false;
>         }

We've been using this in production on SSP 1.x IdP servers for many years now, and I've just ported it ready for use to move to SSP 2.x.

@tvdijen
Copy link
Member

tvdijen commented Feb 5, 2024

So, if I understand this correct, you want to suppress the sending of the HTTP/401 response if the client is not a member of your Active Directory domain?

@tvdijen
Copy link
Member

tvdijen commented Feb 5, 2024

My main concern with this patch is that 1) we shouldn't allow our IdP's to perform a shell_exec because it's risky.. 2) I'm concerned that this extra lookup won't scale well on large environments, 3) you are specifically focussing on Windows-clients, but a Linux or Mac machine may very well be a member of the Kerberos-realm and 4) it adds a dependency on some unix-command we don't control. This will break relentlessly if the output of the command changes.

[..] we couldn't just limit it by subnet as our users use a mix of organisationally owned domain bound machines and their own devices when on site.

This also scares the shit out of me, but who am I 🤣 You shouldn't be mixing trusted and untrusted devices on the same network if you ask me.

While I appreciate your contribution, I am a bit reluctant to merge this.. There must be a better/smarter way to get to the same result

@greenjimll
Copy link
Author

greenjimll commented Feb 5, 2024 via email

@tvdijen
Copy link
Member

tvdijen commented May 1, 2024

Hi Jon! I just came across this issue again.. Do you have any control over the various machines that visit your network? Windows machines will only try NTLM (and show the popup) if the site that your IdP is hosted on is in the 'trusted intranet-sites' zone.. Isn't that something you can work with?

Any machine that doesn't have the IdP in the trusted zone will automatically go to the fallback authentication source. In normal circumstances this would do NTLM auth for your organizationally owned devices and use the fallback for BYOD-devices?

@greenjimll
Copy link
Author

greenjimll commented May 2, 2024 via email

@tvdijen
Copy link
Member

tvdijen commented May 2, 2024

Would it work if you did gethostbyaddr($_SERVER['REMOTE_ADDR']) === $_SERVER['REMOTE_ADDR'] ?
If your DNS-server can translate the IP to a host, it must be a known machine.. If it cannot translate it, it must be a BYOD.
It would depend on how your infrastructure though.. If you hand out DNS-resolvable hostnames in DHCP, this will probably not work.

@tvdijen
Copy link
Member

tvdijen commented May 2, 2024

One other solution I'm thinking about is to leverage the LDAP-module and query the domain controller to find a computer-object where dNSHostName matches gethostbyaddr($_SERVER['REMOTE_ADDR']).. Since we already have the ldap-module available, it kind of makes sense to use it.

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

No branches or pull requests

2 participants