[patch] login module based on WWW-Authenticate: Negotiate

[Mathias Meisfjordskar]

Hello.

I've put together a module for allowing users to log in seamlessly using
Negotiate. I won't describe the process in great detail as most of it is
described in the docs/ directory in the module. Long story short this
module implements:

* Kerberos authentication through the web browser
* Fallback solution for clients unable to authN to Krb.
- This is just falling back to another SSP module
* Automatic redirects for clients failing to authN users (thus
seamlessly).
* Subnet filtering so every client outside of your domain isn't
subjected to a 401
* User metadata handling by LDAP backend.

Our goal was offer SSO from our Windows domain to our IdP completely
invisible to our users. This module does not implement such a daunting
task but it mimics such a behavior by letting domain authenticated users
log in on the IdP by checking their Krb credentials. After a SAML
session is obtained, the two sessions are completely separate.

What I would ask of the list is to inspect the code as I'm half-blind
from staring at this for far too long. Once Olav or another chief A-OKs
it, I would like to request this module be a part of SSP so it can
follow the regular development cycles.

Comments, suggestions and questions are always appreciated.

The only bug I can think of right now is logout handling and how the
authId handling is done. It works right now but it just seems "iffy" to
me.

PS: Look at the "Client" part of the documentation on how to set up a
browser to test this.

[Olav Morken]

Unfortunately, there isn't really a better solution available (yet).
The entire "delegate authentication" concept wasn't considered when
designing the authsource framework. Some changes could probably be made
to make it work better, but I haven't had the time to really look at
what would be required.

You could store the backend id in the state array though:

$state['LogoutState']['negotiate:delegated_auth_id'] = $backend;

It should then be available as $state['negotiate:delegated_auth_id'] in
the logout()-method.

> PS: Look at the "Client" part of the documentation on how to set up a
> browser to test this.

Some comments inline with the patch below:

> diff -ruN simplesamlphp-1.8.0/modules/negotiate/docs/negotiate.txt simplesamlphp-1.8.0_patch/modules/negotiate/docs/negotiate.txt
> --- simplesamlphp-1.8.0/modules/negotiate/docs/negotiate.txt 1970-01-01 01:00:00.000000000 +0100
> +++ simplesamlphp-1.8.0_patch/modules/negotiate/docs/negotiate.txt 2011-06-09 13:31:22.198614000 +0200
> @@ -0,0 +1,190 @@
> +Negotiate module
> +================
> +
> +The Negotiate module implements Microsofts Kerberos SPNEGO mechanism.
> +It is intended to only support Kerberos and not NTLM which RFC4559
> +implements.
> +
> +`negotiate:Negotiate`
> +: Authenticates users via HTTP authentication
> +
> +`negotiate:Negotiate`
> +---------------------
> +
> +Negotiate implements the following mechanics:
> +
> + * Initiate HTTP_AUTHN with the client
> + * Authorize user against a LDAP directory
> + * Collect metadata from LDAP directory
> + * Fall back to other SimpleSamlPhp module for any client/user that
> + fails to authenticate in the Negotiate module
> + * Check only clients from a certain subnet
> +
> +In effect this module aims to extend the Microsoft AD SSO session to
> +the SAML IdP. It doesn't work like this of course but for the user the
> +client is automatically authenticated when an SP sends the client to
> +the IdP. In reality Negotiate authenticates the user via SPNEGO and
> +issues a separate SAML session. The Kerberos session against the
> +Authentication Server is completely separate from the SAML session
> +with the IdP. The only time the Kerberos session affects the SAML
> +session is at authN at the IdP.
> +
> +The module is meant to supplement existing auth modules and not
> +replace them. Users do not always log in on the IdP from a machine in
> +the Windows domain (or another Kerberos domain) and from their own
> +domain accounts. A fallback mechanism must be supplemented.
> +
> +The Kerberos TGS can be issued for a wide variety of accounts so an
> +authoriation backend via LDAP is needed. If the search, with filters,
> +fails, the fallback in invoked. This to prevent kiosk accounts and the
> +likes to get faulty SAML sessions.
> +
> +The subnet is required to prevent excess attempts to authenticate via
> +Kerberos for clients that always will fail. Worst case scenario the
> +browser will prompt the user for u/p in a popup box that will always
> +fail. Only when the user clicks cancel the proper login process will
> +continue. This is handled through the body of the 401 message the
> +client recieves with the Negotiate request. In the body a URL to the
> +fallback mechanism is supplied and Javascript is used to redirect the
> +client.
> +
> +All configuration is handled in authsources.php:
> +
> + 'weblogin' => array(
> + 'negotiate:Negotiate',
> + 'keytab' => '/path/to/keytab-file',
> + 'fallback' => 'ldap',
> + 'hostname' => 'ldap.example.com',
> + 'base' => 'cn=people,dc=example,dc=com',
> + 'adminUser' => 'cn=idp-fallback,cn=services,dc=example,dc=com',
> + 'adminPassword' => 'VerySecretPassphraseHush'
> + ),
> + 'ldap' => array(
> + 'ldap:LDAP',
> + 'hostname' => 'ldap.example.com',
> + 'enable_tls' => TRUE,
> + 'dnpattern' => 'uid=%username%,cn=people,dc=example,dc=com',
> + 'search.enable' => FALSE
> + ),
> +
> +
> +
> +`php_krb5`
> +++++++++++
> +
> +The processing involving the actual Kerberos ticket handling is done
> +by php_krb5. The package is not yet labeled stable but has worked well
> +during testing.
> +
> +NOTE! php_krb5 hardcodes the service name in the keytab file to 'HTTP'
> +as of php_krb5-1.0rc2. To change this you need to edit the module code.
> +Be wary of how much space is allocated to the string in
> +negotiate_auth.c:101.
> +
> +Depending on you apache config you may need a rewrite rule to allow
> +php_krb5 to read the HTTP_AUTHORIZATION header:

What in the Apache config may lead to the HTTP_AUTHORIZATION header not
being provided?

> +
> + RewriteEngine on
> + RewriteCond %{HTTP:Authorization} !^$
> + RewriteRule .* - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization},L]
> +
> +
> +Test the Kerberos setup with the following script:
> +
> + <?php
> + if(!extension_loaded('krb5')) {
> + die('KRB5 Extension not installed');
> + }
> +
> + $foo = apache_request_headers();
> + if(!empty($foo['Authorization'])) {

Shouldn't $_SERVER['HTTP_AUTHORIZATION'] be used here also. (Since you
want to test the same code as the actual module uses?)

> + list($mech, $data) = split(' ', $foo['Authorization']);
> + if(strtolower($mech) == 'basic') {
> + echo "Client sent basic";
> + die('Unsupported request');
> + } else if(strtolower($mech) != 'negotiate') {
> + echo "Couldn't find negotiate";
> + die('Unsupported request');
> + }
> + $auth = new KRB5NegotiateAuth('/path/to/keytab');
> + $reply = '';
> + if($reply = $auth->doAuthentication()) {
> + header('HTTP/1.1 200 Success');
> + echo 'Success - authenticated as ' . $auth->getAuthenticatedUser() . '<br>';
> + } else {
> + echo 'Failed to authN.';
> + die();
> + }
> + } else {
> + header('HTTP/1.1 401 Unauthorized');
> + header('WWW-Authenticate: Negotiate',false);
> + echo 'Not authenticated';
> + }
> + ?>
> +
> +
> +
> +`LDAP`
> +++++++
> +
> +LDAP is used to verify the user due to the lack of metadata in
> +Kerberos. A domain can contain lots of kiosk users, non-personal
> +accounts and the likes. The LDAP lookup will authorize and fetch
> +attributes as defined by SimpleSamlPhp metadata.
> +
> +'hostname', 'enable_tls', 'debugLDAP', 'timeout' and 'base' are
> +self-explanatory. Read the documentation of the LDAP auth module for
> +more information. 'attr' is the attribute that will be used to look up
> +user objects in the directory after extracting it from the Kerberos
> +session. Default is 'uid'.
> +
> +For LDAP directories with restricted access to objects or attributes
> +Negotiate implements 'adminUser' and 'adminPassword'. adminUser must
> +be a DN to an object with access to search for all relevant user
> +objects and to look up attributes needed by the SP.
> +
> +
> +`Subnet filtering`
> +++++++++++++++++++
> +
> +Subnet is meant to filter which clients you subject to the
> +WWW-Authenticate request.
> +
> +Syntax is:
> +
> + 'subnet' => array('127.0.0.0/16','192.168.0.0/16'),
> +
> +Browsers, especially IE, behave erratically when they encounter a
> +WWW-Authenticate from the webserver. Included in RFC4559 Negotiate is
> +NTLM authentication which IE seems prone to fall back to under various
> +conditions. This triggers a popup login box which defeats the whole
> +purpose of this module.
> +
> +TBD: Replace or supplement with LDAP lookups in the domain. Machines
> +currently in the domain should be the only ones that are promted with
> +WWW-Authenticate: Negotiate.
> +
> +
> +`Clients`
> ++++++++++
> +
> +* Internet Explorer
> +
> +YMMV but generally you need to have your IdP defined in "Internet
> +Options" -> "Security" -> "Sites" -> "Advanced". You also need

I guess that this is the Sites-button for the "Local intranet" domain?

> +"Internet Options" -> "Advanced" -> "Security" -> Enable Integrated
> +Windows Authentication" enabled.
> +
> +* Firefox
> +
> +Open "about:config". Locate "network.auth.use-sspi" and verify that
> +this is true (on a Windows machine). Next locate
> +"network.negotiate-auth.trusted-uris" and insert your realm.
> +
> +* Safari
> +
> +TODO
> +
> +* Chrome
> +
> +TODO
> diff -ruN simplesamlphp-1.8.0/modules/negotiate/lib/Auth/Source/Negotiate.php simplesamlphp-1.8.0_patch/modules/negotiate/lib/Auth/Source/Negotiate.php
> --- simplesamlphp-1.8.0/modules/negotiate/lib/Auth/Source/Negotiate.php 1970-01-01 01:00:00.000000000 +0100
> +++ simplesamlphp-1.8.0_patch/modules/negotiate/lib/Auth/Source/Negotiate.php 2011-06-09 13:31:22.171628000 +0200
> @@ -0,0 +1,321 @@
> +<?php
> +
> +if(!extension_loaded('krb5')) {
> + die('KRB5 Extension not installed');
> +}

I think this check belongs in the constructor, and not here, and it
should fail with an exception. (Doing this would allow the
implementation of automated "sanity checking" of configured authsources
at a some point in time.)

> +
> +/**
> + * The Negotiate module. Allows for password-less, secure login by
> + * Kerberos and Negotiate.
> + *
> + * @author Mathias Meisfjordskar, University of Oslo.
> + * <mathias.me...@usit.uio.no>
> + * @package simpleSAMLphp
> + * @version $Id$
> + */
> +
> +class sspmod_negotiate_Auth_Source_Negotiate extends SimpleSAML_Auth_Source {
> +
> + // Constants used in the module
> + const AUTHID = 'sspmod_negotiate_Auth_Source_Negotiate.AuthId';
> + const STAGEID = 'sspmod_negotiate_Auth_Source_Negotiate.StageId';
> + const SESSION_SOURCE = 'negotiate:sessionSource';
> +
> + private $config;
> + protected $ldap = NULL;
> +
> + /**
> + * Constructor for this authentication source.
> + *
> + * @param array $info Information about this authentication source.
> + * @param array $config Configuration.
> + */
> + public function __construct($info, $config) {
> + assert('is_array($info)');
> + assert('is_array($config)');
> +
> + // Call the parent constructor first, as required by the interface.
> + parent::__construct($info, $config);
> +
> + $this->config = SimpleSAML_Configuration::loadFromArray($config);;

If possible, config parsing should be done in the constructor, not in
the individual functions. Doing that ensures that authentication does
not randomly fail due to configuration issues.

> +
> + foreach(array('hostname', 'base', 'keytab', 'fallback') as $i) {
> + if (!array_key_exists($i, $config)) {
> + throw new Exception('The required "'.$i.'" config option was not found');
> + }
> + }
> + }
> +
> + /**
> + * The inner workings of the module.
> + *
> + * Checks to see if client is in the defined subnets (if
> + * defined in config). Sends the client a 401 Negotiate and
> + * responds to the result. If the client fails to provide a
> + * proper Kerberos ticket, the login process is handed over to
> + * the 'fallback' module defined in the config.
> + *
> + * LDAP is used as a user metadata source.
> + *
> + * @param array &$state Information about the current authentication.
> + */
> + public function authenticate(&$state) {
> + assert('is_array($state)');
> +
> + $state[self::AUTHID] = $this->authId;
> +
> + // Save the $state array, so that we can restore if after a redirect
> + $id = SimpleSAML_Auth_State::saveState($state, self::STAGEID);
> + $backend = $this->config->getString('fallback');
> +
> + $mask = $this->checkMask();
> + if (!$mask) {
> + $this->fallBack($backend, $state);

Here you are calling a static function using $this. You should use
self::fallBack(...) instead.

> + // Never executed
> + assert('FALSE');
> + }
> +
> + $url = SimpleSAML_Module::getModuleURL('negotiate/backend.php');

This variable isn't used?

> + $params = array('AuthState' => $id);
> + SimpleSAML_Logger::debug('Negotiate - authenticate(): looking for Negotate');
> + if (!empty($_SERVER['HTTP_AUTHORIZATION'])) {
> + SimpleSAML_Logger::debug('Negotiate - authenticate(): Negotate found');
> + $hostname = $this->config->getString('hostname');
> + $enableTLS = $this->config->getBoolean('enable_tls', FALSE);
> + $debugLDAP = $this->config->getBoolean('debugLDAP', FALSE);
> + $timeout = $this->config->getValue('timeout', 30);
> + $this->ldap = new SimpleSAML_Auth_LDAP($hostname, $enableTLS, $debugLDAP, $timeout);

LDAP initialization probably belongs in the constructor.

> +
> + list($mech, $data) = split(' ', $_SERVER['HTTP_AUTHORIZATION']);

The split() function is deprecated in PHP 5.3. In this case, you should
use explode() instead.

> + if(strtolower($mech) == 'basic') {
> + SimpleSAML_Logger::debug('Negotiate - authenticate(): Basic found. Skipping.');
> + // goto fallback
> + } else if(strtolower($mech) != 'negotiate') {
> + SimpleSAML_Logger::debug('Negotiate - authenticate(): No "Negotiate" found. Skipping.');
> + // goto fallback
> + }

What exactly is the purpose of this if-statement?

> + $auth = new KRB5NegotiateAuth($this->config->getString('keytab'));
> + // Atempt Kerberos authentication
> + if($reply = $auth->doAuthentication()) {
> + // Success. Krb TGS recieved.
> + $user = $auth->getAuthenticatedUser();
> + SimpleSAML_Logger::info('Negotiate - authenticate(): '. $user . ' authenticated.');
> + $lookup = $this->lookupUserData($user, $state);
> + if ($lookup) {
> + $state['Attributes'] = $lookup;
> + // Save source of the authN
> + $session = SimpleSAML_Session::getInstance();
> + $session->setData(self::SESSION_SOURCE, $state[self::AUTHID], "negotiate");

I would use NULL or FALSE instead of "negotiate" here. (What if the
IdP for some reason has an authsource named negotiate?)

> +
> + SimpleSAML_Auth_Source::completeAuth($state);
> + SimpleSAML_Logger::info('Negotiate - authenticate(): '. $user . ' authorized.');
> + exit;

completeAuth() will never return, so those two lines can probably be
replaced with an assert('FALSE').

> + }
> + } else {
> + // Some error in the recieved ticket. Expired?
> + SimpleSAML_Logger::info('Negotiate - authenticate(): Kerberos authN failed. Skipping.');
> + // goto fallback
> + }
> + } else {
> + // No auth token. Send it.
> + SimpleSAML_Logger::debug('Negotiate - authenticate(): Sending Negotiate.');
> + $this->sendNegotiate($params);
> + exit;
> + }
> +
> + SimpleSAML_Logger::info('Negotiate - authenticate(): Client failed Negotiate. Falling back');
> + $this->fallBack($backend, $state);

self::fallBack(...)

> + /* The previous function never returns, so this code is never
> + executed */
> + assert('FALSE');
> + }
> +
> + /**
> + * checkMask() looks up the subnet config option and verifies
> + * that the client is within that range.
> + *
> + * Will return TRUE if no subnet option is configured.
> + *
> + * @returns boolean

The phpdoc syntax is @return.

> + */
> + protected function checkMask() {
> + // No subnet means all clients are accepted.
> + $subnet = $this->config->getArray('subnet', NULL);
> + if (!$subnet)
> + return TRUE;

Note that !$subnet means that an empty array will be treated the same
way as if the option is unset or set to NULL. (This isn't necessarily
wrong, but allowing an empty array to be set would provide a simple way
to temporarily disable Kerberos authentication.)

> + // Config must be an array.
> + if (!is_array($subnet))
> + throw new Exception('The subnet config option must be an array');

I don't think that check can fail, since getArray() will return an
array or throw an exception.

> +
> + foreach ($subnet as $cidr) {
> + $ip = $_SERVER['REMOTE_ADDR'];

This assignment should probably be moved out of the loop. (At least if
you decide to process an empty subnet list.)

> + $ret = SimpleSAML_Utilities::ipCIDRcheck($cidr);
> + if ($ret) {
> + SimpleSAML_Logger::debug('Negotiate: Client "'.$ip.'" matched subnet.');
> + return TRUE;
> + }
> + }
> + SimpleSAML_Logger::debug('Negotiate: Client "'.$ip.'" did not match subnet.');
> + return FALSE;
> + }
> +
> + /**
> + * Send the actual headers and body of the 401. Embedded in
> + * the body is a post that is triggered by JS if the client
> + * wants to show the 401 message.
> + *
> + * @param array $params additional parameters to the URL in
> + * the URL in the body
> + */
> + protected function sendNegotiate($params) {
> + $url = SimpleSAML_Module::getModuleURL('negotiate/backend.php');
> + $params['backend'] = $this->config->getString('fallback');
> + $url = SimpleSAML_Utilities::addURLparameter($url, $params);

getModuleURL() takes an params array as it's second argument, if you want
to simplify this code slightly.

> +
> + header('HTTP/1.1 401 Unauthorized');
> + header('WWW-Authenticate: Negotiate',false);
> + echo '
> +<html>
> +<head>
> +<title>POST data</title>

Is there any specific reason for using a POST request here? If not,
this can probably be handled with a <meta http-equiv="refresh" [...]>.
That type of redirect should work in browsers with javascript disabled.
You should probably still include a link on the page though, just in
case the user has managed to disable meta refresh.

> +<body onload="document.forms[0].submit()">
> +<noscript>
> +<p><strong>Note:</strong> Since your browser does not support JavaScript, you must press the button below once to proceed.</p>
> +</noscript>
> +<form method="post" action="'. $url .'">

You should use htmlspecialchars around $url, in case it contains any
special characters.

> +<noscript>
> +<input type="submit" value="Submit" >
> +</noscript>
> +</form>
> +</body>
> +</html> ';
> +
> + }
> +
> + /**
> + * Passes control of the login process to a different module.
> + *
> + * @param string $backend Name of the fallback module.
> + */
> + public static function fallBack($backend, $state) {
> +
> + $as = SimpleSAML_Auth_Source::getById($backend);
> + if ($as === NULL) {
> + throw new Exception('Invalid authentication source: ' . $backend);
> + }
> + /* Save the selected authentication source for the logout process. */
> + $session = SimpleSAML_Session::getInstance();
> + $session->setData(self::SESSION_SOURCE, $state[self::AUTHID], $backend);
> +
> + try {
> + $as->authenticate($state);
> + } catch (SimpleSAML_Error_Exception $e) {
> + SimpleSAML_Auth_State::throwException($state, $e);
> + } catch (Exception $e) {
> + $e = new SimpleSAML_Error_UnserializableException($e);
> + SimpleSAML_Auth_State::throwException($state, $e);
> + }
> + self::loginCompleted($state);
> + }
> +
> + /**
> + * Strips away the realm of the Kerberos identifier, looks up
> + * what attributes to fetch from SP metadata and searches the
> + * directory.
> + *
> + * @param string $user The Kerberos user identifier
> + * @param array $state The session state.
> + */
> + protected function lookupUserData($user, $state) {
> + // Kerberos usernames include realm. Strip that away.
> + // Works in PHP < 5.3

What happens in PHP >= 5.3?

> + $pos = strpos($user, '@');
> + if ($pos === false)
> + return NULL;
> + $uid = substr($user, 0, $pos);
> +
> + $base = $this->config->getString('base');
> + $attr = $this->config->getString('attr', 'uid');
> +
> + if (isset($state['SPMetadata'])) {
> + $spMetadata = $state['SPMetadata'];
> + if (isset($spMetadata['attributes'])) {
> + SimpleSAML_Logger::debug('Negotiate - found "attributes" in SPmetadata');
> + $attr_list = $spMetadata['attributes'];

You should not set $attr_list based on metadata. Doing that will fail
if the user accesses a different SP requiring more attributes later.
(Since the session will only contain the attributes required by the
first SP.)

> + }
> + } else
> + $attr_list = array();
> +
> + $this->adminBind();
> + try {
> + $dn = $this->ldap->searchfordn($base, $attr, $uid);
> + return $this->ldap->getAttributes($dn, $attr_list);

Shouldn't you call $this->getAttributes($dn) here?

> + } catch (SimpleSAML_Error_Exception $e) {
> + SimpleSAML_Logger::debug('Negotiate - ldap lookup failed: '. $e);
> + return NULL;
> + }
> + }
> +
> + /**
> + * Elevates the LDAP connection to allow restricted lookups if
> + * so configured. Does nothing if not.
> + */
> + protected function adminBind() {
> + if (!$this->config->hasValue('adminUser')) {
> + /* No admin user. */
> + return;
> + }
> +
> + $user = $this->config->getString('adminUser');
> + $pw = $this->config->getString('adminPassword');
> +
> + SimpleSAML_Logger::debug('Negotiate - authenticate(): Binding as system user ' . var_export($user, TRUE));
> +
> + if(!$this->ldap->bind($user, $pw)){
> + $msg = 'Unable to authenticate system user (LDAP_INVALID_CREDENTIALS) ' . var_export($user, TRUE);
> + SimpleSAML_Logger::error('Negotiate - authenticate(): ' . $msg);
> + throw new SimpleSAML_Error_AuthSource($msg);
> + }
> + }
> +
> + /**
> + * Fetch the user's attributes.
> + *
> + * @param string $dn The DN of the user.
> + * @return array Associative array with the user's attribute.
> + */
> + protected function getAttributes($dn) {
> + assert('is_string($dn)');
> +
> + $attributes = $this->location->getValue('attributes', NULL);
> + $maxSize = $this->location->getValue('attributesize.max', NULL);

$this->location doesn't exists. (But this config parsing should
probably be moved into the constructor in any case.)

> +
> + return $this->ldap->getAttributes($dn, $attributes, $maxSize);
> + }
> +
> +
> + /**
> + * Log out from this authentication source.
> + *
> + * This method either logs the user out from Negotiate or passes the
> + * logout call to the fallback module.
> + *
> + * @param array &$state Information about the current logout operation.
> + */
> + public function logout(&$state) {
> + assert('is_array($state)');
> +
> + /* Get the source that was used to authenticate */
> + $session = SimpleSAML_Session::getInstance();
> + $authId = $session->getData(self::SESSION_SOURCE, $this->authId);
> + if ($authId == "negotiate") {
> + parent::logout($state);
> + } else {
> + $source = SimpleSAML_Auth_Source::getById($authId);
> + $source->logout($state);
> + }
> + }
> +
> +}
> +
> +?>

The ?> at the end of the file isn't needed by PHP, and dropping it
allows us to not worry about (apparently) blank lines at the end of the
file causing output to be sent to the browser.

> diff -ruN simplesamlphp-1.8.0/modules/negotiate/www/backend.php simplesamlphp-1.8.0_patch/modules/negotiate/www/backend.php
> --- simplesamlphp-1.8.0/modules/negotiate/www/backend.php 1970-01-01 01:00:00.000000000 +0100
> +++ simplesamlphp-1.8.0_patch/modules/negotiate/www/backend.php 2011-06-09 13:31:22.185616000 +0200
> @@ -0,0 +1,19 @@
> +<?php
> +
> +/**
> + * Provide a URL for the module to statically link to.
> + *
> + * @author Mathias Meisfjordskar, University of Oslo.
> + * <mathias.me...@usit.uio.no>
> + * @package simpleSAMLphp
> + * @version $Id$
> + */
> +
> +$authStateId = $_REQUEST['AuthState'];
> +$backend = $_REQUEST['backend'];

You should probably store $backend in the state array instead. Taking it
from the request allows an user to choose any of the authsources
available on the IdP instead of what is configured in the current
authsource.

> +$state = SimpleSAML_Auth_State::loadState($authStateId, sspmod_negotiate_Auth_Source_Negotiate::STAGEID);
> +
> +sspmod_negotiate_Auth_Source_Negotiate::fallBack($backend, $state);
> +
> +exit;
> +?>
> \ No newline at end of file

The ?> can be removed here also.

Regards,
Olav Morken
UNINETT / Feide

[Mathias Meisfjordskar]

[ Olav Morken ]

Thanks for the fast reply, Olav. I was going to apply your comments
straight away and send a revised patch but a few questions appeared.

>> The only bug I can think of right now is logout handling and how the
>> authId handling is done. It works right now but it just seems "iffy" to
>> me.
>
> Unfortunately, there isn't really a better solution available (yet).
> The entire "delegate authentication" concept wasn't considered when
> designing the authsource framework. Some changes could probably be made
> to make it work better, but I haven't had the time to really look at
> what would be required.
>
> You could store the backend id in the state array though:
>
> $state['LogoutState']['negotiate:delegated_auth_id'] = $backend;
>
> It should then be available as $state['negotiate:delegated_auth_id'] in
> the logout()-method.

That sounds better indeed but I have a few questions about how do do
this later on.

>> +Depending on you apache config you may need a rewrite rule to allow
>> +php_krb5 to read the HTTP_AUTHORIZATION header:
>
> What in the Apache config may lead to the HTTP_AUTHORIZATION header not
> being provided?

I don't not know but during our testing we saw that PHP never recieved
the full headers and stuff like HTTP_AUTHORIZATION was stripped away by
apache. Something about PHP acting like CGI or something like it. Either
way the people setting up this module should check this with the sample
script (and your changes).

>> +
>> + RewriteEngine on
>> + RewriteCond %{HTTP:Authorization} !^$
>> + RewriteRule .* - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization},L]
>

> Shouldn't $_SERVER['HTTP_AUTHORIZATION'] be used here also. (Since you
> want to test the same code as the actual module uses?)

Indeed. Changed.

>> +YMMV but generally you need to have your IdP defined in "Internet
>> +Options" -> "Security" -> "Sites" -> "Advanced". You also need
>
> I guess that this is the Sites-button for the "Local intranet" domain?

Yup. Changed.

>> +if(!extension_loaded('krb5')) {
>> + die('KRB5 Extension not installed');
>> +}
>
> I think this check belongs in the constructor, and not here, and it
> should fail with an exception. (Doing this would allow the
> implementation of automated "sanity checking" of configured authsources
> at a some point in time.)

Sounds sensible. Moved and changed it.

>> + public function __construct($info, $config) {
>> + assert('is_array($info)');
>> + assert('is_array($config)');
>> +
>> + // Call the parent constructor first, as required by the interface.
>> + parent::__construct($info, $config);
>> +
>> + $this->config = SimpleSAML_Configuration::loadFromArray($config);;
>
> If possible, config parsing should be done in the constructor, not in
> the individual functions. Doing that ensures that authentication does
> not randomly fail due to configuration issues.

Ok. From my limited PHP experience, I understand that __construct() is
the constructor. Are you refering to the code later on? I can move those
bits up here.

>> + public function authenticate(&$state) {
>> + assert('is_array($state)');
>> +
>> + $state[self::AUTHID] = $this->authId;
>> +
>> + // Save the $state array, so that we can restore if after a redirect
>> + $id = SimpleSAML_Auth_State::saveState($state, self::STAGEID);
>> + $backend = $this->config->getString('fallback');
>> +
>> + $mask = $this->checkMask();
>> + if (!$mask) {
>> + $this->fallBack($backend, $state);
>
> Here you are calling a static function using $this. You should use
> self::fallBack(...) instead.

How come? self:: will always point to
sspmod_negotiate_Auth_Source_Negotiate:fallBack() while using
$this->fallBack() will allow for sub-classing and overrides of the
function if needed. Isn't that a bit more dynamic design?

If by some reason you wanted to do the above of course. Maybe it's
opening a can of worms down the line, I don't know.

>> + // Never executed
>> + assert('FALSE');
>> + }
>> +
>> + $url = SimpleSAML_Module::getModuleURL('negotiate/backend.php');
>
> This variable isn't used?

Right. Removed.

>> + $params = array('AuthState' => $id);
>> + SimpleSAML_Logger::debug('Negotiate - authenticate(): looking for Negotate');
>> + if (!empty($_SERVER['HTTP_AUTHORIZATION'])) {
>> + SimpleSAML_Logger::debug('Negotiate - authenticate(): Negotate found');
>> + $hostname = $this->config->getString('hostname');
>> + $enableTLS = $this->config->getBoolean('enable_tls', FALSE);
>> + $debugLDAP = $this->config->getBoolean('debugLDAP', FALSE);
>> + $timeout = $this->config->getValue('timeout', 30);
>> + $this->ldap = new SimpleSAML_Auth_LDAP($hostname, $enableTLS, $debugLDAP, $timeout);
>
> LDAP initialization probably belongs in the constructor.

I though about this and moved it here as logout and any other call to
the construct won't trigger a new LDAP initialization. If nothing else
the logs are kept cleaner and less code is executed.

It would be better to have up in the constructor for the same reasons
you explained above but one is more likely to hit a snag with the actual
search which can't be moved. Your call, I'm new at this. :)

>> +
>> + list($mech, $data) = split(' ', $_SERVER['HTTP_AUTHORIZATION']);
>
> The split() function is deprecated in PHP 5.3. In this case, you should
> use explode() instead.

OK. Changed.

>> + if(strtolower($mech) == 'basic') {
>> + SimpleSAML_Logger::debug('Negotiate - authenticate(): Basic found. Skipping.');
>> + // goto fallback
>> + } else if(strtolower($mech) != 'negotiate') {
>> + SimpleSAML_Logger::debug('Negotiate - authenticate(): No "Negotiate" found. Skipping.');
>> + // goto fallback
>> + }
>
> What exactly is the purpose of this if-statement?

Good question. I only put it there as extra debugging in case the
browser spazzes out and sends gibberish in the reply. I think it can be
safely removed but there's little harm in having it there. The test for
basic is just a precaution that the browser wouldn't reply with basic
for some strange reason. It never should. Think I should remove them?

>> + $auth = new KRB5NegotiateAuth($this->config->getString('keytab'));
>> + // Atempt Kerberos authentication
>> + if($reply = $auth->doAuthentication()) {
>> + // Success. Krb TGS recieved.
>> + $user = $auth->getAuthenticatedUser();
>> + SimpleSAML_Logger::info('Negotiate - authenticate(): '. $user . ' authenticated.');
>> + $lookup = $this->lookupUserData($user, $state);
>> + if ($lookup) {
>> + $state['Attributes'] = $lookup;
>> + // Save source of the authN
>> + $session = SimpleSAML_Session::getInstance();
>> + $session->setData(self::SESSION_SOURCE, $state[self::AUTHID], "negotiate");
>
> I would use NULL or FALSE instead of "negotiate" here. (What if the
> IdP for some reason has an authsource named negotiate?)

Hehe, very good point. The code was cut'n pasted from somewhere else.

>> +
>> + SimpleSAML_Auth_Source::completeAuth($state);
>> + SimpleSAML_Logger::info('Negotiate - authenticate(): '. $user . ' authorized.');
>> + exit;
>
> completeAuth() will never return, so those two lines can probably be
> replaced with an assert('FALSE').

Right you are. Replaced.

>> + SimpleSAML_Logger::info('Negotiate - authenticate(): Client failed Negotiate. Falling back');
>> + $this->fallBack($backend, $state);
>
> self::fallBack(...)

Commented on that above.

>> + * @returns boolean
>
> The phpdoc syntax is @return.

Whoopsie. Fixed.

>> + */
>> + protected function checkMask() {
>> + // No subnet means all clients are accepted.
>> + $subnet = $this->config->getArray('subnet', NULL);
>> + if (!$subnet)
>> + return TRUE;
>
> Note that !$subnet means that an empty array will be treated the same
> way as if the option is unset or set to NULL. (This isn't necessarily
> wrong, but allowing an empty array to be set would provide a simple way
> to temporarily disable Kerberos authentication.)

Good idea. Changed.

>> + // Config must be an array.
>> + if (!is_array($subnet))
>> + throw new Exception('The subnet config option must be an array');
>
> I don't think that check can fail, since getArray() will return an
> array or throw an exception.

Oh yeah. I guess getArray() will handle that bit. Removed.

>> +
>> + foreach ($subnet as $cidr) {
>> + $ip = $_SERVER['REMOTE_ADDR'];
>
> This assignment should probably be moved out of the loop. (At least if
> you decide to process an empty subnet list.)

Hehe, the joys of rushed programming. I didn't even notice it until you
pointed it out. Fixed.

>> + protected function sendNegotiate($params) {
>> + $url = SimpleSAML_Module::getModuleURL('negotiate/backend.php');
>> + $params['backend'] = $this->config->getString('fallback');
>> + $url = SimpleSAML_Utilities::addURLparameter($url, $params);
>
> getModuleURL() takes an params array as it's second argument, if you want
> to simplify this code slightly.

Roger. Fixed.

>> +
>> + header('HTTP/1.1 401 Unauthorized');
>> + header('WWW-Authenticate: Negotiate',false);
>> + echo '
>> +<html>
>> +<head>
>> +<title>POST data</title>
>
> Is there any specific reason for using a POST request here? If not,
> this can probably be handled with a <meta http-equiv="refresh" [...]>.
> That type of redirect should work in browsers with javascript disabled.
> You should probably still include a link on the page though, just in
> case the user has managed to disable meta refresh.

I'm utterly clueless when it comes to HTML so no, no reason. I just took
something that worked. Will refresh work with the supplied URL?
www/backend.php only exists to have a fixed URL to point clients to that
won't answer the 401.

>> +<body onload="document.forms[0].submit()">
>> +<noscript>
>> +<p><strong>Note:</strong> Since your browser does not support JavaScript, you must press the button below once to proceed.</p>
>> +</noscript>
>> +<form method="post" action="'. $url .'">
>
> You should use htmlspecialchars around $url, in case it contains any
> special characters.

Fixed.

>> + protected function lookupUserData($user, $state) {
>> + // Kerberos usernames include realm. Strip that away.
>> + // Works in PHP < 5.3
>
> What happens in PHP >= 5.3?

I could have used strstr($user, '@', TRUE) but alas, stuck on PHP
5.2.something.

I also found out that PHP didn't implement goto until 5.3. Talk about
weird development. I would have figured this simple directive would be
one of the first to be implemented. Oh well, the more you know.

>> + if (isset($state['SPMetadata'])) {
>> + $spMetadata = $state['SPMetadata'];
>> + if (isset($spMetadata['attributes'])) {
>> + SimpleSAML_Logger::debug('Negotiate - found "attributes" in SPmetadata');
>> + $attr_list = $spMetadata['attributes'];
>
> You should not set $attr_list based on metadata. Doing that will fail
> if the user accesses a different SP requiring more attributes later.
> (Since the session will only contain the attributes required by the
> first SP.)

Ah, well how do I then respect the attributes list in the SP metadata?
Some modules seem to disregard the list completely and I want this to
act like my fallback module does (meaning it reads the list in metadata
for the SP and returns those attributes).

>> + return $this->ldap->getAttributes($dn, $attr_list);
>
> Shouldn't you call $this->getAttributes($dn) here?

Yes, the last sessions of fixing loose ends was among them attribute
handling. The SPMetadata bits should have been placed in
getAttributes(). I'll fix it once the attribute handling is sorted.

>> + protected function getAttributes($dn) {
>> + assert('is_string($dn)');
>> +
>> + $attributes = $this->location->getValue('attributes', NULL);
>> + $maxSize = $this->location->getValue('attributesize.max', NULL);
>
> $this->location doesn't exists. (But this config parsing should
> probably be moved into the constructor in any case.)

I blame it on too much caffeine and hammering my limbs at the keyboard
for too long. Residue from previous code.

>> + public function logout(&$state) {
>> + assert('is_array($state)');
>> +
>> + /* Get the source that was used to authenticate */
>> + $session = SimpleSAML_Session::getInstance();
>> + $authId = $session->getData(self::SESSION_SOURCE, $this->authId);
>> + if ($authId == "negotiate") {
>> + parent::logout($state);
>> + } else {
>> + $source = SimpleSAML_Auth_Source::getById($authId);
>> + $source->logout($state);
>> + }
>> + }
>> +
>> +}
>> +
>> +?>
>
> The ?> at the end of the file isn't needed by PHP, and dropping it
> allows us to not worry about (apparently) blank lines at the end of the
> file causing output to be sent to the browser.

Did not know that. Running strict mode (if that exists or will be
implemented) won't bit me for this?

>> diff -ruN simplesamlphp-1.8.0/modules/negotiate/www/backend.php simplesamlphp-1.8.0_patch/modules/negotiate/www/backend.php
>> --- simplesamlphp-1.8.0/modules/negotiate/www/backend.php 1970-01-01 01:00:00.000000000 +0100
>> +++ simplesamlphp-1.8.0_patch/modules/negotiate/www/backend.php 2011-06-09 13:31:22.185616000 +0200
>> @@ -0,0 +1,19 @@
>> +<?php
>> +
>> +/**
>> + * Provide a URL for the module to statically link to.
>> + *
>> + * @author Mathias Meisfjordskar, University of Oslo.
>> + * <mathias.me...@usit.uio.no>
>> + * @package simpleSAMLphp
>> + * @version $Id$
>> + */
>> +
>> +$authStateId = $_REQUEST['AuthState'];
>> +$backend = $_REQUEST['backend'];
>
> You should probably store $backend in the state array instead. Taking it
> from the request allows an user to choose any of the authsources
> available on the IdP instead of what is configured in the current
> authsource.

I see. I was going to ask about this as the module multiauth implements
it's source like this and it struct me as a small security hole for the
reasons you describe. It's very minor so probably nothing to worry
about. For multiauth it could be a feature as users are meant to be able
to choose directly in the URL but at least the source should be verified
in the config in authsources.

I'm afraid I don't fully understand the session handling bits in SSP. I
set SESSION_SOURCE when I fall back or accept the Negotiate process so
all cases should be covered. Should I just load backend from $state in
www/backend.php by

$backend = $state[sspmod_negotiate_Auth_Source_Negotiate::SESSION:SOURCE][$authStateId]

or is there a more eloquent method?

Could backend be skippend from backend.php and as an argument to
fallBack() by doing the steps in fallBack() itself as state is passed to
it?

Apologies for the numerous questions and comments and thanks a lot for
all the help. Hopefully there won't be too many cycles of walls of text
before this is completed. Meanwhile, have a great (long) weekend!

--
Mathias Meisfjordskar
GNU/Linux addict.

"If it works; HIT IT AGAIN!"

[Olav Morken]

On Fri, Jun 10, 2011 at 13:42:26 +0200, Mathias Meisfjordskar wrote:
> [ Olav Morken ]
>
> Thanks for the fast reply, Olav. I was going to apply your comments
> straight away and send a revised patch but a few questions appeared.
>
> >> The only bug I can think of right now is logout handling and how the
> >> authId handling is done. It works right now but it just seems "iffy" to
> >> me.
> >
> > Unfortunately, there isn't really a better solution available (yet).
> > The entire "delegate authentication" concept wasn't considered when
> > designing the authsource framework. Some changes could probably be made
> > to make it work better, but I haven't had the time to really look at
> > what would be required.
> >
> > You could store the backend id in the state array though:
> >
> > $state['LogoutState']['negotiate:delegated_auth_id'] = $backend;
> >
> > It should then be available as $state['negotiate:delegated_auth_id'] in
> > the logout()-method.
>
> That sounds better indeed but I have a few questions about how do do
> this later on.

Some parts of the $state array are saved in the session after
authentication, amongs them 'LogoutData' and 'Attributes'. Most other
things in the $state array are temporary for the current authentication
request.

(You can manually request that other pieces of data is stored by adding
the key to the PersistentAuthData array:

$state['PersistentAuthData'][] = 'mymodule:mydata';

Of course, in this case using LogoutData is probably better, since
moving information from the authentication step to the logout step is
exatly what it was designed for.)

> >> +Depending on you apache config you may need a rewrite rule to allow
> >> +php_krb5 to read the HTTP_AUTHORIZATION header:
> >
> > What in the Apache config may lead to the HTTP_AUTHORIZATION header not
> > being provided?
>
> I don't not know but during our testing we saw that PHP never recieved
> the full headers and stuff like HTTP_AUTHORIZATION was stripped away by
> apache. Something about PHP acting like CGI or something like it. Either
> way the people setting up this module should check this with the sample
> script (and your changes).

I got curious, and decided to do a quick grep over the Apache source,
which reveals the following block of code in server/util_script.c:

/*
* You really don't want to disable this check, since it leaves you
* wide open to CGIs stealing passwords and people viewing them
* in the environment with "ps -e". But, if you must...
*/
#ifndef SECURITY_HOLE_PASS_AUTHORIZATION
else if (!strcasecmp(hdrs[i].key, "Authorization")
|| !strcasecmp(hdrs[i].key, "Proxy-Authorization")) {
continue;
}
#endif

I'm not sure that I agree with their reasoning here, but there isn't
much to be done about it in any case.

Btw.: I see that PHP has an environment variable $_SERVER['AUTH_TYPE'].
Maybe it be used to bypass this restriction? (I'm not actually sure
whether it is any more reliable though - maybe PHP only initializes it
from the HTTP_AUTHORIZATION data.)

[...]

> >> + public function __construct($info, $config) {
> >> + assert('is_array($info)');
> >> + assert('is_array($config)');
> >> +
> >> + // Call the parent constructor first, as required by the interface.
> >> + parent::__construct($info, $config);
> >> +
> >> + $this->config = SimpleSAML_Configuration::loadFromArray($config);;
> >
> > If possible, config parsing should be done in the constructor, not in
> > the individual functions. Doing that ensures that authentication does
> > not randomly fail due to configuration issues.
>
> Ok. From my limited PHP experience, I understand that __construct() is
> the constructor. Are you refering to the code later on? I can move those
> bits up here.

I am basically refering to all places where you have
$this->config->get<something>().

> >> + public function authenticate(&$state) {
> >> + assert('is_array($state)');
> >> +
> >> + $state[self::AUTHID] = $this->authId;
> >> +
> >> + // Save the $state array, so that we can restore if after a redirect
> >> + $id = SimpleSAML_Auth_State::saveState($state, self::STAGEID);
> >> + $backend = $this->config->getString('fallback');
> >> +
> >> + $mask = $this->checkMask();
> >> + if (!$mask) {
> >> + $this->fallBack($backend, $state);
> >
> > Here you are calling a static function using $this. You should use
> > self::fallBack(...) instead.
>
> How come? self:: will always point to
> sspmod_negotiate_Auth_Source_Negotiate:fallBack() while using
> $this->fallBack() will allow for sub-classing and overrides of the
> function if needed. Isn't that a bit more dynamic design?
>
> If by some reason you wanted to do the above of course. Maybe it's
> opening a can of worms down the line, I don't know.

You cannot rely on overriding this function in any case, since it is
called statically from backend.php. (I do not know whether PHP allows
you to override static methods either. Methods are normally static to
allow you to call them without an instance of the class available, so
allowing them to be overridden does not really make sense.)

[...]

> >> + $params = array('AuthState' => $id);
> >> + SimpleSAML_Logger::debug('Negotiate - authenticate(): looking for Negotate');
> >> + if (!empty($_SERVER['HTTP_AUTHORIZATION'])) {
> >> + SimpleSAML_Logger::debug('Negotiate - authenticate(): Negotate found');
> >> + $hostname = $this->config->getString('hostname');
> >> + $enableTLS = $this->config->getBoolean('enable_tls', FALSE);
> >> + $debugLDAP = $this->config->getBoolean('debugLDAP', FALSE);
> >> + $timeout = $this->config->getValue('timeout', 30);
> >> + $this->ldap = new SimpleSAML_Auth_LDAP($hostname, $enableTLS, $debugLDAP, $timeout);
> >
> > LDAP initialization probably belongs in the constructor.
>
> I though about this and moved it here as logout and any other call to
> the construct won't trigger a new LDAP initialization. If nothing else
> the logs are kept cleaner and less code is executed.
>
> It would be better to have up in the constructor for the same reasons
> you explained above but one is more likely to hit a snag with the actual
> search which can't be moved. Your call, I'm new at this. :)

I don't expect the search to be moved, but moving the LDAP
initialization should work. But after examining what we do in the
LDAP authsources, I think I will reverse my position, to maintain
consistency. In those modules, we don't initialize the LDAP class
before actually using it.

[...]

> >> + if(strtolower($mech) == 'basic') {
> >> + SimpleSAML_Logger::debug('Negotiate - authenticate(): Basic found. Skipping.');
> >> + // goto fallback
> >> + } else if(strtolower($mech) != 'negotiate') {
> >> + SimpleSAML_Logger::debug('Negotiate - authenticate(): No "Negotiate" found. Skipping.');
> >> + // goto fallback
> >> + }
> >
> > What exactly is the purpose of this if-statement?
>
> Good question. I only put it there as extra debugging in case the
> browser spazzes out and sends gibberish in the reply. I think it can be
> safely removed but there's little harm in having it there. The test for
> basic is just a precaution that the browser wouldn't reply with basic
> for some strange reason. It never should. Think I should remove them?

They can serve as useful debug information, but I think you should
remove the �// goto fallback� comments from that block, since they are
a bit misleading.

[...]

> >> +
> >> + header('HTTP/1.1 401 Unauthorized');
> >> + header('WWW-Authenticate: Negotiate',false);
> >> + echo '
> >> +<html>
> >> +<head>
> >> +<title>POST data</title>
> >
> > Is there any specific reason for using a POST request here? If not,
> > this can probably be handled with a <meta http-equiv="refresh" [...]>.
> > That type of redirect should work in browsers with javascript disabled.
> > You should probably still include a link on the page though, just in
> > case the user has managed to disable meta refresh.
>
> I'm utterly clueless when it comes to HTML so no, no reason. I just took
> something that worked. Will refresh work with the supplied URL?
> www/backend.php only exists to have a fixed URL to point clients to that
> won't answer the 401.

It can be used with an URL. See the third example at:

http://en.wikipedia.org/wiki/Meta_refresh#Examples

An alternative would be to use window.location.replace() in javascript.
The advantage of that is that this page will be removed from the
history in the browser, so that the user won't land on it when using
the back button. You would still need a link for those users with
javascript disabled though.

[...]

> >> + protected function lookupUserData($user, $state) {
> >> + // Kerberos usernames include realm. Strip that away.
> >> + // Works in PHP < 5.3
> >
> > What happens in PHP >= 5.3?
>
> I could have used strstr($user, '@', TRUE) but alas, stuck on PHP
> 5.2.something.

The reason for my question was that the comment led me to believe that
maybe the realm wasn't included in PHP >= 5.3 or something like that.
I didn't consider that it described why the next few lines were written
the way they were. (The next few lines look like the normal way of
doing it in PHP for me.)

If you really want to compress it into one line, I guess the following
will work:

list($uid) = explode('@', $user);

(Extra values appear to be ignored by list().)

Alternatively, what I sometimes do:

$user = explode('@', $user, 2);
$uid = $user[0];

(For some strange reason PHP doesn't allow explode(...)[0] or
(explode(...))[0].)

> I also found out that PHP didn't implement goto until 5.3. Talk about
> weird development. I would have figured this simple directive would be
> one of the first to be implemented. Oh well, the more you know.

Ah, is that the reason for all the �goto fallback� comments? I just
assumed that you used them to indicate that through the next series of
if-statements we would end up at the fallback-call.

> >> + if (isset($state['SPMetadata'])) {
> >> + $spMetadata = $state['SPMetadata'];
> >> + if (isset($spMetadata['attributes'])) {
> >> + SimpleSAML_Logger::debug('Negotiate - found "attributes" in SPmetadata');
> >> + $attr_list = $spMetadata['attributes'];
> >
> > You should not set $attr_list based on metadata. Doing that will fail
> > if the user accesses a different SP requiring more attributes later.
> > (Since the session will only contain the attributes required by the
> > first SP.)
>
> Ah, well how do I then respect the attributes list in the SP metadata?
> Some modules seem to disregard the list completely and I want this to
> act like my fallback module does (meaning it reads the list in metadata
> for the SP and returns those attributes).

Attribute filtering isn't generally done by the authentication modules.
It is handled by the core:AttributeLimit processing filter at a later
stage in the request processing. (So long as you have it enabled.)

[...]

> >> +?>
> >
> > The ?> at the end of the file isn't needed by PHP, and dropping it
> > allows us to not worry about (apparently) blank lines at the end of the
> > file causing output to be sent to the browser.
>
> Did not know that. Running strict mode (if that exists or will be
> implemented) won't bit me for this?

It doesn't complain here. See also the note at:

http://www.php.net/manual/en/language.basic-syntax.instruction-separation.php

(Strict mode exists - at least you can make it give you some warnings.
Just add E_STRICT to error_reporting in php.ini:

error_reporting = E_ALL | E_STRICT

)

I would suggest adding it to the state array before you call
saveState() in authenticate(), and just reading it directly from the
state array in the fallBack()-method.

Also, I wouldn't create a separate const definition for it. Just use
$state['negotiate:backend'] or something like that. The convention is
that anything starting with "<modulename>:" belongs to that module.

> Apologies for the numerous questions and comments and thanks a lot for
> all the help. Hopefully there won't be too many cycles of walls of text
> before this is completed. Meanwhile, have a great (long) weekend!

Not a problem. I don't think many cycles are required. Have a greate
weekend!

[Mathias Meisfjordskar]

[ Olav Morken ]

> On Fri, Jun 10, 2011 at 13:42:26 +0200, Mathias Meisfjordskar wrote:

> Some parts of the $state array are saved in the session after
> authentication, amongs them 'LogoutData' and 'Attributes'. Most other
> things in the $state array are temporary for the current authentication
> request.
>
> (You can manually request that other pieces of data is stored by adding
> the key to the PersistentAuthData array:
>
> $state['PersistentAuthData'][] = 'mymodule:mydata';
>
> Of course, in this case using LogoutData is probably better, since
> moving information from the authentication step to the logout step is
> exatly what it was designed for.)

I've removed all reference to SESSION_SOURCE and replaced them with
setLogoutState()/getLogoutState(). This was what you meant, I hope.

>> I don't not know but during our testing we saw that PHP never recieved
>> the full headers and stuff like HTTP_AUTHORIZATION was stripped away by
>> apache. Something about PHP acting like CGI or something like it. Either
>> way the people setting up this module should check this with the sample
>> script (and your changes).
>
> I got curious, and decided to do a quick grep over the Apache source,
> which reveals the following block of code in server/util_script.c:
>
> /*
> * You really don't want to disable this check, since it leaves you
> * wide open to CGIs stealing passwords and people viewing them
> * in the environment with "ps -e". But, if you must...
> */
> #ifndef SECURITY_HOLE_PASS_AUTHORIZATION
> else if (!strcasecmp(hdrs[i].key, "Authorization")
> || !strcasecmp(hdrs[i].key, "Proxy-Authorization")) {
> continue;
> }
> #endif
>
> I'm not sure that I agree with their reasoning here, but there isn't
> much to be done about it in any case.
>
> Btw.: I see that PHP has an environment variable $_SERVER['AUTH_TYPE'].
> Maybe it be used to bypass this restriction? (I'm not actually sure
> whether it is any more reliable though - maybe PHP only initializes it
> from the HTTP_AUTHORIZATION data.)

Right, however the problem isn't this module but rather php_krb5 which
looks for HTTP_AUTHORIZATION directly when doAuthentication() is called.
It's not fantastic design IMHO but this is what I have to work with.

I have a few suggestions I'm going to post to the author. Being able to
put the header reply as an argument to doAuthentication() is one of the
issues. The other being 'HTTP' is hardcoded as the service name
regarless of what the keytab says.

>> Ok. From my limited PHP experience, I understand that __construct() is
>> the constructor. Are you refering to the code later on? I can move those
>> bits up here.
>
> I am basically refering to all places where you have
> $this->config->get<something>().

All fixed.

>> How come? self:: will always point to
>> sspmod_negotiate_Auth_Source_Negotiate:fallBack() while using
>> $this->fallBack() will allow for sub-classing and overrides of the
>> function if needed. Isn't that a bit more dynamic design?
>>
>> If by some reason you wanted to do the above of course. Maybe it's
>> opening a can of worms down the line, I don't know.
>
> You cannot rely on overriding this function in any case, since it is
> called statically from backend.php. (I do not know whether PHP allows
> you to override static methods either. Methods are normally static to
> allow you to call them without an instance of the class available, so
> allowing them to be overridden does not really make sense.)

Ah, I see you point with fallBack(). But this affects backend.php only,
right? So the rest of the code is still sub-class enabled. I have no
strong feelings on the subject, I can change it. Are there any drawbacks
by pointing to an object's function rather than a class'?

> I don't expect the search to be moved, but moving the LDAP
> initialization should work. But after examining what we do in the
> LDAP authsources, I think I will reverse my position, to maintain
> consistency. In those modules, we don't initialize the LDAP class
> before actually using it.

Ok, keeping it.

>> Good question. I only put it there as extra debugging in case the
>> browser spazzes out and sends gibberish in the reply. I think it can be
>> safely removed but there's little harm in having it there. The test for
>> basic is just a precaution that the browser wouldn't reply with basic
>> for some strange reason. It never should. Think I should remove them?
>
> They can serve as useful debug information, but I think you should
> remove the «// goto fallback» comments from that block, since they are
> a bit misleading.

Removed.

>> I'm utterly clueless when it comes to HTML so no, no reason. I just took
>> something that worked. Will refresh work with the supplied URL?
>> www/backend.php only exists to have a fixed URL to point clients to that
>> won't answer the 401.
>
> It can be used with an URL. See the third example at:
>
> http://en.wikipedia.org/wiki/Meta_refresh#Examples
>
> An alternative would be to use window.location.replace() in javascript.
> The advantage of that is that this page will be removed from the
> history in the browser, so that the user won't land on it when using
> the back button. You would still need a link for those users with
> javascript disabled though.

Trimmed down to:

<html>
<head>
<meta http-equiv="refresh" content="0;url='.htmlspecialchars($url) .'">
<title>Redirect to login</title>
</head>
</html>

>> I could have used strstr($user, '@', TRUE) but alas, stuck on PHP
>> 5.2.something.
>
> The reason for my question was that the comment led me to believe that
> maybe the realm wasn't included in PHP >= 5.3 or something like that.
> I didn't consider that it described why the next few lines were written
> the way they were. (The next few lines look like the normal way of
> doing it in PHP for me.)
>
> If you really want to compress it into one line, I guess the following
> will work:
>
> list($uid) = explode('@', $user);
>
> (Extra values appear to be ignored by list().)
>
> Alternatively, what I sometimes do:
>
> $user = explode('@', $user, 2);
> $uid = $user[0];
>
> (For some strange reason PHP doesn't allow explode(...)[0] or
> (explode(...))[0].)

I removed the comment; it was misleading and nondescript.

I didn't change the test either as I would have to put in another
if-test to check for a hit. I would assume strpos and substr are as
optimal as you can get regarding optimalization in PHP.

>> I also found out that PHP didn't implement goto until 5.3. Talk about
>> weird development. I would have figured this simple directive would be
>> one of the first to be implemented. Oh well, the more you know.
>
> Ah, is that the reason for all the «goto fallback» comments? I just
> assumed that you used them to indicate that through the next series of
> if-statements we would end up at the fallback-call.

Yeah. When navigating through multiple layers of scope due to tests,
I like to force the outcome with gotos from time to time. It didn't come
out as bad without anyway so no harm done.

>> Ah, well how do I then respect the attributes list in the SP metadata?
>> Some modules seem to disregard the list completely and I want this to
>> act like my fallback module does (meaning it reads the list in metadata
>> for the SP and returns those attributes).
>
> Attribute filtering isn't generally done by the authentication modules.
> It is handled by the core:AttributeLimit processing filter at a later
> stage in the request processing. (So long as you have it enabled.)

I have no idea what I did wrong earlier but I removed all of the
attribute handling code and now it seems to work as intended. Cheers. I
didn't consider other sessions with different SPs.

> It doesn't complain here. See also the note at:
>
> http://www.php.net/manual/en/language.basic-syntax.instruction-separation.php
>
> (Strict mode exists - at least you can make it give you some warnings.
> Just add E_STRICT to error_reporting in php.ini:
>
> error_reporting = E_ALL | E_STRICT
>
> )

Alrighty. Removed.

As I said, I moved all of this to setLogoutState/getLogoutState. It
seems to work as intended by the set portion in authenticate is never
followed by a saveState(), neither is it in sendNegotiate() where I set
the fallback module. Implicit saveState()?

Attaching a new patch with all the changes we have discussed. $this vs.
self remains unchanged pending conclusion

[Olav Morken]

On Wed, Jun 15, 2011 at 12:20:59 +0200, Mathias Meisfjordskar wrote:
> [ Olav Morken ]
>
> > On Fri, Jun 10, 2011 at 13:42:26 +0200, Mathias Meisfjordskar wrote:
>
> > Some parts of the $state array are saved in the session after
> > authentication, amongs them 'LogoutData' and 'Attributes'. Most other
> > things in the $state array are temporary for the current authentication
> > request.
> >
> > (You can manually request that other pieces of data is stored by adding
> > the key to the PersistentAuthData array:
> >
> > $state['PersistentAuthData'][] = 'mymodule:mydata';
> >
> > Of course, in this case using LogoutData is probably better, since
> > moving information from the authentication step to the logout step is
> > exatly what it was designed for.)
>
> I've removed all reference to SESSION_SOURCE and replaced them with
> setLogoutState()/getLogoutState(). This was what you meant, I hope.

No, it wasn't. That function is only present for backwards-
compatibility with authentication handlers that don't use the
SimpleSAML_Auth_Source baseclass. I don't think it will work correctly
in your case, since the data you save with setLogoutState will be
deleted once you call completeAuth().

(See futher comments about saveState() below.)

Mainly that it could cause confusion if someone tries to override it,
since some callers would use the static version in the base class,
while some would use the subclassed version.

I still think a normal link should be included, just in case the user
has managed to disable meta refresh somehow. (I also do not know what
the behaviour of the browser is when you navigate to such a page using
the back-button.

I'm not sure what you are thinking about here? Is it not possible
to call saveState() before doing the redirects / showing the pages?

[Mathias Meisfjordskar]

[ Olav Morken ]

> On Wed, Jun 15, 2011 at 12:20:59 +0200, Mathias Meisfjordskar wrote:

>> I've removed all reference to SESSION_SOURCE and replaced them with
>> setLogoutState()/getLogoutState(). This was what you meant, I hope.
>
> No, it wasn't. That function is only present for backwards-
> compatibility with authentication handlers that don't use the
> SimpleSAML_Auth_Source baseclass. I don't think it will work correctly
> in your case, since the data you save with setLogoutState will be
> deleted once you call completeAuth().
>
> (See futher comments about saveState() below.)

Curses! And here I was thinking I was being clever.

>> Trimmed down to:
>>
>> <html>
>> <head>
>> <meta http-equiv="refresh" content="0;url='.htmlspecialchars($url) .'">
>> <title>Redirect to login</title>
>> </head>
>> </html>
>
> I still think a normal link should be included, just in case the user
> has managed to disable meta refresh somehow. (I also do not know what
> the behaviour of the browser is when you navigate to such a page using
> the back-button.

Something like this?

<html>
<head>
<meta http-equiv="refresh" content="0;url='.htmlspecialchars($url) .'">
<title>Redirect to login</title>
</head>

Your browser seems to have meta refresh disabled for some reason. Please
click <a href="'.htmlspecialchars($url).'">here</a>.
</html> ';

>> > I would suggest adding it to the state array before you call
>> > saveState() in authenticate(), and just reading it directly from the
>> > state array in the fallBack()-method.
>> >
>> > Also, I wouldn't create a separate const definition for it. Just use
>> > $state['negotiate:backend'] or something like that. The convention is
>> > that anything starting with "<modulename>:" belongs to that module.
>>
>> As I said, I moved all of this to setLogoutState/getLogoutState. It
>> seems to work as intended by the set portion in authenticate is never
>> followed by a saveState(), neither is it in sendNegotiate() where I set
>> the fallback module. Implicit saveState()?
>
> I'm not sure what you are thinking about here? Is it not possible
> to call saveState() before doing the redirects / showing the pages?

It sure is, and I'm not sure what's going on as I was sure what I tested
was going to fail because of the missing saveState(). It doesn't but I
want this to be bullet-proof so I need to rewrite it.

I'm feeling a bit dense at the moment. Should I set 'LogoutState'
manually in $state (which I thought setLogoutState did) and call
saveState() or make a new module prefixed entry to store it in? You also
talked 'PersistentAuthData'. Use that instead?

I have three places where the client will enter the code:

* authenticate() - twice iff Krb
* fallBack() - Client followed a link provided by me
* logout() - Client wishes to terminate session

I have three places where the client "leaves" the module:

* authenticate() - due to success
* sendNegotiate() - client gets a 401
* logout() - termination

So I should save the state all the times the client leaves SSP, right?
And look for state every time the client enter the module again, in case
it's a redirect. authenticate() with it's 401 has worked from the get
go, but I have no idea if it somehow picks up the session it left or
starts a new one when 401 is provided.

I have no clue why some modules seem to rely heavily on
SimpleSAML_Auth_State and others SimpleSAML_Session which has
complicated things. I'm sure the fault is mine but it means more
hand-holding on your part I'm afraid. :)

A related subject:

With this module, all your Windows (and OSX, *nix if in the domain)
clients will simply get a SAML session as if they were already logged in
when the SP sends the client to the IdP. This creates a new set of
problems with people wishing to log in with different users. Person A's
desktop can't be lent out to someone else for web usage. Person A gets
logged in no matter what.

In the scope of my little project here this is not prioritized. We can
survive with the few cases where this becomes a problem. Long term I
wish to remedy this. I was thinking of having an optional information
page displayed if configured that allows you to disrupt the automatic
login. Something like a page displaying what is happening, a countdown
and a massive "Log in with a different account" button or something like
it. Any thoughts on this?

[Olav Morken]

On Thu, Jun 16, 2011 at 14:46:20 +0200, Mathias Meisfjordskar wrote:
> >> Trimmed down to:
> >>
> >> <html>
> >> <head>
> >> <meta http-equiv="refresh" content="0;url='.htmlspecialchars($url) .'">
> >> <title>Redirect to login</title>
> >> </head>
> >> </html>
> >
> > I still think a normal link should be included, just in case the user
> > has managed to disable meta refresh somehow. (I also do not know what
> > the behaviour of the browser is when you navigate to such a page using
> > the back-button.
>
> Something like this?
>
> <html>
> <head>
> <meta http-equiv="refresh" content="0;url='.htmlspecialchars($url) .'">
> <title>Redirect to login</title>
> </head>
> Your browser seems to have meta refresh disabled for some reason. Please
> click <a href="'.htmlspecialchars($url).'">here</a>.
> </html> ';

Yes, something like that. You could throw in a <body>-element there
if you're feeling kind to the browsers, but I suspect that they won't
care :)

> >> > I would suggest adding it to the state array before you call
> >> > saveState() in authenticate(), and just reading it directly from the
> >> > state array in the fallBack()-method.
> >> >
> >> > Also, I wouldn't create a separate const definition for it. Just use
> >> > $state['negotiate:backend'] or something like that. The convention is
> >> > that anything starting with "<modulename>:" belongs to that module.
> >>
> >> As I said, I moved all of this to setLogoutState/getLogoutState. It
> >> seems to work as intended by the set portion in authenticate is never
> >> followed by a saveState(), neither is it in sendNegotiate() where I set
> >> the fallback module. Implicit saveState()?
> >
> > I'm not sure what you are thinking about here? Is it not possible
> > to call saveState() before doing the redirects / showing the pages?
>
> It sure is, and I'm not sure what's going on as I was sure what I tested
> was going to fail because of the missing saveState(). It doesn't but I
> want this to be bullet-proof so I need to rewrite it.
>
> I'm feeling a bit dense at the moment. Should I set 'LogoutState'
> manually in $state (which I thought setLogoutState did) and call
> saveState() or make a new module prefixed entry to store it in? You also
> talked 'PersistentAuthData'. Use that instead?

I think the fault here is at least partially mine, since I was not
consistent. I think the best approach is to save it in 'LogoutState'.
Just remember that it needs to be an array. Something like this should
work:

$state['LogoutState'] = array(
'negotiate:backend' => $this->backend,
);

This should cause the $state array passed to your logout()-function
to contain a 'negotiate:backend' entry. (And it should also be
available as $state['LogoutState']['negotiate:backend'] in fallback().)

> I have three places where the client will enter the code:
>
> * authenticate() - twice iff Krb
> * fallBack() - Client followed a link provided by me
> * logout() - Client wishes to terminate session
>
> I have three places where the client "leaves" the module:
>
> * authenticate() - due to success
> * sendNegotiate() - client gets a 401
> * logout() - termination
>
> So I should save the state all the times the client leaves SSP, right?
> And look for state every time the client enter the module again, in case
> it's a redirect. authenticate() with it's 401 has worked from the get
> go, but I have no idea if it somehow picks up the session it left or
> starts a new one when 401 is provided.

I think the answer is that you need to save the state any time you want
to be able to read it back. Your module only has one loadState()-call
(in backend.php). The only way an user can arrive on backend.php is
through the sendNegotiate()-function, which means that this is the only
place you need to call saveState().

Once the state array is passed to another module, that module becomes
responsible for saving and restoring it.

As for the 401-error, I believe the browser will resend the original
request, which basically means that the entire authentication process
is restarted.

> I have no clue why some modules seem to rely heavily on
> SimpleSAML_Auth_State and others SimpleSAML_Session which has
> complicated things. I'm sure the fault is mine but it means more
> hand-holding on your part I'm afraid. :)

The direct use of the Session-class in some locations is unfortunate,
since it can cause conflicts in some situations, e.g. if the user
is working through two logins at the same time.

Unfortunately, I do not really have the time to go through those
locations and attempt to fix it up.

> A related subject:
>
> With this module, all your Windows (and OSX, *nix if in the domain)
> clients will simply get a SAML session as if they were already logged in
> when the SP sends the client to the IdP. This creates a new set of
> problems with people wishing to log in with different users. Person A's
> desktop can't be lent out to someone else for web usage. Person A gets
> logged in no matter what.
>
> In the scope of my little project here this is not prioritized. We can
> survive with the few cases where this becomes a problem. Long term I
> wish to remedy this. I was thinking of having an optional information
> page displayed if configured that allows you to disrupt the automatic
> login. Something like a page displaying what is happening, a countdown
> and a massive "Log in with a different account" button or something like
> it. Any thoughts on this?

A splash-page would work. Just remember to include a "continue"-button
to avoid annoying those that are impatient :)

An alternative would be to display a login page with the username and a
fake password filled in, with focus moved to the login-button, and a
timer counting down (maybe embedded in the login button?). In that case
you would (of course) need to stop the timer as soon as the user takes
any action on the page (e.g. moving focus to the username field,
pressing the esc-button).

This would unfortunately require a few extensions so that the username
& password can be passed directly to the backend authsources. This is
something that I started before, but haven't committed yet, since there
isn't a user for that code yet.

[Mathias Meisfjordskar]

[ Olav Morken ]

> On Thu, Jun 16, 2011 at 14:46:20 +0200, Mathias Meisfjordskar wrote:

>> <html>
>> <head>
>> <meta http-equiv="refresh" content="0;url='.htmlspecialchars($url) .'">
>> <title>Redirect to login</title>
>> </head>
>> Your browser seems to have meta refresh disabled for some reason. Please
>> click <a href="'.htmlspecialchars($url).'">here</a>.
>> </html> ';
>
> Yes, something like that. You could throw in a <body>-element there
> if you're feeling kind to the browsers, but I suspect that they won't
> care :)

Heh, indeed. I put some in just in case.

>> I'm feeling a bit dense at the moment. Should I set 'LogoutState'
>> manually in $state (which I thought setLogoutState did) and call
>> saveState() or make a new module prefixed entry to store it in? You also
>> talked 'PersistentAuthData'. Use that instead?
>
> I think the fault here is at least partially mine, since I was not
> consistent. I think the best approach is to save it in 'LogoutState'.
> Just remember that it needs to be an array. Something like this should
> work:
>
> $state['LogoutState'] = array(
> 'negotiate:backend' => $this->backend,
> );
>
> This should cause the $state array passed to your logout()-function
> to contain a 'negotiate:backend' entry. (And it should also be
> available as $state['LogoutState']['negotiate:backend'] in fallback().)

Great! I'll rewrite all the state handling to this. Told you I sucked at
this.

>> I have three places where the client will enter the code:
>>
>> * authenticate() - twice iff Krb
>> * fallBack() - Client followed a link provided by me
>> * logout() - Client wishes to terminate session
>>
>> I have three places where the client "leaves" the module:
>>
>> * authenticate() - due to success
>> * sendNegotiate() - client gets a 401
>> * logout() - termination
>>
>> So I should save the state all the times the client leaves SSP, right?
>> And look for state every time the client enter the module again, in case
>> it's a redirect. authenticate() with it's 401 has worked from the get
>> go, but I have no idea if it somehow picks up the session it left or
>> starts a new one when 401 is provided.
>
> I think the answer is that you need to save the state any time you want
> to be able to read it back. Your module only has one loadState()-call
> (in backend.php). The only way an user can arrive on backend.php is
> through the sendNegotiate()-function, which means that this is the only
> place you need to call saveState().

The one I have is more of reading other peoples code than me figuring
this out. I don't have to save state when I do 401 or exit due to
success then?

Is SimpleSAML_Auth_Source::completeAuth($state); the correct way to
handle and finish an authN attempt by the way? I assumed so after
reading other modules.

> Once the state array is passed to another module, that module becomes
> responsible for saving and restoring it.

I didn't think of this but you're right. I'm passing $state along.

> As for the 401-error, I believe the browser will resend the original
> request, which basically means that the entire authentication process
> is restarted.

Is it? I don't know the inner workings of SSP but after the initial
request by the SP, the client gets redirected to SSOservice.php with a
numerous arguments. The 401 means the client GETs the same URL. The logs
show two lines like this:

Jun 16 16:37:51 simplesamlphp INFO [f206a0bfe0] SAML2.0 - IdP.SSOService: Accessing SAML 2.0 IdP endpoint SSOService

per successful Krb authN. The session ID or log ID or whatever it is
remains the same. Is this the current "session"?

It may be irrelevant but like I said - I want bullet-proof. I don't want
sloppy programming or my lack of understanding to cripple our local IdP.

>> I have no clue why some modules seem to rely heavily on
>> SimpleSAML_Auth_State and others SimpleSAML_Session which has
>> complicated things. I'm sure the fault is mine but it means more
>> hand-holding on your part I'm afraid. :)
>
> The direct use of the Session-class in some locations is unfortunate,
> since it can cause conflicts in some situations, e.g. if the user is
> working through two logins at the same time.
>
> Unfortunately, I do not really have the time to go through those
> locations and attempt to fix it up.

Oh. Does this mean my usage of SimpleSAML_Session::getInstance(); is all
wrong? It may be a moot point as most (all?) my calls to the above are
going to be replaced by direct access to $state. Still, in case not
every call goes away, I want to give away tidy code so the future
clean-up doesn't become even bigger because of me.

>> A related subject:
>>
>> With this module, all your Windows (and OSX, *nix if in the domain)
>> clients will simply get a SAML session as if they were already logged in
>> when the SP sends the client to the IdP. This creates a new set of
>> problems with people wishing to log in with different users. Person A's
>> desktop can't be lent out to someone else for web usage. Person A gets
>> logged in no matter what.
>>
>> In the scope of my little project here this is not prioritized. We can
>> survive with the few cases where this becomes a problem. Long term I
>> wish to remedy this. I was thinking of having an optional information
>> page displayed if configured that allows you to disrupt the automatic
>> login. Something like a page displaying what is happening, a countdown
>> and a massive "Log in with a different account" button or something like
>> it. Any thoughts on this?
>
> A splash-page would work. Just remember to include a "continue"-button
> to avoid annoying those that are impatient :)

Good point.

> An alternative would be to display a login page with the username and a
> fake password filled in, with focus moved to the login-button, and a
> timer counting down (maybe embedded in the login button?). In that case
> you would (of course) need to stop the timer as soon as the user takes
> any action on the page (e.g. moving focus to the username field,
> pressing the esc-button).

That would look cool indeed but the final login page is completely
unknown to Negotiate. I also think you would shorten the life of all
security people seeing this for the first time by ten years. I know I
would choke on my coffee seeing this. ;)

> This would unfortunately require a few extensions so that the username
> & password can be passed directly to the backend authsources. This is
> something that I started before, but haven't committed yet, since there
> isn't a user for that code yet.

This is not usable for me as I have no password to begin with. I also
want Negotiate to be compatible with _any_ module. In theory I could
extend Kerberos domains in Negotiate by setting several different
Negotiate setups in authsources and having them chain eachother by
pointing to the next step in fallback. (I won't so this, I promise!)

Perhaps if all modules had to implement a function which took u/p as
arguments and did the actual authN? Sounds more SSP 2.X or even 3.X as
it would demand massive redesigning. Guessing there's a lot of business
logic tied up in various other functions in modules. Not to mention
password-less authNs like Negotiate.

I see Negotiate as sort of a bastard, or a supplement in nicer terms,
SSP module. It doesn't really work as a standalone module due to all the
limitations of domains (and rightfully so). Multiauth is kinda like
this but even more meta as it doesn't handle any authN on it's own.

[Olav Morken]

On Thu, Jun 16, 2011 at 17:05:47 +0200, Mathias Meisfjordskar wrote:
> [ Olav Morken ]

> > I think the answer is that you need to save the state any time you want
> > to be able to read it back. Your module only has one loadState()-call
> > (in backend.php). The only way an user can arrive on backend.php is
> > through the sendNegotiate()-function, which means that this is the only
> > place you need to call saveState().
>
> The one I have is more of reading other peoples code than me figuring
> this out. I don't have to save state when I do 401 or exit due to
> success then?

I believe the 401-error redirects to backend.php, so you need to save
state before showing it. But when you leave the module after successful
authentication you don't need to save the state.

> Is SimpleSAML_Auth_Source::completeAuth($state); the correct way to
> handle and finish an authN attempt by the way? I assumed so after
> reading other modules.

It is the way that always works. You are also allowed to return
directly from the authenticate function (in the cases where that is
still possible), but you are not required to, and can call completeAuth
instead.

> > As for the 401-error, I believe the browser will resend the original
> > request, which basically means that the entire authentication process
> > is restarted.
>
> Is it? I don't know the inner workings of SSP but after the initial
> request by the SP, the client gets redirected to SSOservice.php with a
> numerous arguments. The 401 means the client GETs the same URL. The logs
> show two lines like this:
>
> Jun 16 16:37:51 simplesamlphp INFO [f206a0bfe0] SAML2.0 - IdP.SSOService: Accessing SAML 2.0 IdP endpoint SSOService
>
> per successful Krb authN.

Yes, if you check your access log, I think you will find two requests
to SSOService.php, one of which returns a 401 status code, and the other
returning a redirect.

Having two requests to SSOService is slightly unfortunate, since it
means that we parse the request twice, but I wouldn't worry too much
about it yet. A future enhancement could be to do a redirect to an
auth-check page, and have that page trigger the authentication.

> The session ID or log ID or whatever it is
> remains the same. Is this the current "session"?

It is the "tracking number" from the current session. Mainly included
to make it possible to debug errors that occur during login.

> > The direct use of the Session-class in some locations is unfortunate,
> > since it can cause conflicts in some situations, e.g. if the user is
> > working through two logins at the same time.
> >
> > Unfortunately, I do not really have the time to go through those
> > locations and attempt to fix it up.
>
> Oh. Does this mean my usage of SimpleSAML_Session::getInstance(); is all
> wrong? It may be a moot point as most (all?) my calls to the above are
> going to be replaced by direct access to $state. Still, in case not
> every call goes away, I want to give away tidy code so the future
> clean-up doesn't become even bigger because of me.

Using the session class directly isn't necessarily wrong, but care must
be taken to avoid potential conflicts. If it is possible to stash the
data in the state array instead, that is usually a simpler solution,
since is per-request, and you have to carry it around in any case.

> > An alternative would be to display a login page with the username and a
> > fake password filled in, with focus moved to the login-button, and a
> > timer counting down (maybe embedded in the login button?). In that case
> > you would (of course) need to stop the timer as soon as the user takes
> > any action on the page (e.g. moving focus to the username field,
> > pressing the esc-button).
>
> That would look cool indeed but the final login page is completely
> unknown to Negotiate.

Yes, that would require some assumptions to be made (i.e. that the
final login handler takes a simple username & password, and that it
implements some interface for receiving the username & password
without displaying a login-page).

> I also think you would shorten the life of all
> security people seeing this for the first time by ten years. I know I
> would choke on my coffee seeing this. ;)

True, it does look scary if you don't know what is going on. I was
thinking of it as a way to avoid having to explain what is going on to
the user on the spash page, but it may actually just create more
confusion.

> > This would unfortunately require a few extensions so that the username
> > & password can be passed directly to the backend authsources. This is
> > something that I started before, but haven't committed yet, since there
> > isn't a user for that code yet.
>
> This is not usable for me as I have no password to begin with. I also
> want Negotiate to be compatible with _any_ module. In theory I could
> extend Kerberos domains in Negotiate by setting several different
> Negotiate setups in authsources and having them chain eachother by
> pointing to the next step in fallback. (I won't so this, I promise!)

I'm sorry - I meant "fallback" authsource, not "backend". I was
thinking of the negotiate module passing the username and password
to the fallback authsource if the user didn't use kerberos
authentication.

> Perhaps if all modules had to implement a function which took u/p as
> arguments and did the actual authN? Sounds more SSP 2.X or even 3.X as
> it would demand massive redesigning. Guessing there's a lot of business
> logic tied up in various other functions in modules. Not to mention
> password-less authNs like Negotiate.

Actually, the patch I have is relatively simple, though it only works
for authsources that subclass UserPassBase. (Happily, this covers most
of them.) Since the interface in UserPassBase already requires a
function that takes username & password, all we need to do is make it
possible to call that function directly from the authenticate()-
function with username & password from the $state-array.

> I see Negotiate as sort of a bastard, or a supplement in nicer terms,
> SSP module. It doesn't really work as a standalone module due to all the
> limitations of domains (and rightfully so). Multiauth is kinda like
> this but even more meta as it doesn't handle any authN on it's own.

I agree with this design.

[Mathias Meisfjordskar]

[ Olav Morken ]

> On Thu, Jun 16, 2011 at 17:05:47 +0200, Mathias Meisfjordskar wrote:

>> The one I have is more of reading other peoples code than me figuring
>> this out. I don't have to save state when I do 401 or exit due to
>> success then?
>
> I believe the 401-error redirects to backend.php, so you need to save
> state before showing it. But when you leave the module after successful
> authentication you don't need to save the state.

The refresh in the 401 body redirects to backend.php, yes. I _think_ the
state stuff is handled correctly now but I've gotten a strange error.
I'll explain later on.

>> Is SimpleSAML_Auth_Source::completeAuth($state); the correct way to
>> handle and finish an authN attempt by the way? I assumed so after
>> reading other modules.
>
> It is the way that always works. You are also allowed to return
> directly from the authenticate function (in the cases where that is
> still possible), but you are not required to, and can call completeAuth
> instead.

I've inserted a TBD in the code since I don't really know what to do
here. I copied some code from another module and started thinking of
cases where authenticate would return without a success. Just remove the
loginCompleted()?

>> > As for the 401-error, I believe the browser will resend the original
>> > request, which basically means that the entire authentication process
>> > is restarted.
>>
>> Is it? I don't know the inner workings of SSP but after the initial
>> request by the SP, the client gets redirected to SSOservice.php with a
>> numerous arguments. The 401 means the client GETs the same URL. The logs
>> show two lines like this:
>>
>> Jun 16 16:37:51 simplesamlphp INFO [f206a0bfe0] SAML2.0 - IdP.SSOService: Accessing SAML 2.0 IdP endpoint SSOService
>>
>> per successful Krb authN.
>
> Yes, if you check your access log, I think you will find two requests
> to SSOService.php, one of which returns a 401 status code, and the other
> returning a redirect.
>
> Having two requests to SSOService is slightly unfortunate, since it
> means that we parse the request twice, but I wouldn't worry too much
> about it yet. A future enhancement could be to do a redirect to an
> auth-check page, and have that page trigger the authentication.

We've discussed this in other channels but for now I have to get this
out the door and into large scale testing.

As a future enhancement Negotiate should redirect the client to a fixed
URL so a reply to the 401 doesn't trigger a new session.

>> > The direct use of the Session-class in some locations is unfortunate,
>> > since it can cause conflicts in some situations, e.g. if the user is
>> > working through two logins at the same time.
>> >
>> > Unfortunately, I do not really have the time to go through those
>> > locations and attempt to fix it up.
>>
>> Oh. Does this mean my usage of SimpleSAML_Session::getInstance(); is
>> all wrong? It may be a moot point as most (all?) my calls to the
>> above are going to be replaced by direct access to $state. Still, in
>> case not every call goes away, I want to give away tidy code so the
>> future clean-up doesn't become even bigger because of me.
>
> Using the session class directly isn't necessarily wrong, but care
> must be taken to avoid potential conflicts. If it is possible to stash
> the data in the state array instead, that is usually a simpler
> solution, since is per-request, and you have to carry it around in any
> case.

All usage of SimpleSAML_Session::getInstance() removed in favor of using
the $state array.

So, to summarize for the list, all of Olav's suggestions have gone into
the patch and I fixed a bug that was causeing $state['LogoutState'] to
disappear when logout() was called. All good. Somehow this bug has
showed up again when I use an SP from a different host. My test SP has
been on the same host as my IdP because it was easy to set up. I don't
understand how. Both SPs authN using Kerberos so it's not related to
backend.php. I also kept the login and logout sessions separate so they
wouldn't interere.

I'm seeing some weird behaviour when using the same client against the
same IdP. It seems the IdP is requesting that the client log in every
time I hit the IdP. Not all of the time. If I go back to the "SAML
installation page" and select the IdP again I get SSO. If I use another
SP I or if I go back the the disco service in the original SP they both
do proper Krb authNs. Related to the problem in any way?

[Søren Grønning Iversen]

Hi guys,

How's this module progressing/working?

It would be totally awesome to be able to combine the ordinary LDAP backend with a Kerberos mechanism like this work seems to imply :)

Best regards,

Søren

[Mathias Meisfjordskar]

[ Søren Grønning Iversen ]

> Hi guys,
>
> How's this module progressing/working?
>
> It would be totally awesome to be able to combine the ordinary LDAP
> backend with a Kerberos mechanism like this work seems to imply :)

Hello Søren.

Apologies for seemingly dropping the ball here but I resolved the last
issues I had with Olav through other channels.

The module is now in testing here at the University of Oslo. Due to the
summer we haven't had the time or manpower to conclude anything. What we
know is that the module works great... if the browsers are set up
correctly.

Most browsers will "fail" gracefully back to the fallback auth mechanism
if the browser doesn't know what to do about the Negotiate Auth request.
The exception is various versions of IE that will freak out and try to
fall back to NTLM. This is a big problem for common users as they will
be presented with a Windows login box that will always fail (naturally,
the module handles only Kerberos). If the user clicks "cancel", IE falls
back to the fallback auth mechanism. Hardly ideal.

What we are planning is locking down the preferences even more for
stationary machines connected to the domain. With our IdP in "Trusted
sites" in "Local Intranet" in the settings, we avoid NTLM altogether.
The people handling user software are looking at ways to ship the
correct Firefox settings too and other browsers will probably get a DIY
howto help page.

Including the latest patch with the small fix that Olav pointed out.
This is the version we are running in our test. Try it out, look for
bugs or just drop a comment if you have one.

[Søren Grønning Iversen]

Hi Mathias

Thanks a lot for the detailed status report on this interesting project!

I'm implementing simpleSAMLphp at Designskolen Kolding in Denmark and I'm very excited about the prospects of true SSO throughout the organization - domain AND web!

I will definately take the meassures mentioned into consideration and take the module for a spin and report back with any comments and thumbs up ;)

Best regards,

Søren

[E L]

Hi,
That module looks very interesting.
But I saw that you need to configure the ldap both as the fall back and as a different entry.
Can I fall back to a multi-ldap that is configured elsewhere in the file?

Thanks,

--
Mathias Meisfjordskar
GNU/Linux addict.

"If it works; HIT IT AGAIN!"

--
You received this message because you are subscribed to the Google Groups "simpleSAMLphp" group.
To post to this group, send email to simple...@googlegroups.com.
To unsubscribe from this group, send email to simplesamlph...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/simplesamlphp?hl=en.

[Mathias Meisfjordskar]

[ E L ]

> Hi,
> That module looks very interesting.
> But I saw that you need to configure the ldap both as the fall back and as
> a different entry.
> Can I fall back to a multi-ldap that is configured elsewhere in the file?

Hello.

The LDAP configuration in the negotiate directive is for authorization
purposes. If you want to filter out shared accounts or admin accounts
because they too get Kerberos tickets. The LDAP server in this config
can be a different one than your AD LDAP server. We have this setup
where AD is filled with "accounts" and an OpenLDAP server handles
federated "users". (I should probably add a filter to this section at
some point to be able to do proper filtering instead of looking at
entire trees)

The fallback mechanism can be pretty much any SimpleSamlPhp auth module.
I toyed with the idea of having another negotiate module after the first
to handle several Kerberos realms. We use a proprietary fallback module.
You can use ldap-multi, CAS, SAML or whatever module that supports authN.

[E L]

Thanks for the answer,
I got the module to work.
But I did notice a problem, you can't really logout. If I want to logout and login as a different user
there is no way to do it. Furthermore if I try to use 'ForceAuthn' => true to require reauthentication
it goes into infinite loop of login.

I think to that solve this problem you should use the fall back if ForceAuthn is true.
I'm not sure how the logout or login as a different user problem can be solved.

[Mathias Meisfjordskar]

On Sun, Aug 21, 2011 at 3:44 PM, E L <nak...@gmail.com> wrote:

Thanks for the answer,
I got the module to work.
But I did notice a problem, you can't really logout. If I want to logout and login as a different user
there is no way to do it. Furthermore if I try to use 'ForceAuthn' => true to require reauthentication
it goes into infinite loop of login.

I think to that solve this problem you should use the fall back if ForceAuthn is true.
I'm not sure how the logout or login as a different user problem can be solved.

You can log out in the sense that you can terminate the SAML session but if the SP or IdP redirects you to the login page, you are automatically logged in again. I'm aware of this issue but it's just one of these issues you can't solve easily. I'm thinking of adding a splash screen during the Negotiate login process to allow breaking the automatic Kerberos login but that's in the future. Perhaps even try to remember the choice by saving a cookie like WAYFs try to remember your IdP. It's just an idea at them moment so I'm open to suggestions.

Due to the SSO nature of the module, multiple accounts is a problem. The only way to address this currently is to either filter out the machine in the IP address list or using a desktop account without federation credentials. Neither is very flexible.

--
Mathias

[E L]

How about the following suggestions:

- Make ForceAuthn go to the fallback
- Have a ForceAuthn page called switch-user which will use the next authentication method allowing you to
switch user.

That assuming that once you logined in as someone else the negotiate module won't kick in anymore.

nakee

--

[Mathias Meisfjordskar]

On Mon, Aug 22, 2011 at 9:24 AM, E L <nak...@gmail.com> wrote:

How about the following suggestions:

- Make ForceAuthn go to the fallback
- Have a ForceAuthn page called switch-user which will use the next authentication method allowing you to
switch user.

That assuming that once you logined in as someone else the negotiate module won't kick in anymore.

Maybe. The details on how to solve this problem are yet to be decided. The main problem now is how to enable the user to abort a login without ruining the otherwise automatic SSO process. Prompts can be as annoying as having to type your u/p.

I'm thinking a countdown once you do a Negotiate authN with some help text and link or two to be able to interrupt Negotiate. I have to air this idea to the web developers here and check if they have comments or another suggestion.
 
Negotiate will only kick in if SSP doesn't have a SAML session with the user. Once you have that, Negotiate is ignored.

--
Mathias

[mschot]

Hi Mathias,

First of all thank you for a fantastic module. I got it to work in IE,
FF and chrome(did not need to change a single thing) but safari 5.1(on
Windows 7) seems to fail unfortunatly.

It fails on $auth->doAuthentication() with an error of: An
unsupported mechanism was requested (65536,0)

Is this a problem in the php_krb5 module or could there be another
explanation?

For now I have put a try & catch around if($reply = $auth-
>doAuthentication()) { rest of code } to catch the exception and fall
back to the ldap module.

below an excerpt from my logs:

Aug 19 13:44:49 simplesamlphp DEBUG [5ddcf01dc1] Session: 'weblogin'
not valid because we are not authenticated.
Aug 19 13:44:49 simplesamlphp DEBUG [5ddcf01dc1] Negotiate -
authenticate(): looking for Negotate
Aug 19 13:44:49 simplesamlphp DEBUG [5ddcf01dc1] Negotiate -
authenticate(): Negotate found
Aug 19 13:44:49 simplesamlphp DEBUG [5ddcf01dc1] Library - LDAP
__construct(): Setup LDAP with host=ad.mydomain.com', tls=false,
debug=false, timeout=30
Aug 19 13:44:49 simplesamlphp ERROR [5ddcf01dc1]
SimpleSAML_Error_Exception: Error 2 -
KRB5NegotiateAuth::doAuthentication(): An unsupported mechanism was
requested (65536,0)
Aug 19 13:44:49 simplesamlphp ERROR [5ddcf01dc1] Backtrace:
Aug 19 13:44:49 simplesamlphp ERROR [5ddcf01dc1] 8 /usr/share/
simplesamlphp/www/_include.php:59 (SimpleSAML_error_handler)
Aug 19 13:44:49 simplesamlphp ERROR [5ddcf01dc1] 7 [builtin]
(KRB5NegotiateAuth::doAuthentication)
Aug 19 13:44:49 simplesamlphp ERROR [5ddcf01dc1] 6 /custom/modules/
negotiate/lib/Auth/Source/Negotiate.php:98
(sspmod_negotiate_Auth_Source_Negotiate::authenticate)
Aug 19 13:44:49 simplesamlphp ERROR [5ddcf01dc1] 5 /usr/share/
simplesamlphp/lib/SimpleSAML/Auth/Default.php:58
(SimpleSAML_Auth_Default::initLogin)
Aug 19 13:44:49 simplesamlphp ERROR [5ddcf01dc1] 4 /usr/share/
simplesamlphp/lib/SimpleSAML/Auth/Simple.php:127
(SimpleSAML_Auth_Simple::login)
Aug 19 13:44:49 simplesamlphp ERROR [5ddcf01dc1] 3 /usr/share/
simplesamlphp/lib/SimpleSAML/IdP.php:332
(SimpleSAML_IdP::authenticate)
Aug 19 13:44:49 simplesamlphp ERROR [5ddcf01dc1] 2 /usr/share/
simplesamlphp/lib/SimpleSAML/IdP.php:367
(SimpleSAML_IdP::handleAuthenticationRequest)
Aug 19 13:44:49 simplesamlphp ERROR [5ddcf01dc1] 1 /usr/share/
simplesamlphp/modules/saml/lib/IdP/SAML2.php:343
(sspmod_saml_IdP_SAML2::receiveAuthnRequest)
Aug 19 13:44:49 simplesamlphp ERROR [5ddcf01dc1] 0 /usr/share/
simplesamlphp/www/saml2/idp/SSOService.php:19 (N/A)
Aug 19 13:44:49 simplesamlphp DEBUG [5ddcf01dc1] Negotiate -
authenticate(): Caught exception: Error while accepting security
context now going to fallback
Aug 19 13:44:49 simplesamlphp INFO [5ddcf01dc1] Negotiate -

authenticate(): Client failed Negotiate. Falling back

Would appreciate any feedback, thanks again.

Martijn Schot


On Aug 19, 8:51 am, Mathias Meisfjordskar

[Mathias Meisfjordskar]

On Fri, Aug 19, 2011 at 3:34 PM, mschot <m...@olute.co.uk> wrote:

Hi Mathias,

First of all thank you for a fantastic module. I got it to work in IE,
FF and chrome(did not need to change a single thing) but safari 5.1(on
Windows 7) seems to fail unfortunatly.

It fails on $auth->doAuthentication() with an error of:  An
unsupported mechanism was requested (65536,0)

Is this a problem in the php_krb5 module or could there be another
explanation?

For now I have put a try & catch around if($reply = $auth-
>doAuthentication()) { rest of code } to catch the exception and fall
back to the ldap module.

Hello.

I bet you've stumbled upon a missing feature or bug in php_krb5. During development I added tons of debug statements to php_krb5 to get some insight into the process. Most of the errors occurred when the client failed to transmit the header correctly.

I've never tested Safari on Windows. Do have an option to display all headers being sent and received? If not, try printing the header in Negotiate and see what Safari sends. php_krb5 is still very light on features and is not very robust. I should probably e-mail the developer and at least tell him someone is using it.

Could be I should modify the code to support doAuth failing. Same with getAuthenticatedUser(). At least clients failing would get redirected to the backend. 

--
I am root! Bow before me!

[E L]

I think the right thing is to respect forceauthn:
- It's important for security (Someone might leave his computer open and you don't want to give anyone free access for everything)
- It solved the logout problem.
- It solved the login as someone else problem.
 
I added the following lines to the authenticate function:

    if ($state['ForceAuthn']) {
            $this->fallBack($state);
        }
       
I think that should at least be a configuration option e.g. respectforceauthn

Thanks for the great module! it worked flawlessy so far with whatever I threw at it:)

nakee

Mathias

--

[E L]

Hi,
Did you figure out how to avoid the jumping window problem with IE?

2011/8/4 Søren Grønning Iversen <s.gro...@gmail.com>

[Mathias Meisfjordskar]

On Thu, Aug 25, 2011 at 12:23 PM, E L <nak...@gmail.com> wrote:

I think the right thing is to respect forceauthn:
- It's important for security (Someone might leave his computer open and you don't want to give anyone free access for everything)
- It solved the logout problem.
- It solved the login as someone else problem.

From the SSP doc:

ForceAuthn

Force authentication allows you to force re-authentication of users even if the user has a SSO session at the IdP.

Note: SAML 2 specific.

Strictly speaking, enabling ForceAuthn would only make Negotiate go through a new round of Kerberos authN as ForceAuthn only tells the IdP to re-authenticate the user.

It doesn't really help if someone leaves the computer as ForceAuthn isn't set automatically instantly. It can't be, with this solution Negotiate would never be used. I'm also aware that Negotiate boarders on being a gaping security hole but what I think and what the brass tell me to do don't always converge.

It sort of solves the logout problem but only for as long as ForceAuthn is set.

It solves the login-as-different-user issue if it's intuitive for the users on how to set ForceAuthn.
 

I added the following lines to the authenticate function:

    if ($state['ForceAuthn']) {
            $this->fallBack($state);
        }
       
I think that should at least be a configuration option e.g. respectforceauthn

I see what you did here and it's an interesting solution to the problem but how do you set the parameter for the user? Links in each of the SPs?

My gripe with this is that I want to control everything on server-side (IdP in this case) as a lot of service providers (the actual providers, not the service) don't understand SAML and SAML services well enough to hack something like this. And then there's proprietary solutions where your hands are tied completely. My experience with SAML is that to get it implemented over something more basic like LDAP, it has to be simplistic. Not that people get LDAP right but it's very common to support out of the box.
 
I'm going to run the scenario by our web developers soon and hopefully come up with a flexible, secure and usable solution. ForceAuthn hadn't really crossed my mind but I'll note it down. If my idea isn't viable, ForceAuthn might be.

Thanks for the great module! it worked flawlessy so far with whatever I threw at it:)

Thank you for testing it and commenting. :)

[E L]

Hi,

On Mon, Aug 29, 2011 at 3:49 PM, Mathias Meisfjordskar <mathias.me...@gmail.com> wrote:

On Thu, Aug 25, 2011 at 12:23 PM, E L <nak...@gmail.com> wrote:

I think the right thing is to respect forceauthn:
- It's important for security (Someone might leave his computer open and you don't want to give anyone free access for everything)
- It solved the logout problem.
- It solved the login as someone else problem.

From the SSP doc:

ForceAuthn

Force authentication allows you to force re-authentication of users even if the user has a SSO session at the IdP.

Note: SAML 2 specific.

Strictly speaking, enabling ForceAuthn would only make Negotiate go through a new round of Kerberos authN as ForceAuthn only tells the IdP to re-authenticate the user.

It doesn't really help if someone leaves the computer as ForceAuthn isn't set automatically instantly. It can't be, with this solution Negotiate would never be used. I'm also aware that Negotiate boarders on being a gaping security hole but what I think and what the brass tell me to do don't always converge.

Yes it wont be used for that specific page.
 

It sort of solves the logout problem but only for as long as ForceAuthn is set.

Well, you can logout, and you can login as a different user using that special page. Of course if you just go again
to the login page it will use negotiate again. I think that makes sense.
 

It solves the login-as-different-user issue if it's intuitive for the users on how to set ForceAuthn.

Yes, I created a login as different user page, so when someone logs out he is being sent to there.
That page is using ForceAuthn.
 

 

I added the following lines to the authenticate function:

    if ($state['ForceAuthn']) {
            $this->fallBack($state);
        }
       
I think that should at least be a configuration option e.g. respectforceauthn

I see what you did here and it's an interesting solution to the problem but how do you set the parameter for the user? Links in each of the SPs?

My gripe with this is that I want to control everything on server-side (IdP in this case) as a lot of service providers (the actual providers, not the service) don't understand SAML and SAML services well enough to hack something like this. And then there's proprietary solutions where your hands are tied completely. My experience with SAML is that to get it implemented over something more basic like LDAP, it has to be simplistic. Not that people get LDAP right but it's very common to support out of the box.
 
I'm going to run the scenario by our web developers soon and hopefully come up with a flexible, secure and usable solution. ForceAuthn hadn't really crossed my mind but I'll note it down. If my idea isn't viable, ForceAuthn might be.

ForceAuthn is being set by the SP, of course if there is a way that can be done from the IDP side only it's a much better solution.
I just couldn't think of one. I'll be happy if there is a better solution which is not as hacky.

Nakee
 

Thanks for the great module! it worked flawlessy so far with whatever I threw at it:)

Thank you for testing it and commenting. :)

--
I am root! Bow before me!

--

[Mathias Meisfjordskar]

Hello again.

I've attached a new patch with numerous changes. The list is:

* Handle doAuthentication() exceptions. This is logged and Negotiate
will fall back to the secondary SSP module.

* Support disabling certain SPs from Negotiate. By putting
'negotiate:disable' => TRUE in the SP entry in saml20-sp-remote.php,
Negotiate will always send the client to fallback. (We use it for some
test clients and such)

* Support disabling per client in Negotate by cookies. Attached are two
simple PHP scripts that set and remove the cookie. Both use SSP's
framework for text (JSON-files) and CSS (by API). URLs will typically
look like:
https://my.idp.net/simplesaml/module.php/negotiate/disable.php
and enable.php. Your surrounding documentation should inform users
about this feature as the login page does not.

* Disable login loops by cookie. When logging out on an SP you can be
sent back to the IdP because your browser sent a GET to a restricted
resorce. Previously this would just log you right back in and send you
on your merry way with a shiny new SAML session. With this change,
Negotiate will set a session cookie that prevents automatically
logging you back in. (More on this later)

Probably some more stuff I've forgotten.

Missing/unfinished:

* The doc file isn't updated to reflect all the changes above. (I'm
waiting for the stuff under to be discussed before changing it)

* Included in the www/ directory is a "retry.php" file that will allow a
user to retry Kerberos login in Negotiate. This is typically linked on
your login page when the page finds the cookie set. Like so:

<a href="'.SimpleSAML_Module::getModuleURL('negotiate/retry.php', $this->data['state']).'">retry</a>'

We have solved this locally by adding some bits of code to our regular
login page but this isn't really dynamic enough for general use. Can
this be solved by hooks in some way?

* Disable should also give the user a message on your regular
login page. Hooks? Note to self: don't show this message is
negotiate:disable is set in metadata.

* When using retry.php, SSP logs this:

Sep 27 13:47:36 simplesamlphp WARNING [b99e6131ee] Wrong stage in state. Was 'foo', shoud be 'sspmod_negotiate_Auth_Source_Negotiate.StageId'.

Should I set stage ID when entering retry.php?

* The JSON files with text for the enable/disable pages are currently
UiO specific. This should be generalized for public use.

Comments, suggestions?

[Olav Morken]

On Tue, Sep 27, 2011 at 14:26:15 +0200, Mathias Meisfjordskar wrote:
>
> Hello again.
>
> I've attached a new patch with numerous changes. The list is:
>
> * Handle doAuthentication() exceptions. This is logged and Negotiate
> will fall back to the secondary SSP module.
>
> * Support disabling certain SPs from Negotiate. By putting
> 'negotiate:disable' => TRUE in the SP entry in saml20-sp-remote.php,
> Negotiate will always send the client to fallback. (We use it for some
> test clients and such)
>
> * Support disabling per client in Negotate by cookies. Attached are two
> simple PHP scripts that set and remove the cookie. Both use SSP's
> framework for text (JSON-files) and CSS (by API). URLs will typically
> look like:
> https://my.idp.net/simplesaml/module.php/negotiate/disable.php
> and enable.php. Your surrounding documentation should inform users
> about this feature as the login page does not.
>
> * Disable login loops by cookie. When logging out on an SP you can be
> sent back to the IdP because your browser sent a GET to a restricted
> resorce. Previously this would just log you right back in and send you
> on your merry way with a shiny new SAML session. With this change,
> Negotiate will set a session cookie that prevents automatically
> logging you back in. (More on this later)

Instead of using another cookie, maybe it can be stored in the session
object? I.e.:

$session = SimpleSAML_Session::getInstance();
$session->setData('negotiate:disable', $this->authId, TRUE, 24*60*60);

(The last parameter is the timeout of the data, in this case 24 hours.)

It can later be checked with:

$session = SimpleSAML_Session::getInstance();
$disabled = $session->getData('negotiate:disable', $this->authId);
if ($disabled) {
/* ... */
}

>
> Probably some more stuff I've forgotten.
>
> Missing/unfinished:
>
> * The doc file isn't updated to reflect all the changes above. (I'm
> waiting for the stuff under to be discussed before changing it)
>
> * Included in the www/ directory is a "retry.php" file that will allow a
> user to retry Kerberos login in Negotiate. This is typically linked on
> your login page when the page finds the cookie set. Like so:
>
> <a href="'.SimpleSAML_Module::getModuleURL('negotiate/retry.php', $this->data['state']).'">retry</a>'

Just a minor note: You should always surround such URLs with
htmlspecialchars. I.e.:

<a href="' . htmlspecialchars(SimpleSAML_Module::getModuleURL('negotiate/retry.php', $this->data['state'])) . '">retry</a>'

> We have solved this locally by adding some bits of code to our regular
> login page but this isn't really dynamic enough for general use. Can
> this be solved by hooks in some way?
>
> * Disable should also give the user a message on your regular
> login page. Hooks? Note to self: don't show this message is
> negotiate:disable is set in metadata.

I'm not against hooks for the standard login page, but I think it will
be difficult to develop in a manner that ensures that it is generic
enough to be used for many usecases. Suggestions/implementations
welcome :)

> * When using retry.php, SSP logs this:
>
> Sep 27 13:47:36 simplesamlphp WARNING [b99e6131ee] Wrong stage in state. Was 'foo', shoud be 'sspmod_negotiate_Auth_Source_Negotiate.StageId'.
>
> Should I set stage ID when entering retry.php?

It continues after logging this? That would indicate that you trigger
the fallback to IdP-initiated SSO, since that message is a "dead end"

for the current authentication request.

The stage parameter is present to avoid users somehow misusing the
AuthState parameter in order to bypass authentication "steps" by
jumping forward in the authentication process. In this case it stopped
you from jumping backwards.

I think that the proper solution is to clone the state array when
generating the retry URL. E.g. you could do something like this when
you switch to the fallback authentication in the negotiate module:

$retryState = $state;
unset($retryState[SimpleSAML_Auth_State::ID]);
$retryID = SimpleSAML_Auth_State::saveState($retryState, 'negotiate:retry');
$state['negotiate:retryURL'] = SimpleSAML_Module::getModuleURL('negotiate/retry.php', array('AuthState' => $retryID));

> * The JSON files with text for the enable/disable pages are currently
> UiO specific. This should be generalized for public use.

Regards,

[Mathias Meisfjordskar]

On Wed, Sep 28, 2011 at 11:02 AM, Olav Morken <olav....@uninett.no> wrote:
> On Tue, Sep 27, 2011 at 14:26:15 +0200, Mathias Meisfjordskar wrote:

>> * Disable login loops by cookie. When logging out on an SP you can be
>>   sent back to the IdP because your browser sent a GET to a restricted
>>   resorce. Previously this would just log you right back in and send you
>>   on your merry way with a shiny new SAML session. With this change,
>>   Negotiate will set a session cookie that prevents automatically
>>   logging you back in. (More on this later)
>
> Instead of using another cookie, maybe it can be stored in the session
> object? I.e.:
>
>    $session = SimpleSAML_Session::getInstance();
>    $session->setData('negotiate:disable', $this->authId, TRUE, 24*60*60);
>
> (The last parameter is the timeout of the data, in this case 24 hours.)
>
> It can later be checked with:
>
>    $session = SimpleSAML_Session::getInstance();
>    $disabled = $session->getData('negotiate:disable', $this->authId);
>    if ($disabled) {
>        /* ... */
>    }

Sound even better. I'll look into using this instead.

>>   <a href="'.SimpleSAML_Module::getModuleURL('negotiate/retry.php', $this->data['state']).'">retry</a>'
>
> Just a minor note: You should always surround such URLs with
> htmlspecialchars. I.e.:
>
>    <a href="' . htmlspecialchars(SimpleSAML_Module::getModuleURL('negotiate/retry.php', $this->data['state'])) . '">retry</a>'

Ah, yes. I forgot about that. I'll include this in whatever we come up
with for hooks.

>>   We have solved this locally by adding some bits of code to our regular
>>   login page but this isn't really dynamic enough for general use. Can
>>   this be solved by hooks in some way?
>>
>> * Disable should also give the user a message on your regular
>>   login page. Hooks? Note to self: don't show this message is
>>   negotiate:disable is set in metadata.
>
> I'm not against hooks for the standard login page, but I think it will
> be difficult to develop in a manner that ensures that it is generic
> enough to be used for many usecases. Suggestions/implementations
> welcome :)

Do you mean implementing hooks in the standard login page at all or
making the hook for Negotiate generic enough? Hooks sound very useful
from the little I've read from docs as it can make a dynamic login
page but it's not really used form what I've gathered. That's a shame
because it really is a neat feature. Probably most useful for examples
like this.

Getting a generic help text for Negotiate will be difficult as the
whole process is a bit complex for a regular user. I'm going to ask
some of the more talented writers here at UiO for help on this. Same
for the disable/enable help text. I've come to terms with being
useless at documenting these things at an elementary level myself.
Just linking the SPNEGO RFC would probably be seen as somewhat lazy
and nonconstructive I bet. :)

>> * When using retry.php, SSP logs this:
>>
>>   Sep 27 13:47:36 simplesamlphp WARNING [b99e6131ee] Wrong stage in state. Was 'foo', shoud be 'sspmod_negotiate_Auth_Source_Negotiate.StageId'.
>>
>>   Should I set stage ID when entering retry.php?
>
> It continues after logging this? That would indicate that you trigger
> the fallback to IdP-initiated SSO, since that message is a "dead end"
> for the current authentication request.
>
> The stage parameter is present to avoid users somehow misusing the
> AuthState parameter in order to bypass authentication "steps" by
> jumping forward in the authentication process. In this case it stopped
> you from jumping backwards.
>
> I think that the proper solution is to clone the state array when
> generating the retry URL. E.g. you could do something like this when
> you switch to the fallback authentication in the negotiate module:
>
>    $retryState = $state;
>    unset($retryState[SimpleSAML_Auth_State::ID]);
>    $retryID = SimpleSAML_Auth_State::saveState($retryState, 'negotiate:retry');
>    $state['negotiate:retryURL'] = SimpleSAML_Module::getModuleURL('negotiate/retry.php', array('AuthState' => $retryID));

It's only once. After hitting retry.php. Once back in Negotiate.php,
everything seems to be back to normal. I haven't checked where SSP
logs this, I'm guessing the first time it tries to load state back in
modules.php/negotiate.

I'll try the above, thanks.

[Olav Morken]

On Wed, Sep 28, 2011 at 20:39:58 +0200, Mathias Meisfjordskar wrote:
> On Wed, Sep 28, 2011 at 11:02 AM, Olav Morken <olav....@uninett.no> wrote:
> > On Tue, Sep 27, 2011 at 14:26:15 +0200, Mathias Meisfjordskar wrote:

> >> � We have solved this locally by adding some bits of code to our regular

> >> � login page but this isn't really dynamic enough for general use. Can
> >> � this be solved by hooks in some way?
> >>
> >> * Disable should also give the user a message on your regular
> >> � login page. Hooks? Note to self: don't show this message is
> >> � negotiate:disable is set in metadata.
> >
> > I'm not against hooks for the standard login page, but I think it will
> > be difficult to develop in a manner that ensures that it is generic
> > enough to be used for many usecases. Suggestions/implementations
> > welcome :)
>
> Do you mean implementing hooks in the standard login page at all or
> making the hook for Negotiate generic enough? Hooks sound very useful
> from the little I've read from docs as it can make a dynamic login
> page but it's not really used form what I've gathered. That's a shame
> because it really is a neat feature. Probably most useful for examples
> like this.

The difficulty is seeing the use-cases for hooks before someone comes
up with them :) One runs the risk of creating something that does not
cover the actual use-cases.

But I'd suggest delaying any sort of hook-implementation until this
module is in simpleSAMLphp.

> Getting a generic help text for Negotiate will be difficult as the
> whole process is a bit complex for a regular user. I'm going to ask
> some of the more talented writers here at UiO for help on this. Same
> for the disable/enable help text. I've come to terms with being
> useless at documenting these things at an elementary level myself.
> Just linking the SPNEGO RFC would probably be seen as somewhat lazy
> and nonconstructive I bet. :)

I definitively see the problem. How to describe "retry windows
integrated authentication" in one sentence using terms the users are
familiar with.

[Mathias Meisfjordskar]

[ Olav Morken ]

Hello again and sorry for the long wait. We've been busy putting this
module into production for ~30,000 users, give or take. So far not a
single error report!

> On Wed, Sep 28, 2011 at 20:39:58 +0200, Mathias Meisfjordskar wrote:
>
>> Do you mean implementing hooks in the standard login page at all or
>> making the hook for Negotiate generic enough? Hooks sound very useful
>> from the little I've read from docs as it can make a dynamic login
>> page but it's not really used form what I've gathered. That's a shame
>> because it really is a neat feature. Probably most useful for examples
>> like this.
>
> The difficulty is seeing the use-cases for hooks before someone comes
> up with them :) One runs the risk of creating something that does not
> cover the actual use-cases.
>
> But I'd suggest delaying any sort of hook-implementation until this
> module is in simpleSAMLphp.

Agreed. For now I've included a section in the documentation on what's
going og and how to locally handle it.

>> Getting a generic help text for Negotiate will be difficult as the
>> whole process is a bit complex for a regular user. I'm going to ask
>> some of the more talented writers here at UiO for help on this. Same
>> for the disable/enable help text. I've come to terms with being
>> useless at documenting these things at an elementary level myself.
>> Just linking the SPNEGO RFC would probably be seen as somewhat lazy
>> and nonconstructive I bet. :)
>
> I definitively see the problem. How to describe "retry windows
> integrated authentication" in one sentence using terms the users are
> familiar with.

We landed on "Try automatic authentication again" as a link at the
bottom of the regular login page. It seems to work as I have only gotten
a single comment about it.

I've made a new patch with a few changes:

* Removed a bit of local flavor in the dictionaries.
* Replaced the meta refresh in the 401 message with JS as the former
hung a bit and gave users a scare.
* Rearranged the code in Negotiate.php a little so some filtering is
accessable outside of the module.

Question now is: Is the code ready to be merged with SSP? Could be there
are a few issues I have forgotten but they should be minor at this point
as we've been running this now for over a month.

[Søren Grønning Iversen]

Hi Mathias,

can I just say that this sounds absolutely AWESOME - I never realised that you have such an enormous testing groud... 30.000 users and no error reports sounds fantastic to me! I'll be looking into integrating this work for our 400 students as soon as I have time, it's really what I've been looking for with the choice to go with SSP for our SSO-needs!

Thanks - I hope it'll make it into SSP soon! -And thanks to Olav for always being such a good sport!

-Søren

[Olav Morken]

On Wed, Nov 23, 2011 at 14:40:54 +0100, Mathias Meisfjordskar wrote:
> Question now is: Is the code ready to be merged with SSP? Could be there
> are a few issues I have forgotten but they should be minor at this point
> as we've been running this now for over a month.

Unfortunately, it looks like I won't have time to look at before
Thursday next week, but I'll try to have a look at it then.

[Mathias Meisfjordskar]

Oh, I wasn't expecting you to drop everything and focus solely on this
for a new release or anything. :) It was more of a general question.
Is the module in a state that allows it to be included in the next
major version, was what I meant. I'm in no hurry (and I can pester you
at the meeting next week anyway) so don't stress it. My goal was met
once we got this into production locally. Having it become a part of
SSP proper is a secondary goal.

--
Cheers!
Mathias Meisfjordskar

[Olav Morken]

On Wed, Nov 23, 2011 at 14:40:54 +0100, Mathias Meisfjordskar wrote:

> I've made a new patch with a few changes:
>
> * Removed a bit of local flavor in the dictionaries.
> * Replaced the meta refresh in the 401 message with JS as the former
> hung a bit and gave users a scare.
> * Rearranged the code in Negotiate.php a little so some filtering is
> accessable outside of the module.
>
> Question now is: Is the code ready to be merged with SSP? Could be there
> are a few issues I have forgotten but they should be minor at this point
> as we've been running this now for over a month.

I'd like to see at least one of the issues from the previous patch
adressed:

> > * When using retry.php, SSP logs this:
> >
> > Sep 27 13:47:36 simplesamlphp WARNING [b99e6131ee] Wrong stage in state. Was 'foo', shoud be 'sspmod_negotiate_Auth_Source_Negotiate.StageId'.
> >
> > Should I set stage ID when entering retry.php?
>
> It continues after logging this? That would indicate that you trigger
> the fallback to IdP-initiated SSO, since that message is a "dead end"
> for the current authentication request.
>
> The stage parameter is present to avoid users somehow misusing the
> AuthState parameter in order to bypass authentication "steps" by
> jumping forward in the authentication process. In this case it stopped
> you from jumping backwards.
>
> I think that the proper solution is to clone the state array when
> generating the retry URL. E.g. you could do something like this when
> you switch to the fallback authentication in the negotiate module:
>
> $retryState = $state;
> unset($retryState[SimpleSAML_Auth_State::ID]);
> $retryID = SimpleSAML_Auth_State::saveState($retryState, 'negotiate:retry');
> $state['negotiate:retryURL'] = SimpleSAML_Module::getModuleURL('negotiate/retry.php', array('AuthState' => $retryID));

I believe this should be fixed since the current behaviour will not
work with any SPs that do not support IdP-initiated authentication.
(I believe Vortex is among the SPs that do not support IdP-initiated
authentication, but I don't know the details.)

(Also note that r2979 added the cloneState()-function that may be used
for this purpose.)

When looking over the patch again, I also saw a few more (mostly minor)
issues:

[...]
> diff --git a/modules/negotiate/docs/negotiate.txt b/modules/negotiate/docs/negotiate.txt
> new file mode 100644
> index 0000000..364496a
> --- /dev/null
> +++ b/modules/negotiate/docs/negotiate.txt
> @@ -0,0 +1,258 @@
[...]
> +Test the Kerberos setup with the following script:
> +
> + <?php
> + if(!extension_loaded('krb5')) {
> + die('KRB5 Extension not installed');
> + }
> +
> + if(!empty($_SERVER['HTTP_AUTHORIZATION'])) {
> + list($mech, $data) = split(' ', $_SERVER['HTTP_AUTHORIZATION']);

It doesn't really matter, since this is just test code, but the
split()-function is deprecated. You should use explode() instead.

[...]
> diff --git a/modules/negotiate/lib/Auth/Source/Negotiate.php b/modules/negotiate/lib/Auth/Source/Negotiate.php
> new file mode 100644
> index 0000000..6204bfe
> --- /dev/null
> +++ b/modules/negotiate/lib/Auth/Source/Negotiate.php
> @@ -0,0 +1,318 @@
> +<?php
> +
> +/**
> + * The Negotiate module. Allows for password-less, secure login by
> + * Kerberos and Negotiate.
> + *
> + * @author Mathias Meisfjordskar, University of Oslo.
> + * <mathias.me...@usit.uio.no>
> + * @package simpleSAMLphp
> + * @version $Id$
> + */
> +
> +class sspmod_negotiate_Auth_Source_Negotiate extends SimpleSAML_Auth_Source {
> +
> + // Constants used in the module
> + const STAGEID = 'sspmod_negotiate_Auth_Source_Negotiate.StageId';
> +
> + private $config;
> + protected $ldap = NULL;
> +
> + /**
> + * Constructor for this authentication source.
> + *
> + * @param array $info Information about this authentication source.
> + * @param array $config Configuration.
> + */
> + public function __construct($info, $config) {
> + assert('is_array($info)');
> + assert('is_array($config)');
> +
> + if(!extension_loaded('krb5'))
> + throw new Exception('KRB5 Extension not installed');
> +
> + // Call the parent constructor first, as required by the interface.
> + parent::__construct($info, $config);
> +
> + $this->config = SimpleSAML_Configuration::loadFromArray($config);;

I think this can be a local variable, since you never read
$this->config outside of the constructor.

> +
> + foreach(array('hostname', 'base', 'keytab', 'fallback') as $i) {
> + if (!array_key_exists($i, $config)) {
> + throw new Exception('The required "'.$i.'" config option was not found');
> + }
> + }

This for-loop isn't needed, since the getString()-calls will emit an
error if the option is missing.

> + $this->backend = $this->config->getString('fallback');
> + $this->hostname = $this->config->getString('hostname');
> + $this->enableTLS = $this->config->getBoolean('enable_tls', FALSE);
> + $this->debugLDAP = $this->config->getBoolean('debugLDAP', FALSE);
> + $this->timeout = $this->config->getValue('timeout', 30);
> + $this->keytab = $this->config->getString('keytab');
> + $this->base = $this->config->getString('base');
> + $this->attr = $this->config->getString('attr', 'uid');
> + $this->subnet = $this->config->getArray('subnet', NULL);
> + $this->admin_user = $this->config->getString('adminUser', NULL);
> + $this->admin_pw = $this->config->getString('adminPassword', NULL);
> +
> + }
> +
> + /**
> + * The inner workings of the module.
> + *
> + * Checks to see if client is in the defined subnets (if
> + * defined in config). Sends the client a 401 Negotiate and
> + * responds to the result. If the client fails to provide a
> + * proper Kerberos ticket, the login process is handed over to
> + * the 'fallback' module defined in the config.
> + *
> + * LDAP is used as a user metadata source.
> + *
> + * @param array &$state Information about the current authentication.
> + */
> + public function authenticate(&$state) {
> + assert('is_array($state)');
> +
> + // Set the default backend to config
> + $state['LogoutState'] = array(
> + 'negotiate:backend' => $this->backend,
> + );
> + $state['negotiate:authId'] = $this->authId;
> + //$id = SimpleSAML_Auth_State::saveState($state, self::STAGEID);

Dead code can be deleted.

[...]
> + SimpleSAML_Logger::debug('Negotiate - authenticate(): looking for Negotate');
> + if (!empty($_SERVER['HTTP_AUTHORIZATION'])) {
> + SimpleSAML_Logger::debug('Negotiate - authenticate(): Negotate found'); $this->ldap = new SimpleSAML_Auth_LDAP($this->hostname, $this->enableTLS, $this->debugLDAP, $this->timeout);
> +
> + list($mech, $data) = explode(' ', $_SERVER['HTTP_AUTHORIZATION']);

I'd use � explode(' ', $_SERVER['HTTP_AUTHORIZATION'], 2); �, just in
case an authorization-header contains more than two strings.

> + if(strtolower($mech) == 'basic')
> + SimpleSAML_Logger::debug('Negotiate - authenticate(): Basic found. Skipping.');
> + else if(strtolower($mech) != 'negotiate')
> + SimpleSAML_Logger::debug('Negotiate - authenticate(): No "Negotiate" found. Skipping.');
[...]

> + /**
> + * Send the actual headers and body of the 401. Embedded in
> + * the body is a post that is triggered by JS if the client
> + * wants to show the 401 message.
> + *
> + * @param array $params additional parameters to the URL in
> + * the URL in the body
> + */
> + protected function sendNegotiate($params) {
> + $url = SimpleSAML_Module::getModuleURL('negotiate/backend.php', $params);
> +
> + header('HTTP/1.1 401 Unauthorized');
> + header('WWW-Authenticate: Negotiate',false);
> + echo '
> +<html>
> +<head>
> +<script type="text/javascript">window.location = "'.htmlspecialchars($url).'"</script>

You should probably also call json_encode on the return-value from
htmlspecialchars(). It should not be possible to get an unescaped
quotation-mark into $url, but it doesn't hurt to to be safe. The string
then becomes:

<script type="text/javascript">window.location = ' . json_encode(htmlspecialchars($url)) . ';</script>

> +<title>Redirect to login</title>
> +</head>
> +<body>
> +Your browser seems to have meta refresh disabled for some reason. Please

This text is no longer correct.

> +click <a href="'.htmlspecialchars($url).'">here</a>.
> +</body>
> +</html> ';
> +
> + }
> +
> + /**
> + * Passes control of the login process to a different module.
> + *
> + * @param string $state Information about the current authentication.
> + */
> + public static function fallBack(&$state) {
> + $authId = $state['LogoutState']['negotiate:backend'];
> +
> + if ($authId === NULL) {
> + $msg = "This code should never be reached.";
> + throw new SimpleSAML_Error_AuthSource($msg);
> + }
> + $source = SimpleSAML_Auth_Source::getById($authId);
> +
> + try {
> + $source->authenticate($state);
> + } catch (SimpleSAML_Error_Exception $e) {
> + SimpleSAML_Auth_State::throwException($state, $e);
> + } catch (Exception $e) {
> + $e = new SimpleSAML_Error_UnserializableException($e);
> + SimpleSAML_Auth_State::throwException($state, $e);
> + }
> + // TBD: Should this loginCompleted?

I think this comment can be replaced with a comment saying that you
must always call loginCompleted() since the fallBack()-function is
called in places where it cannot return.

> + SimpleSAML_Logger::debug('Negotiate: backend returned');
> + self::loginCompleted($state);
> + }
> +

[...]
> + public function logout(&$state) {
> + assert('is_array($state)');
> +
> + /* Get the source that was used to authenticate */
> + $authId = $state['negotiate:backend'];
> + if ($authId === NULL) {
> + setcookie('NEGOTIATE_AUTOLOGIN_DISABLE_SESSION', 'True', 0, '/',
> + SimpleSAML_Utilities::getSelfHost(), TRUE, TRUE);

Here you unconditionally set the cookie for https only. A generic
module should probably allow the operators to at least test it on http.

(In this particular case I don't think making it available over http
should be a problem. It isn't sensitive data.)

> + parent::logout($state);
> + } else {
> + $source = SimpleSAML_Auth_Source::getById($authId);
> + $source->logout($state);
> + }
> + }
> +
> +}
> +
> diff --git a/modules/negotiate/templates/disable.php b/modules/negotiate/templates/disable.php
> new file mode 100644
> index 0000000..5236a4e
> --- /dev/null
> +++ b/modules/negotiate/templates/disable.php
> @@ -0,0 +1,21 @@
> +<?php
> +
> +/**
> + *
> + *
> + * @author Mathias Meisfjordskar, University of Oslo.
> + * <mathias.me...@usit.uio.no>
> + * @package simpleSAMLphp
> + * @version $Id$
> + */
> +$this->includeAtTemplateBase('includes/header.php');
> +?>
> +<h1><?php echo $this->t('{negotiate:negotiate:disable_title}'); ?></h1>
> +<?php
> +$url = SimpleSAML_Module::getModuleURL('negotiate/enable.php');
> +?>
> +<?php echo $this->t('{negotiate:negotiate:disable_info_pre}', array('URL' => $url)); ?>

You should use htmlspecialchars() around $url. It probably doesn't
matter in this case, but it doesn't hurt either.

> +
> +<?php echo $this->t('{negotiate:negotiate:info_post}'); ?>
> +
> +<?php $this->includeAtTemplateBase('includes/footer.php'); ?>
> diff --git a/modules/negotiate/templates/enable.php b/modules/negotiate/templates/enable.php
> new file mode 100644
> index 0000000..9b19181
> --- /dev/null
> +++ b/modules/negotiate/templates/enable.php
> @@ -0,0 +1,22 @@
> +<?php
> +
> +/**
> + *
> + *
> + * @author Mathias Meisfjordskar, University of Oslo.
> + * <mathias.me...@usit.uio.no>
> + * @package simpleSAMLphp
> + * @version $Id$
> + */
> +$this->includeAtTemplateBase('includes/header.php');
> +?>
> +<h1><?php echo $this->t('{negotiate:negotiate:enable_title}'); ?></h1>
> +
> +<?php
> +$url = SimpleSAML_Module::getModuleURL('negotiate/disable.php');
> +?>
> +<?php echo $this->t('{negotiate:negotiate:enable_info_pre}', array('URL' => $url)); ?>

htmlspecialchars() around $url here also.

[...]
> diff --git a/modules/negotiate/www/disable.php b/modules/negotiate/www/disable.php
> new file mode 100644
> index 0000000..1f2a8bf
> --- /dev/null
> +++ b/modules/negotiate/www/disable.php
> @@ -0,0 +1,16 @@
> +<?php
> +
> +/**
> + *
> + *
> + * @author Mathias Meisfjordskar, University of Oslo.
> + * <mathias.me...@usit.uio.no>
> + * @package simpleSAMLphp
> + * @version $Id$
> + */
> +
> +$globalConfig = SimpleSAML_Configuration::getInstance();
> +setcookie('NEGOTIATE_AUTOLOGIN_DISABLE_PERMANENT', 'True', mktime(0,0,0,1,1,2038), '/', SimpleSAML_Utilities::getSelfHost(), TRUE, TRUE);
> +setcookie('NEGOTIATE_AUTOLOGIN_DISABLE_SESSION', 'False', time() - 3600, '/', SimpleSAML_Utilities::getSelfHost(), TRUE, TRUE);

See my above comment about setcookie().

> +$t = new SimpleSAML_XHTML_Template($globalConfig, 'negotiate:disable.php');
> +$t->show();
> diff --git a/modules/negotiate/www/enable.php b/modules/negotiate/www/enable.php
> new file mode 100644
> index 0000000..ae2c610
> --- /dev/null
> +++ b/modules/negotiate/www/enable.php
> @@ -0,0 +1,16 @@
> +<?php
> +
> +/**
> + *
> + *
> + * @author Mathias Meisfjordskar, University of Oslo.
> + * <mathias.me...@usit.uio.no>
> + * @package simpleSAMLphp
> + * @version $Id$
> + */
> +
> +$globalConfig = SimpleSAML_Configuration::getInstance();
> +setcookie('NEGOTIATE_AUTOLOGIN_DISABLE_PERMANENT', 'False', time() - 3600, '/', SimpleSAML_Utilities::getSelfHost(), TRUE, TRUE);
> +setcookie('NEGOTIATE_AUTOLOGIN_DISABLE_SESSION', 'False', time() - 3600, '/', SimpleSAML_Utilities::getSelfHost(), TRUE, TRUE);

See my above comment about setcookie().

> +$t = new SimpleSAML_XHTML_Template($globalConfig, 'negotiate:enable.php');
> +$t->show();
> diff --git a/modules/negotiate/www/retry.php b/modules/negotiate/www/retry.php
> new file mode 100644
> index 0000000..28a76d0
> --- /dev/null
> +++ b/modules/negotiate/www/retry.php
> @@ -0,0 +1,25 @@
> +<?php
> +
> +/**
> + *
> + *
> + * @author Mathias Meisfjordskar, University of Oslo.
> + * <mathias.me...@usit.uio.no>
> + * @package simpleSAMLphp
> + * @version $Id$
> + */
> +
> +setcookie('NEGOTIATE_AUTOLOGIN_DISABLE_SESSION', 'False', time() - 3600, '/', SimpleSAML_Utilities::getSelfHost(), TRUE, TRUE);

See my above comment about setcookie().

> +
> +$authStateId = $_REQUEST['AuthState'];
> +$state = SimpleSAML_Auth_State::loadState($authStateId, sspmod_negotiate_Auth_Source_Negotiate::STAGEID);
> +
> +$metadata = SimpleSAML_Metadata_MetaDataStorageHandler::getMetadataHandler();
> +$idpmeta = $metadata->getMetaDataCurrentEntityID('saml20-idp-hosted', 'metaindex');
> +if ($idpmeta->hasValue('auth')) {

The hasValue()-function does not exists, since $idpmeta is an array.
You should use isset($idpmeta['auth']) instead.

> + $source = SimpleSAML_Auth_Source::getById($idpmeta['auth']);
> + $source->authenticate($state);
> + assert('FALSE');
> +} else {
> + assert('FALSE');
> +}
> \ No newline at end of file

Best regards,

[E L]

In my humble opinion there should also be some configuration function
that disables the plugin if it detects an IE client.
It was a real problem here for us.

Beside that great plugin, which works really well :-)

[Mathias Meisfjordskar]

Thanks for going through the code once again, Olav. I see some of the
mistakes that I forgot. I'll try to address them all in the next
patch.

@E L - We discussed extending the "filter" concept in Negotiate here
at UiO but we implemented only the necessary local ones. Disabling IE
was never an option for us as most users use Windows/IE. It's by far
the most challenging client to support and demands an iron fist-level
of control over your domain. Do you want to disable them because you
can't force config on all clients? Anyway, write a patch that can
toggle such filtering and I'm sure Olav will include it. I've
committed to far too much work to make such a feature myself.

[Mathias Meisfjordskar]

[ Mathias Meisfjordskar ]

> Thanks for going through the code once again, Olav. I see some of the
> mistakes that I forgot. I'll try to address them all in the next
> patch.

Hello again.

Long overdue but here's the latest and (hopefully) final patch for
Negotiate. Changes are:

* Switched from session cookie to internal SSP session variable for
tracking recent logouts.
* Fixed all of Olav's comments (I hope).
* Fixed a state flaw when using retry.php (only affected the code in the
doc)
* Various smaller cleanups in code and doc.

I hope this covers all the earlier comments and flaws. If I missed
something, please let me know.

Many thanks to Olav for patiently helping me fix some of the more
obscure issues with the module. We've been running it for a while now
and have yet to get a single error report on Negotiate itself.

[Olav Morken]

Thanks! I have now committed this patch (plus the minor changes we
discussed on IRC) in r3038.

[E L]

Hi,
Is there a reason why not to add the following line to the negotiate module?

$headers = apache_request_headers();
if (isset($headers["Authorization"]))
$_SERVER['HTTP_AUTHORIZATION'] = $headers["Authorization"];

This should allow finding the header without fighting mod_rewrite.

Ely

[Mathias Meisfjordskar]

On Tue, May 29, 2012 at 9:03 AM, E L <nak...@gmail.com> wrote:
> Hi,
> Is there a reason why not to add the following line to the negotiate module?
>
> $headers = apache_request_headers();
> if (isset($headers["Authorization"]))
>        $_SERVER['HTTP_AUTHORIZATION'] = $headers["Authorization"];
>
> This should allow finding the header without fighting mod_rewrite.

Hello.

It could be included but you would break any ISS compatibility (or any
other non-Apache web server). Not sure if SSP aims to be ISS
compatible.

[E L]

Hi,

I guess it will need to detect the server first.
You are right it sounds too much of a hassle.

Thanks,

Ely

[Mathias Meisfjordskar]

Hello again.

I've made a small patch to fix a small but potentially harmful bug in
Negotiate. The gist of it is that a user with a SAML session obtained
by Negotiate would retain the SAML SSO session even coming from SPs
where Negotiate is disabled. If you use Negotiate as a simple
substitute in front of you regular login, this doesn't matter much to
you. If you, like me, have individual sources on your IdP, this
creates problems. It may also be useful if you want to enforce manual
login on certain SPs. Previously this was broken due to SSO.

All this patch does is implement reauthenticate() so Negotiate will
check each SP metadata on reAuthN.

Many thanks to Olav for reminding me of the reauthenticate() method.

[Olav Morken]

I think there may be a bug in the code -- what if negotiate
authentication is enabled for the SP? It looks like it will fall
through to the assert('FALSE') in that case.

[Mathias Meisfjordskar]

On Fri, Jun 8, 2012 at 8:13 AM, Olav Morken <olav....@uninett.no> wrote:
>
> I think there may be a bug in the code -- what if negotiate
> authentication is enabled for the SP? It looks like it will fall
> through to the assert('FALSE') in that case.

Whoopsie. Small cut'n paste misfire there. I'll send a new patch later today.

[Andrew Blosser]

Can anyone help me figure out how to install this on a windows server?  I can't seem to find php_krb5 compiled for windows anywhere. Is it required on a windows server for the negotiate module?

[Mathias Meisfjordskar]

It is required, yes. Like I said in the other thread, try to compile it with either Heimdal og MIT Kerbros installed. One of them should be available in a Windows repository since a lot of open-source software relies on or can incorporate Kerberos.

On Mon, Feb 25, 2013 at 6:25 PM, Andrew Blosser <anak...@gmail.com> wrote:

Can anyone help me figure out how to install this on a windows server?  I can't seem to find php_krb5 compiled for windows anywhere. Is it required on a windows server for the negotiate module?

--

You received this message because you are subscribed to the Google Groups "simpleSAMLphp" group.

To unsubscribe from this group and stop receiving emails from it, send an email to simplesamlph...@googlegroups.com.

To post to this group, send email to simple...@googlegroups.com.

Visit this group at http://groups.google.com/group/simplesamlphp?hl=en.

For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Jason Haar]

Why would you ever need such a thing under Windows? Oh - is that php for
Apache-win32 or for IIS? Surely if you have PHP for IIS, you'd just use
standard IIS authentication, and then REMOTE_USER would be presented to
PHP - which means you could use $_SERVER["REMOTE_USER"] in SSP?

I'm not a Windows guy - so I'm assuming lots of things - like SSP works
under IIS/PHP...

--
Cheers

Jason Haar
Information Security Manager, Trimble Navigation Ltd.
Phone: +1 408 481 8171
PGP Fingerprint: 7A2E 0407 C9A6 CAF6 2B9F 8422 C063 5EBB FE1D 66D1

[Aj Blosser]

We're running apache-win32

=Ajay

--
You received this message because you are subscribed to a topic in the Google Groups "simpleSAMLphp" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/simplesamlphp/1e8uSgdFWAk/unsubscribe?hl=en.
To unsubscribe from this group and all its topics, send an email to simplesamlph...@googlegroups.com.