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();
 }
}

• Simon Forsberg: You are probably interested in "try-with-resources" that was introduced in Java 7.
• AFP_555: Could you post the rendered code just to check the answer? I'm not sure how are the dynamics in CodeReview, so I don't know if asking for this is ok. Maybe just a piece of the code is what is usually given as answers?
• AFP_555: Also, what would you do in case a mathod depends on two resources, not just one? Let's say I have two files and I want to compare line by line if both files have the same text. I'd need to Streams to read the file.

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.

• AFP_555: Very unusual way to use the for loop. I love it.

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)

• AFP_555: Amazing, thanks. But I have a question. I found that very big files have to be sent using a buffer. This way the whole file isn't sent all at once. This nice approach you showed will allow the use of a buffer?
• mtj: Yes, under the hood it uses a buffer of 8 kb.
• AFP_555: And would this 8 kb buffer allow for AES 128 encryption?
• mtj: Provided that the cypher output stream works as designed, I don't see why not.