Skip to content

Commit 4cfd085

Browse files
tvdijentvdijen
authored
Merge pull request #7 from ghalse/bug-pn-calculation
Fix computation of the PN hash
2 parents b0089ba + 2d1c425 commit 4cfd085

File tree

4 files changed

+263
-18
lines changed

4 files changed

+263
-18
lines changed

‎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: 29 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,9 @@ class Fticks extends Auth\ProcessingFilter
7777
/** @var array F-ticks attributes to exclude */
7878
private array $exclude = [];
7979

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

8184
/**
8285
* Log a message to the desired destination
@@ -168,12 +171,16 @@ private function generatePNhash(array &$state): ?string
168171
/* calculate a hash */
169172
if ($uid !== null) {
170173
$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'];
174+
if (in_array($this->pnHashIsTargeted, ['source', 'both'])) {
175+
if (array_key_exists('saml:sp:IdP', $state)) {
176+
$userdata .= strlen($state['saml:sp:IdP']) . ':' . $state['saml:sp:IdP'];
177+
} else {
178+
$userdata .= strlen($state['Source']['entityid']) . ':' . $state['Source']['entityid'];
179+
}
180+
}
181+
if (in_array($this->pnHashIsTargeted, ['destination', 'both'])) {
182+
$userdata .= strlen($state['Destination']['entityid']) . ':' . $state['Destination']['entityid'];
175183
}
176-
$userdata .= strlen($state['Destination']['entityid']) . ':' . $state['Destination']['entityid'];
177184
$userdata .= strlen($uid) . ':' . $uid;
178185
$userdata .= $this->salt;
179186

@@ -257,17 +264,24 @@ public function __construct(array $config, $reserved)
257264
}
258265
}
259266

267+
if (array_key_exists('pnHashIsTargeted', $config)) {
268+
Assert::string($config['pnHashIsTargeted'], 'pnHashIsTargeted must be a string');
269+
Assert::oneOf(
270+
$config['pnHashIsTargeted'],
271+
['source', 'destination', 'both', 'none'],
272+
'pnHashIsTargeted must be one of [source, destnation, both, none]',
273+
);
274+
$this->pnHashIsTargeted = $config['pnHashIsTargeted'];
275+
}
276+
260277
if (array_key_exists('logdest', $config)) {
261-
if (
262-
is_string($config['logdest']) &&
263-
in_array($config['logdest'], ['local', 'syslog', 'remote', 'stdout', 'errorlog', 'simplesamlphp'])
264-
) {
265-
$this->logdest = $config['logdest'];
266-
} else {
267-
throw new Error\Exception(
268-
'F-ticks log destination must be one of [local, remote, stdout, errorlog, simplesamlphp]',
269-
);
270-
}
278+
Assert::string($config['logdest'], 'F-ticks log destination must be a string');
279+
Assert::oneOf(
280+
$config['logdest'],
281+
['local', 'syslog', 'remote', 'stdout', 'errorlog', 'simplesamlphp'],
282+
'F-ticks log destination must be one of [local, remote, stdout, errorlog, simplesamlphp]',
283+
);
284+
$this->logdest = $config['logdest'];
271285
}
272286

273287
/* match SSP config or we risk mucking up the openlog call */

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

Lines changed: 183 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;
@@ -117,6 +118,63 @@ public function testSPwithUserId(): void
117118
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP#RP=https://localhost/idp#CSI=CL',
118119
'/',
119120
);
121+
$pattern2 = preg_quote(
122+
'#AM=' . Constants::AC_UNSPECIFIED
123+
. '#PN=d63bb55765af1321b06950abb5f9787cffd05ef271a09b67964f402f3f209cc6#TS=1000#',
124+
'/',
125+
);
126+
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '$/');
127+
$result = self::processFilter($config, $request);
128+
$this->assertEquals($request, $result);
129+
}
130+
131+
132+
/**
133+
*/
134+
public function testSPwithUserIdDifferentProviders(): void
135+
{
136+
$config = ['federation' => 'ACME', 'logdest' => 'stdout', 'identifyingAttribute' => 'eduPersonPrincipalName'];
137+
$request = array_merge(self::$minRequest, self::$spRequest, [
138+
'Attributes' => [
139+
'eduPersonPrincipalName' => [ '[email protected]' ],
140+
],
141+
]);
142+
$request['Destination']['entityid'] = 'https://localhost/idp2';
143+
$request['saml:sp:IdP'] = 'https://localhost/saml:sp:IdP2';
144+
$pattern1 = preg_quote(
145+
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP2#RP=https://localhost/idp2#CSI=CL',
146+
'/',
147+
);
148+
$pattern2 = preg_quote(
149+
'#AM=' . Constants::AC_UNSPECIFIED
150+
. '#PN=d63bb55765af1321b06950abb5f9787cffd05ef271a09b67964f402f3f209cc6#TS=1000#',
151+
'/',
152+
);
153+
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '$/');
154+
$result = self::processFilter($config, $request);
155+
$this->assertEquals($request, $result);
156+
}
157+
158+
159+
/**
160+
*/
161+
public function testSPwithUserIdLegacyBehaviour(): void
162+
{
163+
$config = [
164+
'federation' => 'ACME',
165+
'logdest' => 'stdout',
166+
'identifyingAttribute' => 'eduPersonPrincipalName',
167+
'pnHashIsTargeted' => 'both',
168+
];
169+
$request = array_merge(self::$minRequest, self::$spRequest, [
170+
'Attributes' => [
171+
'eduPersonPrincipalName' => [ '[email protected]' ],
172+
],
173+
]);
174+
$pattern1 = preg_quote(
175+
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP#RP=https://localhost/idp#CSI=CL',
176+
'/',
177+
);
120178
$pattern2 = preg_quote(
121179
'#AM=' . Constants::AC_UNSPECIFIED
122180
. '#PN=e5d066a96d5809a21264e153013c3c793e6574cb77afdfa248ad2cefab9b0451#TS=1000#',
@@ -128,6 +186,129 @@ public function testSPwithUserId(): void
128186
}
129187

130188

189+
/**
190+
*/
191+
public function testSPwithUserIdSourceTargeted(): void
192+
{
193+
$config = [
194+
'federation' => 'ACME',
195+
'logdest' => 'stdout',
196+
'identifyingAttribute' => 'eduPersonPrincipalName',
197+
'pnHashIsTargeted' => 'source',
198+
];
199+
$request = array_merge(self::$minRequest, self::$spRequest, [
200+
'Attributes' => [
201+
'eduPersonPrincipalName' => [ '[email protected]' ],
202+
],
203+
]);
204+
$pattern1 = preg_quote(
205+
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP#RP=https://localhost/idp#CSI=CL',
206+
'/',
207+
);
208+
$pattern2 = preg_quote(
209+
'#AM=' . Constants::AC_UNSPECIFIED
210+
. '#PN=d9b260a0830f4a93b407aaf0a578446880fc8acdc58cd81aecdcde12ec0f8cae#TS=1000#',
211+
'/',
212+
);
213+
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '$/');
214+
$result = self::processFilter($config, $request);
215+
$this->assertEquals($request, $result);
216+
}
217+
218+
219+
/**
220+
*/
221+
public function testSPwithUserIdSourceTargetedDifferentDest(): void
222+
{
223+
$config = [
224+
'federation' => 'ACME',
225+
'logdest' => 'stdout',
226+
'identifyingAttribute' => 'eduPersonPrincipalName',
227+
'pnHashIsTargeted' => 'source',
228+
];
229+
$request = array_merge(self::$minRequest, self::$spRequest, [
230+
'Attributes' => [
231+
'eduPersonPrincipalName' => [ '[email protected]' ],
232+
],
233+
]);
234+
$request['Destination']['entityid'] = 'https://localhost/idp2';
235+
$pattern1 = preg_quote(
236+
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP#RP=https://localhost/idp2#CSI=CL',
237+
'/',
238+
);
239+
$pattern2 = preg_quote(
240+
'#AM=' . Constants::AC_UNSPECIFIED
241+
. '#PN=d9b260a0830f4a93b407aaf0a578446880fc8acdc58cd81aecdcde12ec0f8cae#TS=1000#',
242+
'/',
243+
);
244+
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '$/');
245+
$result = self::processFilter($config, $request);
246+
$this->assertEquals($request, $result);
247+
}
248+
249+
250+
/**
251+
*/
252+
public function testSPwithUserIdDestinationTargeted(): void
253+
{
254+
$config = [
255+
'federation' => 'ACME',
256+
'logdest' => 'stdout',
257+
'identifyingAttribute' => 'eduPersonPrincipalName',
258+
'pnHashIsTargeted' => 'destination',
259+
];
260+
$request = array_merge(self::$minRequest, self::$spRequest, [
261+
'Attributes' => [
262+
'eduPersonPrincipalName' => [ '[email protected]' ],
263+
],
264+
]);
265+
$pattern1 = preg_quote(
266+
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP#RP=https://localhost/idp#CSI=CL',
267+
'/',
268+
);
269+
$pattern2 = preg_quote(
270+
'#AM=' . Constants::AC_UNSPECIFIED
271+
. '#PN=2497368e277bd4d6f848c268292e85cbe3fe4dfd0920b4ac2f5a419f523d4374#TS=1000#',
272+
'/',
273+
);
274+
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '$/');
275+
$result = self::processFilter($config, $request);
276+
$this->assertEquals($request, $result);
277+
$request['saml:sp:IdP'] = 'https://localhost/saml:sp:IdP2';
278+
}
279+
280+
281+
/**
282+
*/
283+
public function testSPwithUserIdDestinationTargetedDifferentSource(): void
284+
{
285+
$config = [
286+
'federation' => 'ACME',
287+
'logdest' => 'stdout',
288+
'identifyingAttribute' => 'eduPersonPrincipalName',
289+
'pnHashIsTargeted' => 'destination',
290+
];
291+
$request = array_merge(self::$minRequest, self::$spRequest, [
292+
'Attributes' => [
293+
'eduPersonPrincipalName' => [ '[email protected]' ],
294+
],
295+
]);
296+
$request['saml:sp:IdP'] = 'https://localhost/saml:sp:IdP2';
297+
$pattern1 = preg_quote(
298+
'F-TICKS/ACME/1.0#RESULT=OK#AP=https://localhost/saml:sp:IdP2#RP=https://localhost/idp#CSI=CL',
299+
'/',
300+
);
301+
$pattern2 = preg_quote(
302+
'#AM=' . Constants::AC_UNSPECIFIED
303+
. '#PN=2497368e277bd4d6f848c268292e85cbe3fe4dfd0920b4ac2f5a419f523d4374#TS=1000#',
304+
'/',
305+
);
306+
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '$/');
307+
$result = self::processFilter($config, $request);
308+
$this->assertEquals($request, $result);
309+
}
310+
311+
131312
/**
132313
*/
133314
public function testAsIdentityProvider(): void
@@ -144,7 +325,7 @@ public function testAsIdentityProvider(): void
144325
);
145326
$pattern2 = preg_quote(
146327
'#AM=' . Constants::AC_PASSWORD
147-
. '#PN=d844a9a0666bb3990e88f72b8f5c20accbcfa46f7b8a7ab38593bfbbab6e9cbc#TS=',
328+
. '#PN=16ed2263078ca90f38708681fcf6628d80e0f91f4b5d743054fe8e185c9e0979#TS=',
148329
'/',
149330
);
150331
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '\d+#$/');
@@ -218,7 +399,7 @@ public function testFilteringString(): void
218399
'/',
219400
);
220401
$pattern2 = preg_quote(
221-
'#PN=d844a9a0666bb3990e88f72b8f5c20accbcfa46f7b8a7ab38593bfbbab6e9cbc#TS=',
402+
'#PN=16ed2263078ca90f38708681fcf6628d80e0f91f4b5d743054fe8e185c9e0979#TS=',
222403
'/',
223404
);
224405
$this->expectOutputRegex('/^' . $pattern1 . '[^#]+' . $pattern2 . '\d+#$/');

0 commit comments

Comments
 (0)