IdPv3: race condition causes two persistentId to be generated

[Dominique Petitpierre]

Hello Scott,

thanks for the explanations and comments:

On 09/23/2015 06:15 PM, Cantor, Scott wrote:
> On 9/23/15, 11:28 AM, "users on behalf of Dominique Petitpierre" <users-bounces at shibboleth.net on behalf of Dominique.Petitpierre at unige.ch> wrote:
>>
>> One question:
>>
>> a) Why does StoredPersistentIdGenerationStrategy generate a new
>>     persistentId instead of reporting an error?  I.e. IMHO re-assigning
>>     a new persistentId is a delicate matter and should be done only
>>     under the control of a human by a special tool, not the IdP.
>
> Because getCount() is ignoring the deactivation column whereas the earlier check for an existing ID doesn't. The race is caused in part by the difference in those queries. Deactivating the older ID causes it to generate a new one. That *is* the human control mechanism. The race is between checking for an active ID and then checking how to generate a new one.

OK. So changing getCount() to a getActiveCount() and check if the Id to 
be stored is the same as the one stored would be a quick fix.

- Would this be possible for IdP 3.2?


>> c) To avoid thatgetCount() two identical persistentId be stored (second case
>>     above), may be one could rely on a database (or other storage)
>>     constraint preventing duplicates, and for the second thread to
>>     ignore the duplicate entry error message from the database.
>
> That's not a standard error. Every deployer would have to supply the right message to look for. Without fully transactional semantics underneath (i.e. a full table lock), there will always be a race condition between the two selects and problematic handling of the result. It's very, very hard to make it work based on the experience we have had with the storage service unless we do our own locking (which means occasional cleanup by the sysadmin when stuff fails badly) or basically punt and blame the database when it doesn't. We can fix the race condition in the code, but it will be harder to actually fix it in the database because setting the isolation level in most databases doesn't actually do what it's defined to do.
>
> Of course, we can always outright fail if the database does, it's just that the outcome will be failed identifier generation. That might be good enough given the fact that iframes in general do not work and are not supported anyway.

That is unfortunate: we don't have the control on how web applications 
are implemented.
I was under the impression that on the contrary IdPv3 would support or 
somehow tolerate iframes
(in SSO mode).


> That still leaves tabs though. We can also do what the SP does and provide a rertryable error control where the right messages can be fed in by the deployer to figure out what's a real error and what's a database bug not honoring the rules of the road.
>
> Anyway, bottom line is that the code was skating by sloppy before because of V2's other bugs and now it will have to be rewritten to be correct, but making it harder to use.

If the code is rewritten it would be nice if it was integrated in the
StorageService architecture.  For example one could then use a
memcached implementation with persistence (and high availibility) to
store both persistentIds and user consent.

Thanks again for your answer!

Regards,
Dominique Petitpierre
--
Mr Dominique Petitpierre, user=Dominique.Petitpierre domain=unige.ch
IT Division, University of Geneva, Switzerland