views:

222

answers:

3

I have a program that uses System.DirectoryServices.AccountManagement.PrincipalContext to verify that the information a user entered in a setup screen is a valid user on the domain (the computer itself is not on the domain) and do some operations on the users of the domain. The issue is I do not want the user to need to enter his or her password every time they run the program so I want to save it, but I do not feel comfortable storing the password as plain-text in their app.config file. PrincipalContext needs a plain-text password so I can not do a salted hash as everyone recommends for password storing.

This is what I did

const byte[] mySalt = //It's a secret to everybody.
[global::System.Configuration.UserScopedSettingAttribute()]
public global::System.Net.NetworkCredential ServerLogin
{
    get
    {
        var tmp = ((global::System.Net.NetworkCredential)(this["ServerLogin"]));
        if(tmp != null)
            tmp.Password = new System.Text.ASCIIEncoding().GetString(ProtectedData.Unprotect(Convert.FromBase64String(tmp.Password), mySalt, DataProtectionScope.CurrentUser));
        return tmp;
    }
    set
    {
        var tmp = value;
        tmp.Password = Convert.ToBase64String(ProtectedData.Protect(new System.Text.ASCIIEncoding().GetBytes(tmp.Password), mySalt, DataProtectionScope.CurrentUser));
        this["ServerLogin"] = value;
    }
}

Was this the right thing to do or is there a better way?

EDIT -- Here is a updated version based on everyone's suggestions

private MD5 md5 = MD5.Create();

[global::System.Configuration.UserScopedSettingAttribute()]
public global::System.Net.NetworkCredential ServerLogin
{
    get
    {
        var tmp = ((global::System.Net.NetworkCredential)(this["ServerLogin"]));
        if(tmp != null)
            tmp.Password = System.Text.Encoding.UTF8.GetString(ProtectedData.Unprotect(Convert.FromBase64String(tmp.Password), md5.ComputeHash(System.Text.Encoding.UTF8.GetBytes(tmp.UserName.ToUpper())), DataProtectionScope.CurrentUser));
        return tmp;
    }
    set
    {
        var tmp = value;
        tmp.Password = Convert.ToBase64String(ProtectedData.Protect(System.Text.Encoding.UTF8.GetBytes(tmp.Password), md5.ComputeHash(System.Text.Encoding.UTF8.GetBytes(tmp.UserName.ToUpper())), DataProtectionScope.CurrentUser));
        this["ServerLogin"] = tmp;
    }
}
+2  A: 

Instead of writing new System.Text.ASCIIEncoding(), you should write System.Text.Encoding.ASCII.

Also, I recommend using UTF8 instead.

Other than that, your code looks pretty good.

SLaks
Thanks for the tip about the other way to use the encoder. Why do you recommend UTF8 over ASCII?
Scott Chamberlain
So that passwords can contain Unicode characters.
SLaks
Thanks, I will change it.
Scott Chamberlain
Joel's idea of using per-username salts is an excellent suggestion and I highly recommend it.
SLaks
+4  A: 

For the salt, I'd do a transformation on the username (hash it) rather than share the same salt for everyone.

For something like this, I'd also look for a way to keep the existing session alive longer rather than saving the password to create new sessions.

Joel Coehoorn
The session is alive the entire instance the program is run, I just need to create the session with the server when the program starts, or did you mean keep the session alive between runs?
Scott Chamberlain
You could use a service or other background program to hold the session and keep it alive longer.
Joel Coehoorn
A: 

I like the JoelCoehoorn approach.

Use a value unique for the user machine as the password salt.

So it will be different in each deplyment ; ).

UPDATE: See this thread for ideas: How-To-Get-Unique-Machine-Signature

SDReyes
What is the same user logs on to different machines? It's got to be per-user.
Joel Coehoorn
Hi Joel thanks for asking : )If the user change from pc to pc: he will need to enter its domain credentials again necessarily, before caching ; )
SDReyes
`ProtectedData.Protect` is already per-machine.
SLaks
It can be either user or machine. that is what the last parameter DataProtectionScope is for.
Scott Chamberlain
Yes, but since the machine isn't in a domain, even per-user will also be per-machine.
SLaks