IdPv3: race condition causes two persistentId to be generated

[Dominique Petitpierre]

Hello Scott,

On 09/23/2015 08:28 PM, Cantor, Scott wrote:
> On 9/23/15, 2:12 PM, "users on behalf of Dominique Petitpierre" <users-bounces at shibboleth.net on behalf of Dominique.Petitpierre at unige.ch> wrote:
>>
>> 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.
>
> That just moves the race condition to the database (if locking is optimistic) or triggers errors (if pessimistic). It will take more work than that to fix, and ultimately will require work by deployers in most cases to define appropriate error messages if we try and deal with it using retry logic.

OK. We can live with duplicate entries (tolerant database + a nightly 
batch will remove them), we are happy with computed persistentIds and we 
don't really need persistentId deactivation and regeneration (if ever, 
we'll do that "by hand").
So, another suggestion is to have a simple boolean property that would 
control if one wants to disable the regeneration logic in 
StoredPersistentIdGenerationStrategy.java.
We would disable it and thus never store multiple different 
persistentIds for the same keys, just duplicates.

> The problem is that the two selects (or a select and insert) have to occur under a table lock to be run in separate steps, and only a small number of databases correctly take a table lock when they're told to do so. A real RDBMS that works makes the code very simple to write, but unfortunately most RDBMS' these days are being broken to favor performance over correct behavior. SQL Server is about the only one I know for certain still provides correctly isolated transactions.

Maybe Shibboleth IdP could integrate one chosen noSQL storage library or 
other key-value system simpler than a RDBMS to handle it's own rather 
simple persistent data (like persistentId or user consent).
For deployers that would mean less headaches for the conception and 
construction of a load balancing and high availability architecture.

>> - Would this be possible for IdP 3.2?
>
> First it needs to be filed in Jira and the work scoped.

Currently I don't have access to Shibboleth JIRA. But as soon as this is 
corrected I will file this issue.


>> 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).
>
> It uses cookies. That means it can never support iframes, unless you require third party cookies be turned on.
> Somebody with more time than I have to work on it might be able to figure out how to move the JSESSIONID value into the URL. That's the only way it could ever work, but that's also less secure.

Well, the way it works now in IdPv3 is good enough for us. Please don't 
break it!
In IdP 2.4.x we used code developed at Humboldt-Universitaet zu Berlin 
that moved the login context value into the URL. But we'd rather not 
take such a route again!
cf. 
https://lists.internet2.edu/sympa/arc/shibboleth-dev/2010-12/msg00018.html



Thanks again for the very detailed answers!

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