aspnet / AspNetKatana

Microsoft's OWIN implementation, the Katana project
Apache License 2.0
967 stars 334 forks source link

Owin.OpenIdConnect deadlock at cold start with IIS #220

Closed ZOXEXIVO closed 6 years ago

ZOXEXIVO commented 6 years ago

We have some project, running on IIS. After cold start, sometimes we have deadlock, caused by method:

private static void OnSendingHeaderCallback(object state)
{
     AuthenticationHandler handler = (AuthenticationHandler)state;
     handler.ApplyResponseAsync().Wait();
}

We can't do OpenIdConnect redirect and application freezes. Fixed only by iisreset

DotTrace show this

image

Tratcher commented 6 years ago

Related: https://github.com/aspnet/AspNetKatana/issues/51

For this to hang ApplyResponseAsync must actually go async. Most of the time it will complete sync, but at startup it needs to fetch the metadata document. https://github.com/aspnet/AspNetKatana/blob/e2b18ec84ceab7ffa29d80d89429c9988ab40144/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L138

Possible workaround: Initialize and invoke OpenIdConnectOptions.ConfigurationManager at Startup to fetch the metadata before the first request (or load the metadata statically). See https://github.com/aspnet/AspNetKatana/blob/e2b18ec84ceab7ffa29d80d89429c9988ab40144/src/Microsoft.Owin.Security.OpenIdConnect/OpenIdConnectAuthenticationMiddleware.cs#L70-L102

muratg commented 6 years ago

Closing this one.

ghanchiasif commented 5 years ago

I did similar thing as per your guidance but and also kept the code to check if config value is null then it will call retrieve metadata. And noticed that it is getting null suddenly (after multiple requests in production) for config and going to retrieve metadata and getting stuck and blocking complete service. I don't know if config will be null for each requests after that because our service is getting stuck and doesn't allow to get any thread to read further.

Please refer below code and suggest solution for this.


using System;
using System.Collections.Generic;
using System.Globalization;
using System.IdentityModel.Tokens;
using System.Threading;
using AppInsightsTelemetry;
using Microsoft.IdentityModel.Protocols;
using Microsoft.Owin.Security.Jwt;

namespace MCred.API
{
    // This class is necessary because the OAuthBearer Middleware does not leverage
    // the OpenID Connect metadata endpoint exposed by the STS by default.

    public class OpenIdConnectSecurityTokenProvider : IIssuerSecurityTokenProvider
    {
        public ConfigurationManager<OpenIdConnectConfiguration> _configManager;
        private string _issuer;
        private IEnumerable<SecurityToken> _tokens;
        private OpenIdConnectConfiguration _config;

        private readonly ReaderWriterLockSlim _synclock = new ReaderWriterLockSlim();

        public OpenIdConnectSecurityTokenProvider(string metadataEndpoint)
        {
            Telemetry.TraceInformation("Started", nameof(OpenIdConnectSecurityTokenProvider), ConfigHelper.AppName, false);
            _configManager = new ConfigurationManager<OpenIdConnectConfiguration>(metadataEndpoint);
            RetrieveMetadata(true);
        }

        /// <summary>
        /// Gets the issuer the credentials are for.
        /// </summary>
        /// <value>
        /// The issuer the credentials are for.
        /// </value>
        public string Issuer
        {
            get
            {
                Telemetry.TraceInformation("Started", nameof(Issuer), ConfigHelper.AppName, false);

                bool lockStatus = false;
                try
                {
                    if (_config == null)
                    {
                        RetrieveMetadata(false);
                    }

                    Telemetry.TraceInformation("EnterReadLock", nameof(Issuer), ConfigHelper.AppName, false);

                    lockStatus = _synclock.TryEnterReadLock(TimeSpan.FromSeconds(ConfigHelper.locktimeout));
                    if (lockStatus)
                    {
                        return _issuer;
                    }
                    else
                    {
                        Telemetry.TraceException("Timeout occurred while acquiring read lock for fetching AAD OpenID Metadata", nameof(Issuer), ConfigHelper.AppName, false);
                        return null;
                    }
                }
                catch (Exception ex)
                {
                    Telemetry.TraceException(ex.Message, nameof(Issuer), ConfigHelper.AppName, false);
                    throw;
                }
                finally
                {
                    Telemetry.TraceInformation("ExitReadLock", nameof(Issuer), ConfigHelper.AppName, false);
                    if (lockStatus)
                    {
                        _synclock.ExitReadLock();
                    }
                }
            }
        }

        /// <summary>
        /// Gets all known security tokens.
        /// </summary>
        /// <value>
        /// All known security tokens.
        /// </value>
        public IEnumerable<SecurityToken> SecurityTokens
        {
            get
            {
                Telemetry.TraceInformation("Started", nameof(SecurityTokens), ConfigHelper.AppName, false);

                bool lockStatus = false;
                try
                {
                    if (_config == null)
                    {
                        RetrieveMetadata(false);
                    }

                    Telemetry.TraceInformation("EnterReadLock", nameof(SecurityTokens), ConfigHelper.AppName, false);

                    lockStatus = _synclock.TryEnterReadLock(TimeSpan.FromSeconds(ConfigHelper.locktimeout));
                    if (lockStatus)
                    {
                        return _tokens;
                    }
                    else
                    {
                        Telemetry.TraceException("Timeout occurred while acquiring read lock for fetching AAD OpenID Metadata", nameof(SecurityTokens), ConfigHelper.AppName, false);
                        return null;
                    }
                }
                catch (Exception ex)
                {
                    Telemetry.TraceException(ex.Message, nameof(SecurityTokens), ConfigHelper.AppName, false);
                    throw;
                }
                finally
                {
                    Telemetry.TraceInformation("ExitReadLock", nameof(SecurityTokens), ConfigHelper.AppName, false);
                    if (lockStatus)
                    {
                        _synclock.ExitReadLock();
                    }
                }
            }
        }

        private void RetrieveMetadata(bool IsConstructor)
        {
            Telemetry.TraceInformation("Started from IsConstructor = " + IsConstructor.ToString(), nameof(RetrieveMetadata), ConfigHelper.AppName, false);

            if (_config == null)
            {
                Telemetry.TraceInformation("EnterWriteLock from IsConstructor = " + IsConstructor.ToString(), nameof(RetrieveMetadata), ConfigHelper.AppName, false);

                if (_synclock.TryEnterWriteLock(TimeSpan.FromSeconds(ConfigHelper.locktimeout)))
                {
                    try
                    {
                        if (_config == null)
                        {
                            Telemetry.TraceInformation("GetConfigurationAsync from IsConstructor = " + IsConstructor.ToString(), nameof(RetrieveMetadata), ConfigHelper.AppName, false);
                            _config = _configManager.GetConfigurationAsync().Result;
                            _issuer = _config.Issuer;
                            _tokens = _config.SigningTokens;
                        }
                    }
                    catch (Exception ex)
                    {
                        Telemetry.TraceException(ex.Message, nameof(RetrieveMetadata), ConfigHelper.AppName, false);
                        throw;
                    }
                    finally
                    {
                        Telemetry.TraceInformation("ExitWriteLock from IsConstructor = " + IsConstructor.ToString(), nameof(RetrieveMetadata), ConfigHelper.AppName, false);
                        _synclock.ExitWriteLock();
                    }
                }
                else
                {
                    Telemetry.TraceException("Timeout occurred while acquiring write lock for fetching AAD OpenID Metadata from IsConstructor = " + IsConstructor.ToString(), nameof(RetrieveMetadata), ConfigHelper.AppName, false);
                }
            }
        }
    }
}
Tratcher commented 5 years ago

Comments on closed issues are not tracked, please open a new issue with the details for your scenario.

Check here for an example that avoid blocking request threads: https://github.com/aspnet/AspNetKatana/blob/e2b18ec84ceab7ffa29d80d89429c9988ab40144/src/Microsoft.Owin.Security.ActiveDirectory/WsFedCachingSecurityKeyProvider.cs

Why do you have null checks for _config in Issuer and SecurityTokens when it's always initialized in the constructor?

Tratcher commented 5 years ago

Reading that closer, there's no way for _config to become null after the constructor. If RetrieveMetadata threw in the constructor the entire object never would have been created.