Serializable Keys

[SalusaSecondus]

Description of changes:

Java specifies that implementations of Key are Serializable. Currently ACCP does not allow its keys to be serialized due to the difficulties in handling pointers into native memory and fixing them up while still letting them be final. This change safely serializes and deserializes instances of EvpKey by using a secondary helper class. A code comment explaining how this works is duplicated below.

Java Keys are Serializable but we cannot use the trivial serialization of an EvpKey.

• They contain a pointer to native memory (and associated native memory). This obviously cannot remain valid after being serialized/deserialized.
• The pointer to native memory is final to ensure there is no risk of it being invalid. This means that we cannot just fix it up when deserializing.

The trivial solution would be to make the pointer non-final, but that introduces a greater risk of serious bugs.

Instead, we use writeReplace() and readResolve() to store our information in a dedicated format for serialization.

• writeReplace() on the object to be serialized (an EvpKey) returns the real object to be serialized. In our case, we return an instance of SerializedKey which contains the minimal information to properly save and retrieve a key.
• readResolve() is called on the object which is actually serialized (and thus being deserialized) and returns the object that users care about. In our case, SeralizedKey.readResolve() returns an appropriate instance of EvpKey.

So, the entire (hidden from the user) flow goes like this.

1. User tries to serialize an EvpKey
2. Java detects that EvpKey.writeReplace() exists and calls it
3. EvpKey.writeReplace() returns an instance of SerializedKey
4. Java actually serializes SerializedKey
5. (Some time passes)
6. User tries to deserialize the bytes
7. Java detects that the bytes contain a serialized SerializedKey and so deserializes it.
8. Java detects that SerializedKey.readResolve() exists and calls it
9. SerializedKey.readResolve() creates and returns an instance of EvpKey
10. User gets an instance of EvpKey and never realizes that the above complexity exists

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[amirhosv]

I'm not sure if it's a good idea to support or advocate this. Java's serialization mechanism is not the most suitable approach for serialization and it's best for customers to use more standard crypto formats like ASN1 encodings for such requirements. Also, not all providers need to implement a meaningful implementation for serialization; for example, SunPKCS11 provider that allows applications use keys backed by HSM does not (and cannot provide) meaningful implementation.

The problem with supporting this is it's maintenance can be become a hassle in future and it's best to push customers to avoid using this and rely on more standard formats.

[SalusaSecondus]

@amirhosv I completely agree that people shouldn't serialize keys in this way. However, it is a way in which ACCP differs from the providers bundled with the JDK. The JDK's tests assume that keys are serializable (and so fail for meaningless reasons). Because this solution is built off of existing standardized encodings, the ongoing maintenance cost should be minimal.

So, fixing this makes it so that ACCP is even more of a transparent drop in replacement than it was before.

[amirhosv]

@amirhosv I completely agree that people shouldn't serialize keys in this way. However, it is a way in which ACCP differs from the providers bundled with the JDK. The JDK's tests assume that keys are serializable (and so fail for meaningless reasons). Because this solution is built off of existing standardized encodings, the ongoing maintenance cost should be minimal.

So, fixing this makes it so that ACCP is even more of a transparent drop in replacement than it was before.

Could you share the tests that are failing in JDK?

[brian-jarvis-aws]

Hey @SalusaSecondus, just wanted to give a little more context here. We're trying to get a better understanding of what the use case is for this. We aren't necessarily opposed to bringing it in; as you noted, it shouldn't add a lot of ongoing maintenance. However, a tenet of ACCP is that it is opinionated and I see from the correspondence that both you and @amirhosv are of the opinion that people should not serialize keys in this way. As such, I would expect that the justification for this should outweigh the enablement of what both of you describe as an anti-pattern (my paraphrasing). So, we're just wondering if you could share more about the motivation and use case so we can better understand what the problem is with the current solution.

[SalusaSecondus]

The failing test is here: https://github.com/openjdk/jdk/blob/master/test/jdk/javax/xml/crypto/dsig/GenerationTests.java#L464-L468

While this is a minor issue, getting patches into the JDK is orders of magnitude harder than getting them into ACCP. Neither do I believe that making keys serializable in ACCP will encourage the anti-pattern. If people are already doing the bad thing, they'll just blame ACCP for not being correct and thus won't use ACCP (rather than fixing things). If they aren't doing the bad thing, then better matching the JCA specification won't break things either.

Supporting serialization makes it easier and more reliable for ACCP to be a drop-in transparent replacement. Every friction point makes it harder to get people to adopt ACCP.

[WillChilds-Klein]

It sounds like merging this PR would unblock execution of JDK's JCE tests against ACCP, which would provide useful data on where we stand in terms of compatibility with the default JCE provider(s). Assuming that the maintenance cost of EvpKey's serialization format through time is low (which I agree that it is very likely to be given PKCS8/x509 formats' stability), I'm OK with merging this change.

[amirhosv]

The mac CI task failure has nothing to do with this PR. This PR will resolve those. Once PR 306 is merged, please consider rebasing.