Database credentials and connector including encryption

I'm designing a small Java desktop application to interact with my database and this is a very important part of it as a majority of the operations will be involving the SQL Server 2012 database. I am using the SQL Server JDBC driver (v6.0) provided by Microsoft.

I wrote a class to hold credentials used to connect, and another one to provide a connection to be able to interact with the database. I'm using Java 8. Looking for any and all improvements.
ConnectionCredentials

```
import javax.crypto.Cipher;
import javax.crypto.spec.SecretKeySpec;
import java.security.Key;
import java.security.MessageDigest;

/**
* Hold and make available credentials to connect to SQL database.
*/
public class ConnectionCredentials {
private String serverName = "";
private String databaseName = "";
private String username = "";
private Key passwordKey;
private byte[] encryptedPassword;

/**
* Constructor
* @param serverName name/address of the server
* @param databaseName name of the database
* @param username user/login name on the database
* @param password user/login password on the database (plain text)
*/
public ConnectionCredentials(String serverName, String databaseName, String username, String password) {
this.serverName = serverName;
this.databaseName = databaseName;
this.username = username;
encryptedPassword = encrypt(password);
}

/**
* Encrypts password prior to storing in a ConnectionCredentials object
* @param password
* @return the encrypted password
*/
private byte[] encrypt(String password) {
// Encryption code based on
// http://stackoverflow.com/a/32583766/3626537
byte[] encrypted = {};
try {
MessageDigest digester = MessageDigest.getInstance("MD5");
digester.update(String.valueOf(password).getBytes("UTF-8"));
byte[] digest = digester.digest();
passwordKey = new Sec

Encrypts password prior to storing in a ConnectionCredentials object

Why are you encrypting the password in this object? There are very few things attacks that it would actually prevent that I can see here.

• Maybe you're trying to prevent unauthorised classes from accessing the used credentials object?

Well, the credentials object is only ever used inside of DatabaseConnector, as is not exposed; unless your application leaks it to other parts, this should not be an issue. This could be prevented simply by ensuring that you never expose an instance of the credentials object anywhere.

• Maybe you're worried about memory reading attacks?

As far as I understand, in Java, strings are by default interned by the compiler. This means that they are stored on the heap - anyone could just as easily access the password in memory.

Only literals are interned. Thanks @h.j.k

This is ignoring the interesting reflection tricks they could use to access the private field anyway.

Furthermore, encrypting the password before immediately passing it to the database connector and having the db connector decrypt it is pretty poor, architecturally: you're assuming that any password passed to the db connector would be encrypted with a given algorithm. This doesn't make it very reusable at all!

My suggestion? Don't encrypt the password. It is making you feel safe, but it is not doing anything at all. Instead, make sure that the Credentials class only touches as few places as it needs to touch and heavily restrict its access.

There's a rather insidious bug caused by how you've arranged encrypt in your constructor. First off, as a result of thumb, constructors should not throw. Given your types are correct (which the compiler should catch), your constructor should only assign fields.

In your example, you have this;

public ConnectionCredentials(String serverName, String databaseName, String username, String password) {
    this.serverName = serverName;
    this.databaseName = databaseName;
    this.username = username;
    encryptedPassword = encrypt(password);
}

/**
 * Encrypts password prior to storing in a ConnectionCredentials object
 * @param password
 * @return the encrypted password
 */
private byte[] encrypt(String password) {
    // Encryption code based on
    // http://stackoverflow.com/a/32583766/3626537
    byte[] encrypted = {};
    try {
        ...snip...
    } catch(Exception exc) {
        exc.printStackTrace();
    }
    return encrypted;
}

If the snipped block threw an exception (which you try/caught presumably to avoid throwing in the ctor), then a really bad bug would occur where, from the user's perspective, nothing would happen until they tried to connect to a DB object. When they tried to this, they would notice that encrypted was set to an empty byte array, and the connection would fail, and then that would throw.

This bug would take a while to track down. Do not do this. If you must encrypt your password, do it in an object that constructs ConnectionCredentials.

Use catch sparingly. There are seldom occasions where it is a good idea to try/catch as many exceptions will be catastrophic errors (Exceptions should not be used for control flow, after all). You should let the exception propagate up until something can handle it (logging it to the console and leaving the object in an uninitialised state is not handling it). If the object cannot recover, let it turn into a crash.

Note that in Java, if a constructor throws, it will leave the object in a semi initialised state. This may cause further bugs. So, if the constructor throwing is a bad idea and catching the exception is a bad idea, what do you do? You move the action that is causing the throw to another location.

StackExchange Code Review Q#132747, answer score: 5