Database credentials and connector including encryption

Question

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 SecretKeySpec(digest, "AES");
            Cipher cipher = Cipher.getInstance("AES");
            cipher.init(Cipher.ENCRYPT_MODE, passwordKey);
            encrypted = cipher.doFinal(password.getBytes());
        } catch(Exception exc) {
            exc.printStackTrace();
        }
        return encrypted;
    }

    /**
     * Format credentials to a String.
     * @return the credentials formatted as a String
     */
    @Override
    public String toString() {
        return String.format(
                "serverName: %s | databaseName: %s | username: %s | password: HIDDEN",
                getServerName(),
                getDatabaseName(),
                getUsername()
        );
    }

    /*
    * Field getters and setters
    */
    public String getServerName() {
        return serverName;
    }

    public void setServerName(String serverName) {
        this.serverName = serverName;
    }

    public String getDatabaseName() {
        return databaseName;
    }

    public void setDatabaseName(String databaseName) {
        this.databaseName = databaseName;
    }

    public String getUsername() {
        return username;
    }

    public void setUsername(String username) {
        this.username = username;
    }

    public Key getPasswordKey() {
        return passwordKey;
    }

    public byte[] getEncryptedPassword() {
        return encryptedPassword;
    }

    public void setPassword(String password) {
        encryptedPassword = encrypt(password);
    }
}

---

DatabaseConnector

import javax.crypto.Cipher;
import java.sql.Connection;
import java.sql.DriverManager;

/**
 * Connect to SQL Server database using ConnectionCredentials object.
 */
public class DatabaseConnector {
    private String connectionUrl = "";
    private ConnectionCredentials credentials;

    /**
     * Constructor
     * @param credentials ConnectionCredentials object containing the required configuration to connect to the database server instance.
     */
    public DatabaseConnector(ConnectionCredentials credentials) {
        this.credentials = credentials;
    }

    /**
     * Get a JDBC connection using ConnectionCredentials object.
     * @param credentials the ConnectionCredentials object to use to open JDBC connection
     * @return the JDBC connection
     */
    public Connection getJdbcConnection(ConnectionCredentials credentials) {
        Connection jdbcConnection = null;
        try {
            Class.forName("com.microsoft.sqlserver.jdbc.SQLServerDriver");
            jdbcConnection = DriverManager.getConnection(
                    getConnectionUrl(credentials),
                    credentials.getUsername(),
                    decrypt(credentials.getEncryptedPassword())
            );
        } catch(Exception exc) {
            exc.printStackTrace();
        }
        return jdbcConnection;
    }

    /**
     * Builds the required connection URL required by DriverManager.
     * @param credentials ConnectionCredentials object containing the required configuration to connect to the database server instance.
     * @return the connection URL built from the credentials
     */
    private String getConnectionUrl(ConnectionCredentials credentials) {
        String serverName = credentials.getServerName();
        String databaseName = credentials.getDatabaseName();
        String username = credentials.getUsername();

        connectionUrl = String.format(
                "jdbc:sqlserver://%s;databaseName=%s;",
                serverName,
                databaseName
        );
        return connectionUrl;
    }

    /**
     * Decrypts data encrypted by ConnectionCredentials, e.g., a password.
     * @param encrypted the encrypted data
     * @return the decrypted data
     */
    private String decrypt(byte[] encrypted) {
        String decrypted = "";
        StringBuilder stringBuilder = new StringBuilder();
        try {
            Cipher cipher = Cipher.getInstance("AES");
            cipher.init(Cipher.DECRYPT_MODE, credentials.getPasswordKey());
            decrypted = new String(cipher.doFinal(encrypted));
        } catch(Exception exc) {
            exc.printStackTrace();
        }
        return decrypted;
    }
}

---

Example usage

ConnectionCredentials credentials = new ConnectionCredentials(server, database, username, password);
DatabaseConnector connector = new DatabaseConnector(credentials);
Connection connection = connector.getJdbcConnection(credentials);
try {
    Statement statement = connection.createStatement();
    ResultSet resultSet = statement.executeQuery("select Id, Name, Organization from PsychoProductions.Persons;");
    while(resultSet.next()) {
        System.out.printf(
                "Id: %d Name: %s Organization: %s%n",
                resultSet.getInt("Id"),
                resultSet.getString("Name"),
                resultSet.getString("Organization")
        );
    }
} catch(Exception exc) {
    exc.printStackTrace();
}

Answer 1

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.

1. 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.

2. 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.

Answer 2

Small-ish addition to Dan Pantry's review:

Your ConnectionCredentials don't seem to actually ever have to change. The serverName, databaseName, username and password aren't ever expected to change. so why not make them impossible to change after creating ConnectionCredentials?

public class ConnectionCredentials {
    private final String serverName;
    private final String databaseName;
    private final String username;
    private final String password;

    // skipped javadoc
    public ConnectionCredentials(String serverName, String databaseName, String username, String password) {
        this.serverName = serverName;
        this.databaseName = databaseName;
        this.username = username;
        this.password = password;
    }
}

Note that this assumes you take Dan's advice to not encrypt the password. Also note that this implies you remove all the setters of that class. (Not that you need them anyways, right?)