rocklib / rocklib.configuration Goto Github PK

in RockLib.Configuration/RockLibConfigurationBuilderExtensions.cs the value of ASPNETCORE_ENVIRONMENT is converted to lower case to find the file, that works fine in Windows but file names are case sensitive in Linux, so that could cause exceptions

builder.AddJsonFile($"appsettings.{environment .ToLower() }.json", optional: true)

Hi

A justification for this suggestion is to avoid leaking configuration secrets into logs via an exception message.
When getting a configuration object via the ConfigurationObjectFactory.Create extension method. If there is a value conversion error, it emits an InvalidOperationException which contains the problematic value in its message. I'm talking about the one produced by RockLib.Configuration.ObjectFactory.Exceptions.CannotConvertSectionValueToTargetType method.

In case there is some secret in the value (which is common for configuration) it can end up being exposed somewhere in a log/console output along with the exception. So I'd like to have an option to prevent output of configuration values to exception messages. While maintaining other context of an error like the section name. E.g. as an optional parameter to the Create method.

Otherwise, I currently resorted to catching an exception and throwing a generic one without much context. Which makes harder to find the source of an error in configuration.

Overview

When using RockLib.Configuration.ObjectFactory, under certain circumstances, a memory leak can occur. This memory leak appears to only happen when setting up automatic config file reloading via ConfigReloadingProxyFactory. (CreateReloadingProxy specifically)

Details

It's hard to say when this problem was introduced - i've been able to verify that it is at least occurring in 2.x versions as well as the latest 4.x release. Other than that, I have not confirmed, but it's highly likely that this is a problem there as well.

The problem occurs when the type configured to be setup as a reloadable type implements IDisposable (e.g. RockLib.Logging.ILogger).

What happens is that, when you request a reloadable configuration object, under the covers a type is dynamically created using a bunch of fancy hand-waving with IL and reflection to get an actual instance of a new type of object. As a consumer you would request an object of type IMyConfigType, and you would indeed be returned with an instance of that interface.

So using RockLib.Logging as an example, if you would like to create an instance of ILogger that reloads on-demand any time the appsettings.json file is changed, you would call into this method. Prior to calling this method, you would need to setup a default type to be passed in that acts as a mapping between the interface and the actual concrete type you're using to house your config data (i.e. ILogger and Logger). When you call ConfigReloadingProxyFactory.CreateReloadingProxy, what it actually returns is a completely new type that implements ILogger, and all of the implementations of ILogger defined in Logger have been magically copied to that new object.

Using a more simple example of IMyCustomConfigType and MyCustomConfigType, you may be returned an object (SuperSpecialDynamicallyCreatedType) that looks something like this:

public interface IMyCustomConfigType
{
  void MySpecialMethod();
}

public class MyCustomConfigType : IMyCustomConfigType
{
   public void MySpecialMethod()
   {
     Console.WriteLine((1 + 1) == 11);
   }
}

public class SuperSpecialDynamicallyCreatedType<IMyCustomConfigType> : ConfigReloadingProxy, IMyCustomConfigType
{
   public void MySpecialMethod()
   {
     Console.WriteLine((1 + 1) == 11);
   }
}

This is cool, but there are actually three problems.

1. The SuperSpeciallyDynamicallyCreatedType is actually based on an existing type - ConfigReloadingProxy. And ConfigReloadingProxy actually creates and (currently) does not keep track of, nor dispose of an IDisposable object. In the constructor for this class, there is the line:

ChangeToken.OnChange(section.GetReloadToken, () => ReloadObject(false));

2. Let's same problem 1 is fixed, you still have the other problem that there is no requirement that the type being created is even disposable. So even if you did update the Dispose method on ConfigReloadingProxy to dispose of the change token disposable, the Dispose method may not be called when you want it to - or may not even be called at all.
3. Even if you enforced that any type you're creating for this dynamic reloading proxy were to be IDisposable, there's an even stranger thing. When the super special dynamic type is created, the Dispose method is copied from the implementation type.. which leads to a problem where the base class Dispose method is hidden, and it never gets called.

Fortunately, it appears that the fix for this is pretty easy:

1. Update the ConfigReloadingProxy to keep track of and dispose of the IDisposable returned from ChangeToken.OnChange
2. Enforce that the interface types passed into the CreateReloadingProxy method MUST implement IDisposable
3. Do not copy the Dispose method from the implementation type to the dynamic type - let the base class CreateReloadingProxy.Dispose() take care of it.

Is your feature request related to a problem? Please describe.

Currently the Config object is static, which makes it very difficult to unit test around. Other RockLib libraries adhere to the static nature of this class. As a result, we have no control over what is set in the configuration unless we use a standard .NET app configuration or environment variables. For unit tests, we have to avoid directly using code that relies on this static class because its impossible to mock configuration on a per test basis.

Our application doesn't rely on this library directly, as we have our own way of providing configuration that is decoupled completely from the provider; meaning, our tests/prod code are able to supply whatever configuration is necessary rather than relying on a static global configuration. However, we do rely on other libraries that rely on RockLib.Configuration and its very difficult to unit test with them because we are unable to mock how those components are configured. We often get warnings letting us know that certain required variables/config is missing even though we don't need it at all.

Describe the solution you'd like

We'd like to have support for proper dependency injection instead of static classes. Microsoft has already provided tooling for setting up and creating IConfiguration and providers in a way that allows consumers to provide and inject it the way they want, but this seems to create its own IConfiguration and AppSettings combination which limits how consumers are able to provide configuration. Because DI is not the standard in this library and static classes are used instead, no consumer is able to use DI. This is a problem if you dont have an appsettings.json file or environment variables - neither of which are unit testable. If they are unit testable, its a roundabout way that requires a lot more overhead than injecting a single IConfiguration as needed.

Describe alternatives you've considered

If we were a consumer that needed to use this configuration directly but wanted a layer of separation, we could create our own anti-corruption layer, but this is not a viable solution for all consumers as that will create an explosion of duplicated boilerplate that may not be compatible across all consumers. For example, if I used an authentication library and a logging library (assuming both are maintained by different parties who created anti-corruption layers to better leverage DI), I might need to work with two different yet nearly identical layers of abstraction in order to setup the configuration for both when writing unit tests.

There is a vulnerability identified by snyk: https://app.snyk.io/vuln/SNYK-DOTNET-SYSTEMTEXTENCODINGSWEB-1253267 that can be addressed with a simple version update.