PSK: Remove withNonce from PskMessageBuilder?

[sappenin]

Wondering if we should remove #withNonce() and #withNonce(byte[] nonce) from PskMessageBuilder, and just let the builder implementation always do the right thing with respect to the nonce? (e.g., make #withNonce() private and just do that by default).

The nonce is supposed to be cryptographically random, and so it seems a bit risky to allow a caller to send any ole data into there. Wouldn't it be better to merely have something like this as the builder contract...

// Notice no "withNonce" call...
final PskMessage pskMessage = new PskMessageBuilder()
      .addPrivateHeader(privateHeader1)
      .addPublicHeader(publicHeader1)
      .setApplicationData(applicationData)
      .toMessage();

... and let the PskMessageBuilder always create random nonce data using SecureRandom? That way the implementation can control the quality of the nonce in all cases.

Thoughts?

[earizon]

+1

[andrew-g-za]

The thinking was that we deliberately provided a default implementation - withNonce() - that would do the right thing, but also provide a withNonce(byte[]) for users who wanted to do something special - for example if a user trusts the bouncy-castle random generator more, or have some custom nonce scheme, or some other special requirement. Having the two methods give it more flexibility, but at the risk that its either abused or simply omitted.

Since the builder is meant to be a convenience, perhaps you are right and it should be more opinionated - it should do the right thing, and if someone wants something special they either implement their own builder or construct PskMessages directly (or wrestle with the headers after using the builder). That way at least there is no risk of the mandatory nonce header being accidentally omitted.

+1 Go for it.

[adrianhopebailie]

I think using SecureRandom is the right way to go. This way you stick with Java's intentional separation of concerns between the application code and the deployment (and thereby choice of randomness).

The JVM can be configured outside of the application to use different sources of randomness under the hood of SecureRandom.

Useful reference: https://tersesystems.com/2015/12/17/the-right-way-to-use-securerandom/

[andrew-g-za]

Cool, looks like we are all agreed. Will make the change quickly.

[andrew-g-za]

@sappenin - I see there are now two sets of PSK tests, one in

org.interledger.ilp.psk and one in org.interledger.psk. I presume i should remove the original set in org.interledger.ilp.psk?

[sappenin]

Yes, that looks like an oversight - the tests in org.interledger.ilp.psk should no longer be there (though we should double-check the contents to make sure there aren't significant differences between the two sets of tests -- My guess is that was just an artifact of package moving, but just to be safe...).

[andrew-g-za]

Cool. Bar import differences they are the same. I'll remove the duplicate tests in org.interledger.ilp.psk with the submission.

[adrianhopebailie]

In the refactor I did to get consistency across builders I have allowed the specification of a nonce. This is required when deserializing a message. I think we should allow users the flexibility to build a message however they want but generate a nonce if required using SecureRandom.

WDYT? Can we close this?

[sappenin]

LGTM. I like how we create one by default in the proper way, so I'm satisfied. I'll close this issue and see how it is to develop against that builder (I'll create a new issue in the future if there's any reason to).