Critique of FilterInputStream and FilterOutputStream classes I'm working my way through The Java Programming Language, Fourth Edition - The Java Series.  This is Exercise Exercise 20.3:


  Create a pair of Filter stream classes that encrypt bytes using any 
  algorithm you choose—such as XORing the bytes with some value—with your 
  DecryptInputStream able to decrypt the bytes that your EncryptOutputStream
  class creates.


Is the following an adequate solution? I understand that the key should be randomly generated and at least as long as the message. They key used is just for illustration.

EncryptOutputStream.java

import java.io.FilterOutputStream;
import java.io.IOException;
import java.io.OutputStream;

public class EncryptOutputStream extends FilterOutputStream {
    private final static int XOR_KEY = 1;

    public EncryptOutputStream(OutputStream out) {
        super(out);
    }

    @Override
    public void write(int b) throws IOException {
        super.write(b ^ XOR_KEY);
    }
}


DecryptInputStream.java

import java.io.FilterInputStream;
import java.io.IOException;
import java.io.InputStream;

public class DecryptInputStream extends FilterInputStream {
    private final static int XOR_KEY = 1;

    protected DecryptInputStream(InputStream in) {
        super(in);
    }

    @Override
    public int read() throws IOException {
        int b = super.read();
        return (b != -1 ? b ^ XOR_KEY : b);
    }
}


EncryptionTest.java

import org.junit.Test;
import static org.junit.Assert.*;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.FilterInputStream;
import java.io.FilterOutputStream;
import java.io.InputStream;
import java.io.OutputStream;

public class EncryptionTest {
    @Test
    public void testEncryptOutputStream() throws Exception {
        String testStr = "The quick brown fox jumps over the lazy dog.";
        InputStream in = new ByteArrayInputStream(testStr.getBytes("UTF-8"));
        OutputStream out = new ByteArrayOutputStream();
        FilterOutputStream encryptOut = new EncryptOutputStream(out);

        int b;
        while ((b = in.read()) != -1) {
            encryptOut.write(b);
        }

        assertEquals("Uid!pthbj!csnvo!gny!ktlqr!nwds!uid!m`{x!enf/", out.toString());

        in.close();
        encryptOut.close();
    }

    @Test
    public void testDecryptInputStream() throws Exception {
        String testStr = "Uid!pthbj!csnvo!gny!ktlqr!nwds!uid!m`{x!enf/";
        InputStream in = new ByteArrayInputStream(testStr.getBytes("UTF-8"));
        FilterInputStream decryptIn = new DecryptInputStream(in);
        OutputStream out = new ByteArrayOutputStream();

        int b;
        while ((b = decryptIn.read()) != -1) {
            out.write(b);
        }

        assertEquals("The quick brown fox jumps over the lazy dog.", out.toString());

        decryptIn.close();
        out.close();
    }

    @Test
    public void testEncryptDecrypt() throws Exception {
        String testStr = "The quick brown fox jumps over the lazy dog.";
        OutputStream out = new ByteArrayOutputStream();
        InputStream in = new ByteArrayInputStream(testStr.getBytes("UTF-8"));
        FilterOutputStream encryptOut = new EncryptOutputStream(out);

        int b;
        while ((b = in.read()) != -1) {
            encryptOut.write(b);
        }

        assertEquals("Uid!pthbj!csnvo!gny!ktlqr!nwds!uid!m`{x!enf/", out.toString());

        String resultStr = out.toString();
        InputStream inResult = new ByteArrayInputStream(resultStr.getBytes("UTF-8"));
        FilterInputStream decryptIn = new DecryptInputStream(inResult);
        OutputStream outResult = new ByteArrayOutputStream();

        b = 0;
        while ((b = decryptIn.read()) != -1) {
            outResult.write(b);
        }

        assertEquals(testStr, outResult.toString());

        encryptOut.close();
        decryptIn.close();
        outResult.close();
    }
}
It looks like you have covered the bases quite well. There are some things I would like to suggest, and the first one is the most important:


  The performance of Input/Output Streams that read/write 1 byte at a time is really, really bad. You should also override the bulk read(...)/write(...) methods.


Now, when you get around to doing the bulk methods, you will have some challenges with getting the byte manipulation right, but that's another problem.... ;-)

Now in your decrypt routine you have:

int b = super.read();
return (b != -1 ? b ^ XOR_KEY : b);


I don't like this. The reason is that it has too much negative logic..... and even though it relates b to a constant -1 you return b instead of -1. Consider the following alternative:

return b == -1 ? -1 : b ^ XOR_KEY;


I know it is a minor nit-pick, but it does make a difference.

On that note, you had better make sure that XOR_KEY is always positive... a negative XOR_KEY will cause the result to be negative, and break all sorts of InputStream readers.

Now, you say the key should be as long as the message... that's not true. Even some of the best algorithms use 2048-bit keys (256 bytes). and many of them use much less (16 bytes...).

Additionally, you hint that you know that storing the key the way you do is incorrect ... It should be passed in as a constructed value or something, and not hard-coded as a static variable.

What you have is more than 'adequate', it is neat, and appears to be fully accurate... but, in order to be good, or great, it needs to do the bulk-operations...