Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redis DataProtection start to work too late #11790

Closed
mazuryv opened this issue May 31, 2022 · 16 comments
Closed

Redis DataProtection start to work too late #11790

mazuryv opened this issue May 31, 2022 · 16 comments
Milestone

Comments

@mazuryv
Copy link
Contributor

mazuryv commented May 31, 2022

Hello.
We use 2 hosts to deploy our solution based on Orchard.Core.
Relation with them ensure with Redis master-slave/sentinel approach.
Modules are enabled:

  • "OrchardCore.Redis.Cache",
  • "OrchardCore.Redis.Bus",
  • "OrchardCore.Redis.DataProtection",
  • "OrchardCore.Redis.Lock",
  • "OrchardCore.Tenants.Distributed" ,
  • "OrchardCore.OpenId",
  • "OrchardCore.OpenId.Management",
  • "OrchardCore.OpenId.Server",
  • "OrchardCore.OpenId.Validation".
  "OrchardCore": {
    "OrchardCore_Redis": {
      "Configuration": "localhost:26379,password=secret,serviceName=master,allowAdmin=true", 
      "InstancePrefix": "OurSite" 
    },
    "OrchardCore_Shells_Database": {
      "DatabaseProvider": "Oracle", // Set to a supported database provider.
      "ConnectionString": "{connection string}", // Set to the database connection string.
      "TablePrefix": "", // Optionally, configure a table prefix.
      "MigrateFromFiles": true // Optionally, enable to migrate existing App_Data files to Database automatically.
    }
  }

When application is starting:

  • Microsoft.Extensions.DependencyInjection.ServiceCollectionExtensions method AddDataProtection(OrchardCoreBuilder builder)
    var collection = new ServiceCollection()
        .AddDataProtection()
        .PersistKeysToFileSystem(directory)
        .SetApplicationName(settings.Name)
        .AddKeyManagementOptions(o => o.XmlEncryptor = o.XmlEncryptor ?? new NullXmlEncryptor())
        .Services;

    // Remove any previously registered options setups.
    services.RemoveAll<IConfigureOptions<KeyManagementOptions>>();
    services.RemoveAll<IConfigureOptions<DataProtectionOptions>>();

and looks like protection is start to use with inMemory key.
Later configured new KeyManagementOptions

[Feature("OrchardCore.Redis.DataProtection")]
    public class RedisDataProtectionStartup : StartupBase
    {
        public override void ConfigureServices(IServiceCollection services)
        {
            if (services.Any(d => d.ServiceType == typeof(IRedisService)))
            {
                services.AddTransient<IConfigureOptions<KeyManagementOptions>, RedisKeyManagementOptionsSetup>();
            }
        }
    }

and use key from Redis key:
image

But when system uses not Redis dataprotection key OpenIdServerService start to create new certificates:
Old certificates can't be read (password can't extract the certificate password from the separate .pwd file.)
And new certificates are created!!!
They are created each time when installation is started.
Later OpenIdBackgroundTask throws warning message about all old certificates each 30 mins!!!
image

I propose:

  • Fix DataProtection for use only Redis key
  • Or remove OpenId certificates which can't be opened to another directory ("Invalid")

What do you think about that?

@jptissot
Copy link
Member

@kevinchalet @jtkech may have some insights here.

@Skrypt
Copy link
Contributor

Skrypt commented May 31, 2022

The issue starts from the fact that your solution is using Database Shell settings instead of Blob Storage. We currently don't save the certificates in the database which would probably solve the issue. Else, if you have 2 different hosts it will save a new certificate in each of them which will cause the issue. Your 2 hosts would need at least to use the same shared App_Data folder but it is not recommended to do so because of concurrency issues.

So, yeah, I think it is a missing implementation for the Redis feature and the OpenId feature (certificates).

In the mean time, the workaround is to use the shared blob storage settings feature "OrchardCore_DataProtection_Azure" and "OrchardCore_Shells_Azure" instead of "OrchardCore_Shells_Database".

@mazuryv
Copy link
Contributor Author

mazuryv commented May 31, 2022

Thank you, but we may not use "OrchardCore_DataProtection_Azure" and "OrchardCore_Shells_Azure". Our application should deploy only in local network. How can we store certificates in the database?

@Skrypt
Copy link
Contributor

Skrypt commented May 31, 2022

I will take a look if @jtkech doesn't

@Skrypt
Copy link
Contributor

Skrypt commented May 31, 2022

I believe it starts from there :

public async Task<ImmutableArray<SecurityKey>> GetEncryptionKeysAsync()

@jtkech
Copy link
Member

jtkech commented Jun 1, 2022

I will try to look at it too when I will have time

Yes, need to understand why it first still uses a FileSystemXmlRepository, not a RedisXmlRepository.

@sebastienros sebastienros added this to the 1.x milestone Jun 2, 2022
@jtkech
Copy link
Member

jtkech commented Jun 3, 2022

@mazuryv

Maybe related to #11501 (comment), I can repro locally the same kind of configuration, so I will try to investigate more when I will have time.

@jtkech
Copy link
Member

jtkech commented Jun 5, 2022

@mazuryv

Okay, I made some progress on #11501 with the same kind of config locally, and now I can debug in pod containers, so I started some first tests. For now just put some breakpoints and enabled the openId server,

In the OpenIdServerService ctor I saw that the data protector uses the right RedisXmlRepository.

Redis DP 1

Then I saw that it persists a certificate, the data protector still using a RedisXmlRepository.

Redis DP 2

Then, when reloading the tenant, I saw that it doesn't persist again a certificate but read a password.

Redis DP 3

This is where I"m, for now all seems to be as expected ;)

Will see this week end if I can find something.

@mazuryv
Copy link
Contributor Author

mazuryv commented Jun 5, 2022

Hello @jtkech
The issue happens only when all application started after some timespan, not tenant reload or application recycle by IIS.
We deploy our application every morning and I can see new certificates only in hosts.
I can't to reproduce it during debug.
But I try to change Order

[Feature("OrchardCore.Redis.DataProtection")]
    public class RedisDataProtectionStartup : StartupBase
    {
        public override int Order => 1;
        public override void ConfigureServices(IServiceCollection services)
        {
            if (services.Any(d => d.ServiceType == typeof(IRedisService)))
            {
                services.AddTransient<IConfigureOptions<KeyManagementOptions>, RedisKeyManagementOptionsSetup>();
            }
        }
    }

It works without new certificate creation during 3 days, but today DataProtection key has been changed for some reason (I don't know why).
For decrease Warning message count I have added in OpenIdServerService

        private void RemoveCertificateAsync(string path)
        {
            try
            {
                var destinationDirectory = Path.Combine(Path.GetDirectoryName(path), "Unencrypted");
                if (!Directory.Exists(destinationDirectory))
                {
                    Directory.CreateDirectory(destinationDirectory);
                }
                var certificateFileName = Path.GetFileName(path);
                var passwordFilePath = Path.ChangeExtension(path, ".pwd");
                var passwordFileName = Path.GetFileName(passwordFilePath);

                if (File.Exists(path))
                {
                    File.Move(path, Path.Combine(destinationDirectory, certificateFileName));
                }

                if (File.Exists(passwordFilePath))
                {
                    File.Move(passwordFilePath, Path.Combine(destinationDirectory, passwordFileName));
                }
            }
            catch (Exception exception)
            {
                _logger.LogWarning(exception, "An error occurred while trying to move a X.509 certificate to Unencrypted path.");
            }
        }

But maybe we should use single certificate store (in DB for example) in multi host configuration?

@jtkech
Copy link
Member

jtkech commented Jun 5, 2022

@mazuryv

It works without new certificate creation during 3 days

Okay, good to know, I saw in the code that the lifetime is around 7 days.

but today DataProtection key has been changed for some reason (I don't know why).

Okay, so I don't think it is a config order concern.

When you do a new deployment, I assume that the .pfx and .pwd files are still there because you are moving the .pwd files to limit the warnings. So, as I understand, the only reason that would explain the issue is that you lost the redis DP keys.

Are your redis instance(s) configured for any persistency?


For info, a startup has an Order property and also a ConfigureOrder that we can also override, useful when a startup has both a ConfigureServices() and Configure() and need to have different order. But if you don't override ConfigureOrder, it"s value will just come from the Order value.

@mazuryv
Copy link
Contributor Author

mazuryv commented Jun 10, 2022

@jtkech
Thank you for your help.
I have configured persistence in Redis as Snapshots and all works correctly for us now.
Sorry for my fault.

@mazuryv mazuryv closed this as completed Jun 10, 2022
@jtkech
Copy link
Member

jtkech commented Jun 10, 2022

@mazuryv

Cool, good to know that it now works on your side.
Thank you for letting us know ;)

@micrub-it
Copy link

Hi,
with my Redis configuration with sentinel I experienced this error:
System.NotImplementedException: The ConnectionMultiplexer is not a Sentinel connection.
at StackExchange.Redis.ConnectionMultiplexer.GetSentinelMasterConnection(ConfigurationOptions config, TextWriter log) in //src/StackExchange.Redis/ConnectionMultiplexer.cs:line 2354
at StackExchange.Redis.ConnectionMultiplexer.SentinelMasterConnectAsync(ConfigurationOptions configuration, TextWriter log) in /
/src/StackExchange.Redis/ConnectionMultiplexer.cs:line 1128

I was able to resolve the error by including the MultiCache.StackExchangeRedis nuget package.

This package should not be included in the OrchardCore.Redis.Abstractions module?

@jtkech
Copy link
Member

jtkech commented Jul 8, 2022

@micrub-it

Yes, to make it working @mazuryv needed to reference an up to date package, see #11617 (comment), and we also needed to update OrchardCore which has been done through a merged PR.

Maybe in our last main branch we now reference the up to date package, @mazuryv can you help @micrub-it on this one, if you have time ;)

@micrub-it
Copy link

@jtkech
Thanks for the reply. For now I have solved it by modifying the source of OrchardCore.Redis.Abstractions. I hope that with the next release the problem is solved.

I also verified that, with redis and distributed cache enabled, if there is a connection problem with redis/sentinel, admin authentication fails. This line fails:
await _distributedCache.SetAsync (token.ToString (), token.ToByteArray (), new DistributedCacheEntryOptions () {AbsoluteExpirationRelativeToNow = expiration});

This is a bit frustrating! As this way you can't disable redis and distributed cache!

@mazuryv
Copy link
Contributor Author

mazuryv commented Jul 12, 2022

Hello.
You should update StackExchange.Redis nuget package to last release. It can be changed in OrchardCore.Redis.Abstractions.

@Skrypt Skrypt modified the milestones: 1.x, 1.5 Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants