Skip to content

Commit 274ec53

Browse files
ghalsetvdijen
ghalse
authored and
tvdijen
committed
Fix computation of the PN hash
1 parent b0089ba commit 274ec53

4 files changed

Lines changed: 236 additions & 8 deletions

File tree

‎UPGRADE.md‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
# Upgrade notes for simplesamlphp-module-fticks
2+
3+
## Change in PN generation
4+
5+
The `PN` F-Ticks attribute is defined as "A unique identifier for the
6+
subject involved in the event." To allow statistics to be aggregated,
7+
this is commonly implemented as a privacy-preserving hash, and
8+
simplesamlphp-module-fticks is no different.
9+
10+
To ensure uniqueness in multiple identity provider (bridge)
11+
configurations, simplesamlphp-module-fticks originally
12+
scoped the generation of the `PN` hash in the same way as the
13+
[saml:PersistentNameID](https://simplesamlphp.org/docs/stable/saml/nameid.html).
14+
Unfortunately, in deriving the hashing algorithm, earlier versions
15+
of this module erroneously included both the source and the
16+
destination entityId in the calculation of the hash. Where the
17+
same user logs into several different services, a new PN hash is
18+
generated for each service. This may result in an overinflation
19+
in the number of unique principals. This behaviour is compatible
20+
with the definition but is different to the way e.g.
21+
[Shibboleth IdP](https://wiki.shibboleth.net/confluence/display/IDP30/FTICKSLoggingConfiguration)
22+
computes a hash, and may not be what aggregators expect.
23+
24+
In order to align the statistics generation with other software, the
25+
default behaviour has been changed to create a PN hash that only depends
26+
on the `identitfyingAttribute` and the `federation`.
27+
28+
People with existing statistics who wish to retain the old behaviour
29+
should set the `pnHashIsTargeted` option to `both`.
30+
31+
People using bridges where the `identitfyingAttribute` cannot be
32+
guarenteed unique should set the `pnHashIsTargeted` option to `source`.

‎docs/authproc_fticks.md‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,24 @@ The filter supports the following configuration options:
3333
[supported by PHP](http://php.net/manual/en/function.hash-algos.php)
3434
can be used.
3535

36+
`pnHashIsTargeted`
37+
: When generating the F-Ticks _PN_ attribute, include the source or
38+
destination entityId to create a targeted version of the subject. Must
39+
be one of the following options:
40+
41+
> * `none` - _PN_ depends only on the `federation` and
42+
`identifyingAttribute` (this is the default, and compatible with
43+
other implementations).
44+
> * `source` - _PN_ is targeted based on the SAML source. This is useful
45+
for [bridging configurations](bridging) where the `identifyingAttribute`
46+
may not be unique.
47+
> * `destination` - _PN_ is targeted based on the SAML destination
48+
> * `both` - _PN_ is targeted based on both the SAML source and destination
49+
(this option exists to preserve backwards-compatibility, and may
50+
lead to overcounting of subjects).
51+
52+
[bridging]: https://simplesamlphp.org/docs/stable/simplesamlphp-advancedfeatures.html#bridging-between-protocols
53+
3654
`exclude`
3755
: An array of F-ticks attributes to exclude/filter from the output.
3856

@@ -141,7 +159,7 @@ generated/derived:
141159

142160
`PN`
143161
: The PN is generated in a similar way too, but completely independently from
144-
a [saml:PersistentNameID][5].
162+
a [saml:PersistentNameID][5]. Depends on the setting of `pnHashIsTargeted`.
145163

146164
[5]: https://simplesamlphp.org/docs/stable/saml:nameid
147165

‎src/Auth/Process/Fticks.php‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use function array_key_exists;
1818
use function array_keys;
1919
use function array_map;
20+
use function boolval;
2021
use function constant;
2122
use function defined;
2223
use function gethostbyname;
@@ -77,6 +78,9 @@ class Fticks extends Auth\ProcessingFilter
7778
/** @var array F-ticks attributes to exclude */
7879
private array $exclude = [];
7980

81+
/** @var bool Enable legacy handing of PN (for backwards compatibility) */
82+
private string $pnHashIsTargeted = 'none';
83+
8084

8185
/**
8286
* Log a message to the desired destination
@@ -168,12 +172,16 @@ private function generatePNhash(array &$state): ?string
168172
/* calculate a hash */
169173
if ($uid !== null) {
170174
$userdata = $this->federation;
171-
if (array_key_exists('saml:sp:IdP', $state)) {
172-
$userdata .= strlen($state['saml:sp:IdP']) . ':' . $state['saml:sp:IdP'];
173-
} else {
174-
$userdata .= strlen($state['Source']['entityid']) . ':' . $state['Source']['entityid'];
175+
if (in_array($this->pnHashIsTargeted, ['source', 'both'])) {
176+
if (array_key_exists('saml:sp:IdP', $state)) {
177+
$userdata .= strlen($state['saml:sp:IdP']) . ':' . $state['saml:sp:IdP'];
178+
} else {
179+
$userdata .= strlen($state['Source']['entityid']) . ':' . $state['Source']['entityid'];
180+
}
181+
}
182+
if (in_array($this->pnHashIsTargeted, ['destination', 'both'])) {
183+
$userdata .= strlen($state['Destination']['entityid']) . ':' . $state['Destination']['entityid'];
175184
}
176-
$userdata .= strlen($state['Destination']['entityid']) . ':' . $state['Destination']['entityid'];
177185
$userdata .= strlen($uid) . ':' . $uid;
178186
$userdata .= $this->salt;
179187

@@ -257,6 +265,19 @@ public function __construct(array $config, $reserved)
257265
}
258266
}
259267

268+
if (array_key_exists('pnHashIsTargeted', $config)) {
269+
if (
270+
is_string($config['pnHashIsTargeted']) &&
271+
in_array($config['pnHashIsTargeted'], ['source', 'destination', 'both', 'none'])
272+
) {
273+
$this->pnHashIsTargeted = $config['pnHashIsTargeted'];
274+
} else {
275+
throw new Error\Exception(
276+
'F-ticks log pnHashIsTargeted must be one of [source, destnation, both, none]',
277+
);
278+
}
279+
}
280+
260281
if (array_key_exists('logdest', $config)) {
261282
if (
262283
is_string($config['logdest']) &&

‎tests/src/Auth/Process/FticksTest.php‎

Lines changed: 159 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ class FticksTest extends TestCase
5454
*/
5555
private static function processFilter(array $config, array $request): array
5656
{
57+
$_SERVER['REQUEST_URI'] = '/simplesaml/'; /* suppress warning from SimpleSAML/Utils/HTTP */
5758
$filter = new Fticks($config, null);
5859
$filter->process($request);
5960
return $request;
@@ -67,6 +68,7 @@ protected function setUp(): void
6768
Configuration::loadFromArray([
6869
'secretsalt' => 'secretsalt',
6970
], '[ARRAY]', 'simplesaml');
71+
7072
}
7173

7274

@@ -117,6 +119,58 @@ public function testSPwithUserId(): void
117119
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP#RP=https://localhost/idp#CSI=CL',
118120
'/',
119121
);
122+
$pattern2 = preg_quote(
123+
'#AM=' . Constants::AC_UNSPECIFIED
124+
. '#PN=d63bb55765af1321b06950abb5f9787cffd05ef271a09b67964f402f3f209cc6#TS=1000#',
125+
'/',
126+
);
127+
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '$/');
128+
$result = self::processFilter($config, $request);
129+
$this->assertEquals($request, $result);
130+
}
131+
132+
133+
/**
134+
*/
135+
public function testSPwithUserIdDifferentProviders(): void
136+
{
137+
$config = ['federation' => 'ACME', 'logdest' => 'stdout', 'identifyingAttribute' => 'eduPersonPrincipalName'];
138+
$request = array_merge(self::$minRequest, self::$spRequest, [
139+
'Attributes' => [
140+
'eduPersonPrincipalName' => [ 'user2@example.net' ],
141+
],
142+
]);
143+
$request['Destination']['entityid'] = 'https://localhost/idp2';
144+
$request['saml:sp:IdP'] = 'https://localhost/saml:sp:IdP2';
145+
$pattern1 = preg_quote(
146+
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP2#RP=https://localhost/idp2#CSI=CL',
147+
'/',
148+
);
149+
$pattern2 = preg_quote(
150+
'#AM=' . Constants::AC_UNSPECIFIED
151+
. '#PN=d63bb55765af1321b06950abb5f9787cffd05ef271a09b67964f402f3f209cc6#TS=1000#',
152+
'/',
153+
);
154+
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '$/');
155+
$result = self::processFilter($config, $request);
156+
$this->assertEquals($request, $result);
157+
}
158+
159+
160+
/**
161+
*/
162+
public function testSPwithUserIdLegacyBehaviour(): void
163+
{
164+
$config = ['federation' => 'ACME', 'logdest' => 'stdout', 'identifyingAttribute' => 'eduPersonPrincipalName', 'pnHashIsTargeted' => 'both',];
165+
$request = array_merge(self::$minRequest, self::$spRequest, [
166+
'Attributes' => [
167+
'eduPersonPrincipalName' => [ 'user2@example.net' ],
168+
],
169+
]);
170+
$pattern1 = preg_quote(
171+
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP#RP=https://localhost/idp#CSI=CL',
172+
'/',
173+
);
120174
$pattern2 = preg_quote(
121175
'#AM=' . Constants::AC_UNSPECIFIED
122176
. '#PN=e5d066a96d5809a21264e153013c3c793e6574cb77afdfa248ad2cefab9b0451#TS=1000#',
@@ -128,6 +182,109 @@ public function testSPwithUserId(): void
128182
}
129183

130184

185+
/**
186+
*/
187+
public function testSPwithUserIdSourceTargeted(): void
188+
{
189+
$config = ['federation' => 'ACME', 'logdest' => 'stdout', 'identifyingAttribute' => 'eduPersonPrincipalName', 'pnHashIsTargeted' => 'source',];
190+
$request = array_merge(self::$minRequest, self::$spRequest, [
191+
'Attributes' => [
192+
'eduPersonPrincipalName' => [ 'user2@example.net' ],
193+
],
194+
]);
195+
$pattern1 = preg_quote(
196+
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP#RP=https://localhost/idp#CSI=CL',
197+
'/',
198+
);
199+
$pattern2 = preg_quote(
200+
'#AM=' . Constants::AC_UNSPECIFIED
201+
. '#PN=d9b260a0830f4a93b407aaf0a578446880fc8acdc58cd81aecdcde12ec0f8cae#TS=1000#',
202+
'/',
203+
);
204+
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '$/');
205+
$result = self::processFilter($config, $request);
206+
$this->assertEquals($request, $result);
207+
}
208+
209+
210+
/**
211+
*/
212+
public function testSPwithUserIdSourceTargetedDifferentDest(): void
213+
{
214+
$config = ['federation' => 'ACME', 'logdest' => 'stdout', 'identifyingAttribute' => 'eduPersonPrincipalName', 'pnHashIsTargeted' => 'source',];
215+
$request = array_merge(self::$minRequest, self::$spRequest, [
216+
'Attributes' => [
217+
'eduPersonPrincipalName' => [ 'user2@example.net' ],
218+
],
219+
]);
220+
$request['Destination']['entityid'] = 'https://localhost/idp2';
221+
$pattern1 = preg_quote(
222+
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP#RP=https://localhost/idp2#CSI=CL',
223+
'/',
224+
);
225+
$pattern2 = preg_quote(
226+
'#AM=' . Constants::AC_UNSPECIFIED
227+
. '#PN=d9b260a0830f4a93b407aaf0a578446880fc8acdc58cd81aecdcde12ec0f8cae#TS=1000#',
228+
'/',
229+
);
230+
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '$/');
231+
$result = self::processFilter($config, $request);
232+
$this->assertEquals($request, $result);
233+
}
234+
235+
236+
/**
237+
*/
238+
public function testSPwithUserIdDestinationTargeted(): void
239+
{
240+
$config = ['federation' => 'ACME', 'logdest' => 'stdout', 'identifyingAttribute' => 'eduPersonPrincipalName', 'pnHashIsTargeted' => 'destination',];
241+
$request = array_merge(self::$minRequest, self::$spRequest, [
242+
'Attributes' => [
243+
'eduPersonPrincipalName' => [ 'user2@example.net' ],
244+
],
245+
]);
246+
$pattern1 = preg_quote(
247+
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP#RP=https://localhost/idp#CSI=CL',
248+
'/',
249+
);
250+
$pattern2 = preg_quote(
251+
'#AM=' . Constants::AC_UNSPECIFIED
252+
. '#PN=2497368e277bd4d6f848c268292e85cbe3fe4dfd0920b4ac2f5a419f523d4374#TS=1000#',
253+
'/',
254+
);
255+
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '$/');
256+
$result = self::processFilter($config, $request);
257+
$this->assertEquals($request, $result);
258+
$request['saml:sp:IdP'] = 'https://localhost/saml:sp:IdP2';
259+
}
260+
261+
262+
/**
263+
*/
264+
public function testSPwithUserIdDestinationTargetedDifferentSource(): void
265+
{
266+
$config = ['federation' => 'ACME', 'logdest' => 'stdout', 'identifyingAttribute' => 'eduPersonPrincipalName', 'pnHashIsTargeted' => 'destination',];
267+
$request = array_merge(self::$minRequest, self::$spRequest, [
268+
'Attributes' => [
269+
'eduPersonPrincipalName' => [ 'user2@example.net' ],
270+
],
271+
]);
272+
$request['saml:sp:IdP'] = 'https://localhost/saml:sp:IdP2';
273+
$pattern1 = preg_quote(
274+
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP2#RP=https://localhost/idp#CSI=CL',
275+
'/',
276+
);
277+
$pattern2 = preg_quote(
278+
'#AM=' . Constants::AC_UNSPECIFIED
279+
. '#PN=2497368e277bd4d6f848c268292e85cbe3fe4dfd0920b4ac2f5a419f523d4374#TS=1000#',
280+
'/',
281+
);
282+
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '$/');
283+
$result = self::processFilter($config, $request);
284+
$this->assertEquals($request, $result);
285+
}
286+
287+
131288
/**
132289
*/
133290
public function testAsIdentityProvider(): void
@@ -144,7 +301,7 @@ public function testAsIdentityProvider(): void
144301
);
145302
$pattern2 = preg_quote(
146303
'#AM=' . Constants::AC_PASSWORD
147-
. '#PN=d844a9a0666bb3990e88f72b8f5c20accbcfa46f7b8a7ab38593bfbbab6e9cbc#TS=',
304+
. '#PN=16ed2263078ca90f38708681fcf6628d80e0f91f4b5d743054fe8e185c9e0979#TS=',
148305
'/',
149306
);
150307
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '\d+#$/');
@@ -218,7 +375,7 @@ public function testFilteringString(): void
218375
'/',
219376
);
220377
$pattern2 = preg_quote(
221-
'#PN=d844a9a0666bb3990e88f72b8f5c20accbcfa46f7b8a7ab38593bfbbab6e9cbc#TS=',
378+
'#PN=16ed2263078ca90f38708681fcf6628d80e0f91f4b5d743054fe8e185c9e0979#TS=',
222379
'/',
223380
);
224381
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '\d+#$/');

0 commit comments

Comments
 (0)