Malee dev branch

[MSeechan]

Attempt salt hash password implementation

[andy-keene]

Yup, you're right! This doesn't convert the salt correctly. I wrote a test and in-line comments to explain the issue.

  @Test
  public void testByteStringConversions() {

    // [0, 1, 5, 7]
    byte[] salt = {0, 1, 5, 7};

    // Arrays.toString() makes a string-representation of an array. Think of the conversion like
    // "[" + array[0].toString + ", " + ... + "]";
    //
    // This yields the literal string
    // "[0, 1, 5, 7]"
    String savedSaltString = Arrays.toString(salt);

    // Strings are bytes interpreted using some encoding scheme, like UTF-8. If you convert a string
    // to bytes it will take EACH CHARACTER in the string and convert it to bytes of its encoding scheme.
    //
    // The first character in savedSaltString is actually the "[" added by the Arrays.toString()
    // function. You see the first byte value from savedSaltString.getBytes() is 91. That's because
    // "[".getBytes() = 91.
    //
    // This leads to a couple of things:
    //   1.  "[" and "," in the array string included in .getBytes() - 44 represents a
    //    comma, notice how it repeats?
    //   2. Since .getBytes() returns the UTF-8 byte value for each character, the original values
    //    like 1 will NOT be returned, because "1".getBytes() != 1
    //   3. Side note, UTF-8 characters range from 1 to 3 bytes.
    //
    // Read this Stack Overflow discussion about the exact issue you're dealing with:
    // https://stackoverflow.com/questions/1536054/how-to-convert-byte-array-to-string-and-vice-versa
    //
    // I think the suggestion of storing bytes as a base64 encoding is the best suggestion for most
    // use cases, yours included.
    //
    // [91, 48, 44, 32, 49, 44, 32, 53, 44, 32, 55, 93]
    byte[] reconstructedSalt = savedSaltString.getBytes();


    // True, [91, 48, 44, 32, 49, 44, 32, 53, 44, 32, 55, 93] != [0, 1, 5, 7]
    assertThat(reconstructedSalt).isNotEqualTo(salt);
  }

I'd also suggest making a common class to manage the encoding/decoding & hashing logic so that both servlets can use. This makes it easy to enforce encoding and decoding are in sync. It also cleans up your servlet code and lets them focus on the application logic, not on which encoding scheme is used.

Util classes are common in Java. They're just a container for helper methods. The methods are static so that you don't have to instantiate the class.

With base64 it's look something like

  class HashUtils {
    static public String encode(byte[] bytes) {
      // BTW - Just Google base64 encoders for Java and you'll find this one in the java.util
      // package! https://docs.oracle.com/javase/8/docs/api/java/util/Base64.html
      return Base64.getEncoder().encodeToString(bytes);
    }

    static public byte[] decode(String base64String) {
      return Base64.getDecoder().decode(base64String);
    }

    // add other utils here.. like
    static public byte[] hashWithSalt(String password, byte[] salt) {
      // this is your code :)
      return null;
    }
  }

Then we can run the same encode/decode test:

  @Test
  public void testByteBase64Encodings() {
    byte[] salt = {0, 1, 5, 7};

    // What you'd save
    // "AAEFBw=="
    String encodedSalt = HashUtils.encode(salt);

    // How you'd retrieve the saved salt as a byte array
    // [0, 1, 5, 7]
    byte[] decodedSalt = HashUtils.decode(encodedSalt);

    // True! [0, 1, 5, 7] = [0, 1, 5, 7]
    assertThat(decodedSalt).isEqualTo(salt);
  }