Re-introduce crypto-conditions

[adrianhopebailie]

How I would implement https://github.com/interledger/java-ilp-core/issues/101

This highlights a few issues that have been introduced into java-crypto-conditions OR suggests this is a bad approach:

1. The implementations of the different conditions have package private constructors for constructing a condition from just the fingerprint. This makes creating an instance from a codec.read() operation a challenge.

2. As a result of not being able to use org.interledger.cryptoconditions.PreimageSha256Condition (see above) it was necessary to create an alternate implementation. This may not be a bad thing to do but we should consider whether these are comparable to each other. I.e. Are two conditions equal where they have the same hash but one is an instance of org.interledger.cryptoconditions.InterledgerSha256Condition and the other org.interledger.cryptoconditions.PreimageSha256Condition

3. The codec context fails lookups if the provided class for a write operation implements an interface that extends another interface that was used to register the codec. i.e. Passing an instance of InterledgerSha256Condition fails because it implements SimpleCondition which extends Condition which is the class used to register the codec. The issue is in https://github.com/interledger/java-ilp-core/blob/development/src/main/java/org/interledger/codecs/CodecContext.java#L297

[sappenin]

The implementations of the different conditions have package private constructors for constructing a condition from just the fingerprint. This makes creating an instance from a codec.read() operation a challenge.

Yes, I see the problem. I think the constructors of PreimageSha256Condition are improperly structured (read more about that below).

Overall, I definitely think we do not need a separate class type for InterledgerSha256Condition, but I am open to keeping InterledgerSha256Fulfillment. But even then, I'm not so sure.

From my perspective, this issue has two orthogonal concerns:

• How do we encode an ILP Condition/Fulfillment in OER, while in the same application allow a crypto-condition variant to be encoded/decoded using DER.
• How do we restrict the ILP Condition/Fulfillment to have a cost of 32, and a type of `PREIMAGE_SHA_256".

The first concern (encoding) is already solved by the Codec framework. First, in the general case, no developer should be directly creating a PreimageSha256 Condition. Instead, developers should be creating a Fulfillment and then generating the Condition from there. Two exceptions to this rule are in a Codec, or in a testing scenario (and in the testing scenario, creating a Fulfillment, and then a Condition, should suffice). In the Codec scenario, we do need to allow developers to deserialize from something (bytes, JSON, etc) into a Condition class, which means we need some sort of Constructor there to allow Codecs that don't live in the org.interledger.cryptoconditions package. More specifically, encoding/decoding of a Condition should be the same for either type, but we need a mechanism to allow a single piece of software to encode the same class (PreimageSha256Condition) using two different encodings: OER sometimes, and DER sometimes. In this case, I think the CodecFactory solves for this: a developer simply needs to create two different codecs (one for OER, and one for DER), and use them where appropriate. Likewise if a third or fourth encoding were to be introduced (like JSON or XML, for example), then there would be additional Codec's constructed. In other words, it's already a requirement for the same application to be able to encode/decode the same classes using different encodings, and the Codec framework solves for this -- so, no need to create a special class-type just to satisfy the Codec.

Next, on the Fulfillment side, we need to allow developers to be able to create a special type of PreimageSha256Fulfillment called InterledgerSha256Fulfillment that restricts the preimage size and cost. Here, I'm on the fence. On the one hand, we could create just a factory class called InterledgerSha256Fulfillment, and it will simply generate a PreimageSha256Fulfillment while enforcing the constraints that it should have. The downside to this is that in various service interfaces, the type would still be PreimageSha256Fulfillment, and implementations would need to enforce the restrictions, which is not ideal.

So, this is why I'm open to a new type of Fulfillment, specifically for ILP, that lives in java-ilp-core. To that end, I propose the following:

• Adjust the constructor for PrimageSha256Condition that takes a preimage to be package-private. Nobody should be constructing a condition from a preimage (instead, they should construct a Fulfillment and then get the corresponding Condition).
• Adjust the constructor that takes a cost and fingerprint to be public. This will allow Codec developers to deserialize whatever encoding they're reading into an Instance of PreimageSha256Condition. In my mind, the PreimageSha256Condition and the InterledgerSha256Condition are both PreimageSha256 conditions -- only difference is the required cost, but that value will have come from a Fulfillment that was serialized to the encoding, so no need for a special class-type here -- it's all a PreimageSha256Condition.
• Adjust the toString of PreimageSha256Fulfillment to use the class simpleName when emitting the toString.
• Keep the class InterledgerSha256Fulfillment, and structure it like this (extending from PreimageSha256Fulfillment):

/**
 * An implementation of {@link Fulfillment} for a crypto-condition fulfillment of type
 * "PREIMAGE-SHA-256" based upon a preimage and the SHA-256 hash function.
 *
 * @see "https://datatracker.ietf.org/doc/draft-thomas-crypto-conditions/"
 */
public class InterledgerSha256Fulfillment extends PreimageSha256Fulfillment
        implements Fulfillment<InterledgerSha256Condition> {

    public static final int INTERLEDGER_FULFILLMENT_LENGTH = 32;

    /**
     * Constructs an instance of the fulfillment.
     *
     * @param preimage The preimage associated with the fulfillment.
     */
    public InterledgerSha256Fulfillment(final byte[] preimage) {
        super(preimage);

        if(preimage.length != INTERLEDGER_FULFILLMENT_LENGTH) {
            throw new IllegalArgumentException("Preimage must be exactly 32 bytes");
        }
    }
}

In this way, an interface could specify a InterledgerSha256Fulfillment or a PreimageSha256Fulfillment, and interfaces (like java-ilp-plugin) could either type to PreimageSha256Fulfillment and accept both without issue, or be made generic, like this:

interface handleFulfillment<T extends PreimageSha256Fulfillment>{
  void handleFulfillment(T fulfillment);
}

In summary, this counter-proposal seems to solve for all three of the requirements:

• Allow a Condition/Fulfillment to be encoded using OER (hash for a condition and preimage for a Fulfillment).
• Force the InterledgerSha256Fulfillment to always be of type PREIMAGE-SHA-256.
• Force the InterledgerSha256Fulfillment to always have a cost of 32.

Anything I'm missing?

[adrianhopebailie]

If you're cool fixing the constructors then I am keen to drop both custom implementations. They are only there out of necessity, I'd prefer not to have them.

I don't think it's an issue throwing an error during the Interledger-specific OER encoding if the fulfillment length != 32 or the condition cost != 32.

One issue I failed to mention was that deprecating getFingerprint() is problematic. I think we should keep it in and simply return a copy of the array each time.

[sappenin]

If you're cool fixing the constructors then I am keen to drop both custom implementations. They are only there out of necessity, I'd prefer not to have them.

Even better! I'll make the changes to https://github.com/interledger/java-crypto-conditions then and report back here once that's completed.

[adrianhopebailie]

Even better! I'll make the changes to https://github.com/interledger/java-crypto-conditions then and report back here once that's completed.

Done, see https://github.com/interledger/java-crypto-conditions/pull/73

There are a few issues in this that make me think Crypto-Conditions could do with a refactor. Type checking based on the getType() is useless if the different types have different members that you want to access.

We should probably define an interface for each type so that alternative implementations can exist and still expose things like PreimageSha256Fulfillment.getPreimage().

Also, the inconsistency between returning Base64Url encoded data and byte arrays sucks. I'd much prefer we just do a safe copy of the array to maintain immutability. Until it's a perf problem we shouldn't prematurely optimize.

Finally, the convention within the ILP project has been to use Base64Url encoding with no padding. We need to be consistent in that too.

[sappenin]

From @adrianhopebailie:

We should probably define an interface for each type so that alternative implementations can exist and still expose things like PreimageSha256Fulfillment.getPreimage().

I Agree. I created https://github.com/interledger/java-crypto-conditions/issues/75 for tracking.

[sappenin]

From @adrianhopebailie:

Also, the inconsistency between returning Base64Url encoded data and byte arrays sucks. I'd much prefer we just do a safe copy of the array to maintain immutability. Until it's a perf problem we shouldn't prematurely optimize.

I agree. Also added an issue to discuss perhaps just removing the Base64Url variant entirely. See: https://github.com/interledger/java-crypto-conditions/issues/76

[sappenin]

From @adrianhopebailie:

Type checking based on the getType() is useless if the different types have different members that you want to access.

I'm not sure what you mean by the above statement - was this something you encountered in the codec in java-ilp-core, or something you're noticing in the java-crypto-conditions library itself?

[adrianhopebailie]

Type checking based on the getType() is useless if the different types have different members that you want to access.

I'm not sure what you mean by the above statement - was this something you encountered in the codec in java-ilp-core, or something you're noticing in the java-crypto-conditions library itself?

Yes. When encoding a Condition/Fulfillment we must check that it is a PREIMAGE SHA-256 type condition (as only these are accepted). The correct way to do this would be to call Condition.getType() as this should return a consistent value for all implementations.

However, we also need access to members like 'PreimageSha256Fulfillment.getPreimage()` so we need to cast to a specific type anyway.

I think interledger/java-crypto-conditions#75 will solve this