Nikey646 / VndbSharp

A C# Vndb API Library. #OriginalNamingScheme
MIT License
18 stars 4 forks source link

User auth #69

Closed micah686 closed 3 years ago

micah686 commented 3 years ago

Replaced UserAuth compiler directives in favor of Environmental variables. This way, the user can set if they want to use the insecure SecureString in their project using the nuget package, instead of trying to compile the project from source.

Right now, it just looks for the environmental variable key ALLOW_UNSECURE_SECURESTRING, but I can also have it look for that, AND what value is inside it, if need be.

.Net Standard was updated from 1.3 to 2.0, so that Trace could be used.

The method AllowInsecure allows for throwing an error if it's false, so any attempts to login without the ENV set will throw an error. However, parts where it was just doing a check if UserAuth was on before now won't generate an error.

This should allow the developers using the nuget package to decide if they want the risks associated with using SecureString, without having it enabled by default. It's now an opt in value that the programmers have to manually create in order to use it.

Nikey646 commented 3 years ago

Not a fan of this solution, it either requires that the developer of the app to modify the environment variables, which from my understanding is typically a big no no, or to have the end user of the app manually set the environment variable which is a pretty bad design.

I personally dislike environment variables in general, because they are effectively magic strings. Refer to the telemetry for .Net Core's dotnet.exe, you either need to see the message inside the console or google it. And same for the default "environment" variable for Generic/Web hosts in ASP.Net / .Net Core.

micah686 commented 3 years ago

After reviewing what you said, I think I've come up with another solution: Example Library functions

public partial class Class1
    {
        private const string AllowInsecureStr = "I_UNDERSTAND_THE_RISKS_OF_USING_SecureString_ON_NON-WINDOWS_OPERATING_SYSTEMS_AND_WILL_NOT_CREATE_ISSUES_OR_PRs_INVOLVING_IT_https://git.io/JU5mt";
        private String SecurityState { get; set; }

        public void Vndb(bool isTrue)
        {
            Console.WriteLine("normal");
        }

        public void Vndb(string username, SecureString password)
        {
            if (IsInsecureAllowed(false) == true)
            {
                Trace.WriteLine("OK VALUE");
            }
            else
            {
                Trace.WriteLine("BAD VALUE");
            }
        }

        private Boolean IsInsecureAllowed(bool throwOnFail)
        {
            if (SecurityState.Equals(AllowInsecureStr))
            {
                Trace.TraceWarning("VndbSharp Unsecure SecureStrings are allowed. Make sure that you are aware of the risks of setting this VARIABLE. https://git.io/JU5mt");
                return true;
            }
            else
            {
                var notice =
                    "SecureString is not secure on non-Windows OSes when using .Net Core, or at all in Mono." +
                    "By setting the 'ALLOW_UNSECURE_SECURESTRING' environment variable, and/or this warning, you acknowledge the risks and will not make PRs or Issues regarding this unless the situation in .Net Core / Mono changes. \n" +
                    "To read more above the above messages, check out https://github.com/Nikey646/VndbSharp/wiki/Mono-and-.Net-Core#securestring--username--password-logins \n" +
                    "If that link is down, do some research on SecureString implementations in .Net Core, to see if they encrypt the data in memory on Unix.";

                if (throwOnFail == true)
                {
                    throw new NotSupportedException(notice);
                }
                else
                {
                    return false;
                }
            }
        }

        public void SetSecureState(string state)
        {
            SecurityState = state;
        }
    }

Example Exe method

class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Hello World!");

            var t1 = new Class1();
            t1.SetSecureState("BAD_VAL");
            t1.Vndb("", new SecureString());
            Console.Read();
        }
    }

It follows most of the same logic as before, but instead of reading from an Environment variables, the user would set a string that has them accepting the risk of using the SecureString. It would check that before allowing access to any of the functions that USE SecureString.

This would also add a TraceWarning message to the output(not console output, but VS output) that insecure strings are allowed, so the developer will always be aware if they are enabled.