Sending a file over a socket with AES encryption

Question

I'm not sure what's the best way to deal with streams that need to be closed whenever I'm done using them? Usually the need to also catch the exception inside the finally statement is very, very annoying. Am I doing this right?

public class FileSender {

    public static final String KEY = "00000000000000000000000000000000";
    public static final String ALGORITHM = "AES";
    public static final String FILE = "C:\\Users\\Andres\\Desktop\\file.txt";
    public static final String HASH_ALG = "MD5";

    private String filePath;
    private String serverMachine;
    private int serverPort;

    public FileSender(String filePath, String serverMachine, int serverPort){
        this.filePath = filePath;
        this.serverMachine = serverMachine;
        this.serverPort = serverPort;
    }

    public void sendFile(){
        CipherOutputStream cipherOut = null;
        Socket client = null;
        String checkSum;
        try {
            System.out.println("Running client ...");
            client = connectToServer();
            checkSum = getHashChecksum();
            cipherOut = initCipherStream(client);
            transferData(cipherOut, checkSum);
        } catch (Exception e) {
            System.out.println(e.getMessage());
        } finally {

        }
    }

    private String getHashChecksum() {
        FileInputStream fis = null;
        try {
            MessageDigest md = MessageDigest.getInstance(HASH_ALG);
            fis = new FileInputStream(this.filePath);
            byte[] byteArray = new byte[1024];
            int bytesCount = 0;
            while ((bytesCount = fis.read(byteArray)) >= 0)
                md.update(byteArray, 0, bytesCount);
            byte[] hash = md.digest();
            fis.close();
            return String.format("%032x",new BigInteger(1, hash));
        } catch (Exception e) {
            e.printStackTrace();
            throw new RuntimeException("Error in checksum");
        } finally {
            try {
                fis.close();
            } catch (IOException e) {
                throw new RuntimeException("Error in checksum. Provided file can't be closed.");
            }
        }
    }

    private Socket connectToServer() throws Exception{
        try {
            Socket client = new Socket(this.serverMachine, this.serverPort);
            return client;
        } catch (IOException e) {
            throw new Exception("Server can't be reached");
        }
    }

    private CipherOutputStream initCipherStream(Socket client) {
        SecretKeySpec keySpec = new SecretKeySpec(hex2byte(KEY), ALGORITHM);
        Cipher cipher;
        try {
            cipher = Cipher.getInstance(ALGORITHM + "/ECB/PKCS5PADDING");
            cipher.init(Cipher.ENCRYPT_MODE, keySpec);
            return new CipherOutputStream(client.getOutputStream(), cipher);
        } catch (Exception  e) {
            e.printStackTrace();
            throw new RuntimeException("Error in cipher stream initialization.");
        }

    }

    private void transferData(CipherOutputStream cipherOut, String checksum) {
        FileInputStream fileReader = null;
        try {
            //Missing here: send MD5 checksum. TODO
            fileReader = new FileInputStream(this.filePath);
            byte[] fileBuffer = new byte[1024];
            int len;
            while ((len = fileReader.read(fileBuffer)) != -1)
                cipherOut.write(fileBuffer, 0, len);
        } catch (Exception e) {
            throw new RuntimeException("File " + this.filePath + " not found");
        } finally{
            try {
                if(fileReader != null) fileReader.close();
            } catch (IOException e) {
                throw new RuntimeException("Socket stream can't be closed");
            }
        }
    }

    private byte[] hex2byte(String s) {
        return DatatypeConverter.parseHexBinary(s);
    }

    public static void main(String [] args) throws Exception {
        FileSender fileSender = new FileSender(FILE, "localhost", 8081);
        fileSender.sendFile();
    }
}

Answer 1

try-with-resources

        FileInputStream fis = null;
        try {
            MessageDigest md = MessageDigest.getInstance(HASH_ALG);
            fis = new FileInputStream(this.filePath);
            byte[] byteArray = new byte[1024];
            int bytesCount = 0;
            while ((bytesCount = fis.read(byteArray)) >= 0)
                md.update(byteArray, 0, bytesCount);
            byte[] hash = md.digest();
            fis.close();
            return String.format("%032x",new BigInteger(1, hash));
        } catch (Exception e) {
            e.printStackTrace();
            throw new RuntimeException("Error in checksum");
        } finally {
            try {
                fis.close();
            } catch (IOException e) {
                throw new RuntimeException("Error in checksum. Provided file can't be closed.");
            }
        }

In a normal run, you try to close the stream twice. You do not need to call close() before the return. The finally block will take care of it.

There is also an issue. If there is an exception in the getInstance call, you will attempt to dereference a null pointer in the finally block.

You can rewrite this

        try (FileInputStream fis = new FileInputStream(this.filePath)) {
            MessageDigest md = MessageDigest.getInstance(HASH_ALG);

            byte[] bytes = new byte[1024];
            for (int bytesCount = fis.read(bytes); bytesCount >= 0; bytesCount = fis.read(bytes)) {
                md.update(byteArray, 0, bytesCount);
            }

            return String.format("%032x", new BigInteger(1, md.digest()));
        } catch (Exception e) {
            e.printStackTrace();
            throw new RuntimeException("Error in checksum");
        }

This uses a try-with-resources so that you don't have to manually close the resource.

I prefer names like bytes to byteArray as being simpler and more focused on what the data means logically than how it is stored physically. You can read more about this in discussions of Hungarian notation. This is somewhat controversial. I mostly mention it because the code was scrolling when it was named the Hungarian notation way.

Using the for loop to replace the while reduces the scope of the bytesCount variable. Other than that, both do the the same thing.

I added some vertical whitespace to break things up.

It's not necessary to assign the result of md.digest() to a variable. We can pass it directly to the BigInteger constructor. Either way works though.

Answer 2

mdfst13 already gave some useful comments regarding the message digest, so I'd like to add something regarding transferData:

When you copy from a file path to a stream, there's no need to write this manually anymore. Files to the rescue:

Files.copy(this.filePath, cipherOut);

Furthermore, check whether you can alter your transfer logic, so that you utilize the streams decorator pattern to the fullest. I imagine, that you could actually wrap the output stream into a digest ouput stream first and in a cyper output stream second, so that you need only a single read-file-and-copy-to-stream operation. (See https://docs.oracle.com/javase/8/docs/api/java/security/DigestOutputStream.html)