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

[API Proposal]: Customizable IConfigurationRoot.GetDebugView() for hiding the value #60065

Closed
oskrabanek opened this issue Oct 6, 2021 · 9 comments
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Configuration help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@oskrabanek
Copy link
Contributor

Background and motivation

As the GetDebugView() method will display the whole configuration, we want to have it in logs for debug purposes. This brings a security risk as some secrets such as connection string, Identity credentials and others might be included in the configuration.

API Proposal

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace Microsoft.Extensions.Configuration
{
    /// <summary>
    /// Extension methods for <see cref="IConfigurationRoot"/>.
    /// </summary>
    public static class ConfigurationRootExtensions
    {
        /// <summary>
        /// Generates a human-readable view of the configuration showing where each value came from.
        /// </summary>
        /// <returns> The debug view. </returns>
        public static string GetDebugView(this IConfigurationRoot root)
        {
            return GetDebugView(root, null);
        }

        /// <summary>
        /// Generates a human-readable view of the configuration showing where each value came from.
        /// </summary>
        /// <param name="root">Configuration root</param>
        /// <param name="processValue">
        /// Function for processing the value e.g. hiding secrets
        /// Parameters:
        ///   Key: Key of the current configuration section
        ///   Path: Full path to the configuration section
        ///   Value: Value of the configuration section
        ///   ConfigurationProvider: Provider of the value of the configuration section
        ///   returns: Value is used to assign as the Value of the configuration section
        /// </param>
        /// <returns> The debug view. </returns>
        public static string GetDebugView(this IConfigurationRoot root, Func<string, string, string, IConfigurationProvider, string> processValue)
        {
            void RecurseChildren(
                StringBuilder stringBuilder,
                IEnumerable<IConfigurationSection> children,
                string indent)
            {
                foreach (IConfigurationSection child in children)
                {
                    (string Value, IConfigurationProvider Provider) valueAndProvider = GetValueAndProvider(root, child.Path);

                    if (valueAndProvider.Provider != null)
                    {
                        var value = processValue != null
                            ? processValue(child.Key, child.Path, valueAndProvider.Value, valueAndProvider.Provider)
                            : valueAndProvider.Value;

                        stringBuilder
                            .Append(indent)
                            .Append(child.Key)
                            .Append('=')
                            .Append(value)
                            .Append(" (")
                            .Append(valueAndProvider.Provider)
                            .AppendLine(")");
                    }
                    else
                    {
                        stringBuilder
                            .Append(indent)
                            .Append(child.Key)
                            .AppendLine(":");
                    }

                    RecurseChildren(stringBuilder, child.GetChildren(), indent + "  ");
                }
            }

            var builder = new StringBuilder();

            RecurseChildren(builder, root.GetChildren(), "");

            return builder.ToString();
        }

        private static (string Value, IConfigurationProvider Provider) GetValueAndProvider(
            IConfigurationRoot root,
            string key)
        {
            foreach (IConfigurationProvider provider in root.Providers.Reverse())
            {
                if (provider.TryGet(key, out string value))
                {
                    return (value, provider);
                }
            }

            return (null, null);
        }
    }
}

API Usage

public Startup(IWebHostEnvironment env, IConfiguration configuration)
{
    ...
  
    if (configuration is IConfigurationRoot configurationRoot)
    {
        var startupLoggerFactory = StartupLoggerHelper.CreateStartupLoggerFactory(configuration, env);
        var startupLogger = startupLoggerFactory.CreateLogger<Startup>();
        startupLogger.LogDebug(configurationRoot.GetDebugView(HideSecrets));
    }
}

private string HideSecrets(string key, string path, string value, IConfigurationProvider provider)
{
    var providerName = provider.ToString();
    if (providerName.Contains("KeyVault"))
    {
        return "*** secret ***";
    }

    return value;
}

Risks

None as far as I know.

@oskrabanek oskrabanek added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 6, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Configuration untriaged New issue has not been triaged by the area owner labels Oct 6, 2021
@ghost
Copy link

ghost commented Oct 6, 2021

Tagging subscribers to this area: @maryamariyan, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

As the GetDebugView() method will display the whole configuration, we want to have it in logs for debug purposes. This brings a security risk as some secrets such as connection string, Identity credentials and others might be included in the configuration.

API Proposal

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace Microsoft.Extensions.Configuration
{
    /// <summary>
    /// Extension methods for <see cref="IConfigurationRoot"/>.
    /// </summary>
    public static class ConfigurationRootExtensions
    {
        /// <summary>
        /// Generates a human-readable view of the configuration showing where each value came from.
        /// </summary>
        /// <returns> The debug view. </returns>
        public static string GetDebugView(this IConfigurationRoot root)
        {
            return GetDebugView(root, null);
        }

        /// <summary>
        /// Generates a human-readable view of the configuration showing where each value came from.
        /// </summary>
        /// <param name="root">Configuration root</param>
        /// <param name="processValue">
        /// Function for processing the value e.g. hiding secrets
        /// Parameters:
        ///   Key: Key of the current configuration section
        ///   Path: Full path to the configuration section
        ///   Value: Value of the configuration section
        ///   ConfigurationProvider: Provider of the value of the configuration section
        ///   returns: Value is used to assign as the Value of the configuration section
        /// </param>
        /// <returns> The debug view. </returns>
        public static string GetDebugView(this IConfigurationRoot root, Func<string, string, string, IConfigurationProvider, string> processValue)
        {
            void RecurseChildren(
                StringBuilder stringBuilder,
                IEnumerable<IConfigurationSection> children,
                string indent)
            {
                foreach (IConfigurationSection child in children)
                {
                    (string Value, IConfigurationProvider Provider) valueAndProvider = GetValueAndProvider(root, child.Path);

                    if (valueAndProvider.Provider != null)
                    {
                        var value = processValue != null
                            ? processValue(child.Key, child.Path, valueAndProvider.Value, valueAndProvider.Provider)
                            : valueAndProvider.Value;

                        stringBuilder
                            .Append(indent)
                            .Append(child.Key)
                            .Append('=')
                            .Append(value)
                            .Append(" (")
                            .Append(valueAndProvider.Provider)
                            .AppendLine(")");
                    }
                    else
                    {
                        stringBuilder
                            .Append(indent)
                            .Append(child.Key)
                            .AppendLine(":");
                    }

                    RecurseChildren(stringBuilder, child.GetChildren(), indent + "  ");
                }
            }

            var builder = new StringBuilder();

            RecurseChildren(builder, root.GetChildren(), "");

            return builder.ToString();
        }

        private static (string Value, IConfigurationProvider Provider) GetValueAndProvider(
            IConfigurationRoot root,
            string key)
        {
            foreach (IConfigurationProvider provider in root.Providers.Reverse())
            {
                if (provider.TryGet(key, out string value))
                {
                    return (value, provider);
                }
            }

            return (null, null);
        }
    }
}

API Usage

public Startup(IWebHostEnvironment env, IConfiguration configuration)
{
    ...
  
    if (configuration is IConfigurationRoot configurationRoot)
    {
        var startupLoggerFactory = StartupLoggerHelper.CreateStartupLoggerFactory(configuration, env);
        var startupLogger = startupLoggerFactory.CreateLogger<Startup>();
        startupLogger.LogDebug(configurationRoot.GetDebugView(HideSecrets));
    }
}

private string HideSecrets(string key, string path, string value, IConfigurationProvider provider)
{
    var providerName = provider.ToString();
    if (providerName.Contains("KeyVault"))
    {
        return "*** secret ***";
    }

    return value;
}

Risks

None as far as I know.

Author: Andree643
Assignees: -
Labels:

api-suggestion, untriaged, area-Extensions-Configuration

Milestone: -

@ghost ghost added this to Untriaged in ML, Extensions, Globalization, etc, POD. Oct 6, 2021
@maryamariyan maryamariyan added this to the 7.0.0 milestone Oct 6, 2021
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Oct 6, 2021
@ghost ghost moved this from Untriaged to 7.0.0 in ML, Extensions, Globalization, etc, POD. Oct 6, 2021
@safern
Copy link
Member

safern commented Oct 6, 2021

@Andree643 seems reasonable to me. I do have a question, do you see a scenario where someone might care about the key and the path when processing the value, or is the Provider enough to know if it is a secret value or not?

@oskrabanek
Copy link
Contributor Author

oskrabanek commented Oct 6, 2021

@Andree643 seems reasonable to me. I do have a question, do you see a scenario where someone might care about the key and the path when processing the value, or is the Provider enough to know if it is a secret value or not?

@safern I actually do. At least the path is really useful when you want to hide only a single specific value by exact path or to check the path for existence of some substring such as "ConnectionString". At least I assume it would be helpful to give the developers access to these properties since we have them and might be useful for them.

The key might not be needed as it's already as a suffix of the path, but it can be more efficient to check only the key when needed rather then parsing the path.

@safern
Copy link
Member

safern commented Oct 6, 2021

Sounds good. I'll mark it as ready for review as it looks good as proposed, we can discuss if we want the key or not during the review of the API.

@safern safern added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 6, 2021
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Oct 12, 2021
@terrajobst
Copy link
Member

terrajobst commented Oct 12, 2021

Video

  • The scenario makes sense but we don't like the use of Func because it's not clear what the first three strings
  • Should Value be nullable?
namespace Microsoft.Extensions.Configuration
{
    public readonly struct ConfigurationDebugViewContext
    {
        public ConfigurationDebugViewContext(string path, string key, string value, IConfigurationProvider configurationProvider);
        public string Path { get; }
        public string Key { get; }
        public string Value { get; }
        public IConfigurationProvider ConfigurationProvider { get; }
    }

    public static class ConfigurationRootExtensions
    {
        // Existing
        // public static string GetDebugView(this IConfigurationRoot root);
        public static string GetDebugView(this IConfigurationRoot root, Func<ConfigurationDebugViewContext, string> processValue);
    }
}

@safern safern added the help wanted [up-for-grabs] Good issue for external contributors label Oct 12, 2021
@oskrabanek
Copy link
Contributor Author

Video

  • The scenario makes sense but we don't like the use of Func because it's not clear what the first three strings
  • Should Value be nullable?
namespace Microsoft.Extensions.Configuration
{
    public readonly struct ConfigurationDebugViewContext
    {
        public ConfigurationDebugViewContext(string path, string key, string value, IConfigurationProvider configurationProvider);
        public string Path { get; }
        public string Key { get; }
        public string Value { get; }
        public IConfigurationProvider ConfigurationProvider { get; }
    }

    public static class ConfigurationRootExtensions
    {
        // Existing
        // public static string GetDebugView(this IConfigurationRoot root);
        public static string GetDebugView(this IConfigurationRoot root, Func<ConfigurationDebugViewContext, string> processValue);
    }
}
  • This surely is more understandable rather than the Func parameter.
  • From my experience the Value is not null if the key exists. As the IConfigurationProvider has method bool TryGet(string key, out string value); which return non-nullable string I guess the nullable is not needed.
...
    public interface IConfigurationProvider
    {
        /// <summary>
        /// Tries to get a configuration value for the specified key.
        /// </summary>
        /// <param name="key">The key.</param>
        /// <param name="value">The value.</param>
        /// <returns><c>True</c> if a value for the specified key was found, otherwise <c>false</c>.</returns>
        bool TryGet(string key, out string value);
...

@oskrabanek
Copy link
Contributor Author

oskrabanek commented Oct 27, 2021

@safern Could you please help with the build agent? I'm getting following error:

.dotnet/sdk/6.0.100-rc.1.21430.12/Microsoft.Common.CurrentVersion.targets(4823,5): error MSB3026: (NETCORE_ENGINEERING_TELEMETRY=Build) Could not copy "/__w/2/s/artifacts/bin/microsoft.netcore.app.runtime.linux-x64/Release/runtimes/linux-x64/lib/net7.0/Microsoft.VisualBasic.dll" to "bin/Release/net7.0/linux-x64/Microsoft.VisualBasic.dll". Beginning retry 1 in 1000ms. No space left on device 

@SteveDunn
Copy link
Contributor

SteveDunn commented Jun 5, 2022

Is this closed by #60391?

@eerhardt
Copy link
Member

eerhardt commented Jun 7, 2022

Yes, it looks like we missed this. Thanks for the heads up.

Fixed by #60391

@eerhardt eerhardt closed this as completed Jun 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Configuration help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

6 participants