[Bug] Race condition on ConfigurationManager._syncAfter

[bruno-brant]

Which version of Microsoft.IdentityModel are you using? Microsoft.IdentityModel 8.1.2

Where is the issue?

• [ ] M.IM.JsonWebTokens
• [ ] M.IM.KeyVaultExtensions
• [ ] M.IM.Logging
• [ ] M.IM.ManagedKeyVaultSecurityKey
• [X] M.IM.Protocols
• [ ] M.IM.Protocols.OpenIdConnect
• [ ] M.IM.Protocols.SignedHttpRequest
• [ ] M.IM.Protocols.WsFederation
• [ ] M.IM.TestExtensions
• [ ] M.IM.Tokens
• [ ] M.IM.Tokens.Saml
• [ ] M.IM.Validators
• [ ] M.IM.Xml
• [ ] S.IM.Tokens.Jwt
• Other (please describe)

Description

Every calling thread to ConfigurationManager.GetConfigurationAsync will result in a read to _syncAfter to check if a sync should be started; later, a background thread will update _syncAfter with the new sync time. syncAfter is a DateTimeOffset, a struct, so writes to it aren't atomic, so there's a possibility that the calling thread will read corrupt data.

Possible solution

• Rewrite _syncAfter so that it can be atomically read/written - we could refactor it to be total seconds (int32) or any other type that is atomic.
• Rearchitect the metadata refresh to happen completely in the background, to avoid every calling thread having to access the syncAfter variable.

Additional context / logs / screenshots / links to code

• _syncAfter read

[jennyf19]

FYI @brentschmaltz

[brentschmaltz]

I like the idea to use Epoch time as a long.

[bruno-brant]

I like the idea to use Epoch time as a long.

Longs can still cause an issue in 32-bit architectures unless we use Interlocked or any other synchronization strategy. A int32 is always atomic.

Given the use case, maybe an int32 would suffice? I know the max time shouldn't larger than an int.MaxValue, not sure if the subtraction is cheaper than an interlocked though.

[keegan-caruso]

@brentschmaltz We don't need to store the epoch time, just the offset, which should always be capturable in an int. I think this is the point @bruno-brant is trying to make.

[brentschmaltz]

@keegan-caruso that makes sense also, hadn't thought about 32bit architectures, that is a good point and should be considered.