PSK: PskMessage does not contain an Encryption header until after its written

[sappenin]

Should instances of PskMessage built by the PskMessageBuilder.java contain the Encryption header before the PSK data is actually written via PskWriterFactory?

As it stands now, that header is only written by the PskWriterFactory, and so doesn't show up in Java until the PskMessage is written, and then read back into a PskMessage.

As an example, consider the following snippet...

final PskMessage pskMessage = new PskMessageBuilder().withNonce()
      .addPrivateHeader(privateHeader1)
      .addPublicHeader(publicHeader1)
      .setApplicationData(applicationData)
      .toMessage();

// This is always false (until the message is written, and then read back into a PskMessage
    boolean isPresent = decodedPskMessage.getPublicHeaders()
          .stream()
          .filter(header -> header.getName().equalsIgnoreCase("Encryption"))
          .findFirst().isPresent();

...isPresent is always false, which was unexpected.

[andrew-g-za]

Yeah, this was one of the awkward trade-offs with the writer / message design that I was wrestling with. At the moment the message encryption is handled by the appropriate writer, so it's the writers job to set the correct encryption header based on what its doing (either none, or the aes-256-gcm indicator plus auth data). This leads to the problem you have pointed out.

Unfortunately another problem with this pattern is that setting an encryption header in the builder is at best meaningless, and at worst misleading, since the 'real' header will be created by the writer. For example, I think its pretty hard to reason whats going to happen here:

final PskMessage pskMessage = new PskMessageBuilder().withNonce()
      .addPrivateHeader(privateHeader1)
      .addPublicHeader(publicHeader1)
      .addPublicHeader("Encryption", "none")   //NB <-- no encryption in this message please
      .setApplicationData(applicationData)
      .toMessage();

PskWriterFactory.getEncryptedWriter(key).writeMessage(pskMessage) //NB <-- encrypted writer, aaarg whats going to happen?

I think i left a TODO in the builder about this, because i haven't stumbled on a clean, concise way of addressing it that fits all cases.

We could fix it by simply having the writer validate that the appropriate encryption header is set, so that at least we enforce that the correct writer is used with the corresponding header.

Or maybe we should have the user set the encryption header in the builder, then pass the PSK message to the writer factory which would return the appropriate writer based on inspecting the header, something like:

PskWriterFactory writerFactory = PskWriterFactory.initailize(secretKey);

final PskMessage pskMessage = new PskMessageBuilder().withNonce()
      .addPrivateHeader(privateHeader1)
      .addPublicHeader(publicHeader1)
      .addPublicHeader("Encryption", "none")   //NB <--
      .setApplicationData(applicationData)
      .toMessage();

byte[] pskMessageData = writerFactort.getWriter(pskMessage).writeMessage();

This would at least get the header visible up front, but unfortunately the exact value of the header before writing and after writing (for the aes-256-gcm method) would look different, since the auth tag in that header would only be present after the writer has done its job. I don't think that's a deal-breaker, but if you are concerned that the output of the builder doesn't match the output of the writer, that problem has only partly gone away.

One of the alternatives could be to forgo the writer entirely, and let the builder manage the encryption if requested. That way its clear whats going to happen, but you won't have access to a PSK message object when writing one, though what you would do with it anyways I'm not exactly sure. The output of the builder would instead be a byte[] ready to be put on the wire. For example, something like:

final byte[] encryptedPskMessage = new PskMessageBuilder().withNonce()
      .addPrivateHeader(privateHeader1)
      .addPublicHeader(publicHeader1)
      .setApplicationData(applicationData)
      .setEncrypt(secretKey)
      .toMessage();

final byte[] unencryptedPskMessage = new PskMessageBuilder().withNonce()
      .addPrivateHeader(privateHeader1)
      .addPublicHeader(publicHeader1)
      .setApplicationData(applicationData)
      .toMessage();

Does that all make sense? Have I missed a trick somewhere? What are your thoughts?

[sappenin]

Hmmm...interesting problem. What if we did this:

1. Enforce in the MessageBuilder.addPublic/Private headers that you can't add a header with the names Encryption, Nonce, or Key.

2. We remove the distinction between an "Encryption Header" (currently PskEncryptionHeader) and a regular header (currently PskMessageHeader)? These aren't actually different things from a Java model perspective. Instead, an encryption header is just a header with the name Encryption, containing special data that our library should be assembling, and which happens to be a reserved header name.

So, what if we delete PskEncryptionHeader and then add an EncryptionType to the Message? That way, we can even get rid of the distinction between an encrypted reader/writer vs a non-encrypted reader/writer.

So, for example, if I want to write an encrypted message, I specify the encryption type in the PskMessageBuilder, like this:

PskMessage message = new PskMessageBuilder()
  .withEncryptionType(AES_256_GCM)
  //...other withers
  .toMessage();

PskMessageWriterFactory.getWriter().write(message);

If I don't want encryption, I would do this:

new PskMessageBuilder().withEncryptionType(NONE)
  //...other withers
  .toMessage();
PskMessageWriterFactory.getWriter().write(message);

By default, the MessageBuilder should default to AES_256_GCM, so anybody forgetting to call withEncryption doesn't accidentally make a mistake. And, the reader can simply inspect the message to determine if it should encrypt or not.

Last but not least, based upon the supplied encryption type, the MessageBuilders can simply do the right thing with the Nonce and the Key headers. (Btw, my read of the PSK spec is that the Nonce header shouldn't show up if Encryption=NONE), and the readers/writers should just work properly.

Thoughts? If I get time this weekend, maybe I'll put something together to illustrate this.

[sappenin]

One of the alternatives could be to forgo the writer entirely, and let the builder manage the encryption if requested. That way its clear whats going to happen, but you won't have access to a PSK message object when writing one, though what you would do with it anyways I'm not exactly sure. The output of the builder would instead be a byte[] ready to be put on the wire.

This is an interesting idea as well -- I've been going back and forth in my mind about this, and it's making me think that perhaps we need to separate the concepts of PskMessage and EncryptedPskMessage. For example, PskMessage has no Nonce, Encryption nor Key values. It's just the raw input that a developer would put into it, unencrypted. This could then be transformed into an EncryptedPskMessage, which would have all the proper headers, and would have a Java contract that hid implementation details like nonce-creation, proper "Encryption" header, etc from the outside, but would still be useful for certain testing scenarios. Then, the reader/writer could read/write these things, and translate them back to PskMessage so that a developer using these would actually be able to read the information.

I can't stop thinking about the fact that we want some information to be encrypted for on-the-wire purposes, but we want the data to be un-enecrypted when it's being used by a developer. Seems like it might help to separate these two concepts more formally in our object model?

Also, I'm starting to feel like this whole thing could be wildly simplified if we didn't support the concept of Encryption: NONE. It seems like this was added purely as an aid to developers for testing purposes, and is not meant for production. It adds a whole ton of code-path forks and makes things really strange from a development perspective, with all kinds of weird edge-cases.

[adrianhopebailie]

Also, I'm starting to feel like this whole thing could be wildly simplified if we didn't support the concept of Encryption: NONE. It seems like this was added purely as an aid to developers for testing purposes, and is not meant for production. It adds a whole ton of code-path forks and makes things really strange from a development perspective, with all kinds of weird edge-cases.

That's been my argument on the core issue too.

The idea of encrypted PSK and unencrypted PSK is weird. They are at different layers of the stack.

[adrianhopebailie]

You guys should comment on https://github.com/interledger/rfcs/issues/197

Implementor feedback is very important

[adrianhopebailie]

I have changed this in my PR (https://github.com/interledger/java-ilp-core/pull/54) and it feels cleaner so I'd be interested to hear what you guys think.

The way it works now is that an PskMessage is either encrypted or not. You can determine this by looking at the encryption header.

If it is encrypted then the data is in encrypted form and includes the encrypted private headers, if not then the data is in the clear and there are private headers that can be accessed directly through PskMessage.getPrivateHeaders().

Using a PskContext a PskMessage can be translated from encrypted to decrypted and visa versa. This is no longer coupled to the reading and writing.