0

I am using ASP.NET Core and using DI to build Hashing functionality. Since I don't know the type of hashing used at this stage (we are storing it in persistent storage).

The ICrypto Contract

public interface ICrypto
{
    string HashPassword(string plainPassword);
    bool VerifyHashedPassword(string hashedPassword, string providedPassword);     
}

I have several Implementations of ICrypto and they just wrap other libraries and provide the implementation of ICrypto Signatures. For example:

  • CryptoMD5
  • CryptoSHA1
  • CytpoOther

Now, in UserService I inject the ICyrpto to hash passwords for example:

Public class UserService
{
     ICrypto _crypto;

     public UserService(ICrypto crypto)
     {
         _crypto = crypto;
     }

    public bool Login (string username, string password)
    {   
        //code omitted
         var hash = _crypto.HashPassword(password);
    }
}

Adding Dependencies to the Container in Startup class

//get encryption type stored in cache, db or somewhere
   var cryptoType = //get param
   if (cryptoType  = "SHA1")
   {
       services.AddTransient<ICrypto, CryptoSHA1>();
   }
   else if (cryptoType  = "MD5")
   {
       services.AddTransient<ICrypto, CryptoMD5>();
   }

I am looking for a way to do this according to best practices and will reflect what Steves has mentioned.

Community
  • 1
  • 1
Hussein Salman
  • 7,806
  • 15
  • 60
  • 98
  • 1
    Factories do not violate DI and you did misread Stevens answer. What you were using is a static factory, this violates the decoupled nature of IoC/DI. If you inject your factory its perfectly fine – Tseng Nov 10 '16 at 15:37
  • @h.salman, in my point of view, you're doing this wrong. You're injecting ICrypto in your service. This means you want to inject a object that can crypt/decrypt your data regardless of the algorithm. Otherwise, you would inject directly an ICryptoSHA1 or ICryptoMD5. So, or you inject a ICrypto that can crypt/decrypt many algorithms or you define the type you need, then inject the right one. – Fabricio Koch Nov 10 '16 at 16:26
  • @FabricioKoch, Please illustrate this by answering the question and indicating how you can implement this. – Hussein Salman Nov 10 '16 at 16:30
  • @Tseng not exactly. As I explain [here](https://www.cuttingedge.it/blogs/steven/pivot/entry.php?id=100) a factory abstraction often violates the Dependency Inversion Principle which is the driving force behind Dependency Injection. – Steven Nov 10 '16 at 17:49
  • I think that @FabricioKoch makes an interesting point, which is that you might be violating the Liskov Substitution Principle, which happens when a consumer breaks when you inject a different ICrypto implementation. I think this statement is true if you try to take a stored hash computed with algorithm A and match it with a fresh hash computed with B. Giving each algorithm each own interface however doesn't solve your design challenge, since you need to be able to 'upgrade'. – Steven Nov 10 '16 at 17:55
  • @Steven 's answer is perfect if you know the cryptoType for the whole app. For example, you have a config in your DB telling you to use "SHA1" for this APP. So, you get this config from your DB and use Steven's solution to inject the right ICrypto. But if you have many users in your DB each one using a different crypt for the password. In this case, you only know what ICrypto you should use in the moment you get the user from DB. In this case you have to use a ICrypto that can use all algorithms your app allows. – Fabricio Koch Nov 10 '16 at 18:13
  • @FabricioKoch, Lets say I have many users in DB with each using different crypt. What is your approach to sove that? Isn't the Proxy proposed by Steve which Implements ICrypto using all algorithms and hashing the string accordingly. – Hussein Salman Nov 10 '16 at 18:37
  • 1
    I would create a ICrypto with the methods `Crypt(string text, string algorithm)` and `Decrypt(string text, string algorithm)`. – Fabricio Koch Nov 10 '16 at 19:05
  • @FabricioKoch In this way we will have one implementation of the interface and all hashing logic is handled inside according to algorithm passed. But would that Comply with SOLID principles? – Hussein Salman Nov 10 '16 at 19:20

1 Answers1

1

In case the cryptoType value you get from the database is constant during the lifetime of the application (which means, if you want to change it, you're fine with restarting the application), this means that the cryptoType is a configuration value and you can simply wire your application as you described:

var cryptoType = //get param
if (cryptoType = "SHA1")
{
    services.AddTransient<ICrypto, CryptoSHA1>();
}
else if (cryptoType = "MD5")
{
    services.AddTransient<ICrypto, CryptoMD5>();
}

If however you need to swap implementations dynamically (which I find very unlikely in your specific case, but let's assume for the sake of argument), the solution is to implement a proxy and wrap the real implementations. Example:

public interface DatabaseCryptoSelectorProxy : ICrypto
{
    private readonly CryptoSHA1 sha;
    private readonly CryptoMD5 md5;

    public DatabaseCryptoSelectorProxy(CryptoSHA1 sha, CryptoMD5 md5) {
        this.sha = sha;
        this.md5 = md5;
    }

    public string HashPassword(string plainPassword) =>
        GetCrypto().HashPasswords(plainPassword);

    public bool VerifyHashedPassword(string hashedPassword, string providedPassword) =>
        GetCrypto().VerifyHashedPassword(hashedPassword, providedPassword);

    private ICrypto GetCrypto() {
        var cryptoType = // get param
        if (cryptoType = "SHA1") return this.sha;
        if (cryptoType = "MD5") return this.md5;
        throw new InvalidOperationException("Unknown cryptotype: " + cryptotype);
    }
}

This proxy has a few clear advantages:

  • It makes the consumers of ICrypto oblivious to the fact that some complex dispatching is happening based on some data from a database.
  • It prevents having to execute this query during object graph construction, since this would make this process unreliable and hard to verify.

Some notes though about your design around password hashing from a security perspective. I don't see any strong reason to switch from crypto methods the way you are doing, and especially not the switch to algorithms like MD5. Instead, I advise using PBKDF2 in the form of Rfc2898DeriveBytes. An example of how to do this can be shown here. By concatenating the number of hash iterations to the hashed password (for instance by simply doing + "|" + iterations), you can later on increase the number of used iterations by keeping up with the industry standard and it allows you to automatically rehash the user's password on login if his number if you detect the number of used iterations an old value.

Additionally, if you think that you ever need to move away from PBKDF2, you can prefix the hash with the used algorithm, this way you can again use a proxy that passes a hashed password on to the right implementation based on the algorithm-prefix. By storing the algorithm in the password hash in the database, you can migrate transparently without having to convert all existing passwords at once (which is impossible, because you can't decrypt a hashed password).

Steven
  • 166,672
  • 24
  • 332
  • 435
  • Thanks @Steven you are amazing. In your approach of using the proxy, there is no need to pass the dependecies for the container, since it will be already doing that during runtime, and this is its advantage. Correct me if I am wrong. – Hussein Salman Nov 10 '16 at 17:25
  • @h.salman you can make the proxy 'self sustained' by letting it create its dependencies. This will probably work well in your crypto use case where the real implementations will probably have no dependencies on their own and are thread-safe. More than often, these conditions don't hold and it would be better to make the container in control of the object graph. This allows the graph to be verified and diagnosed and makes it easier to replace, decorate or intercept components. – Steven Nov 10 '16 at 17:45