ehn-dcc-development / hcert-java

Java version of the protocol
3 stars 6 forks source link

CBOR encoding in GreenCertificateEncoder will be incorrect for date-times #6

Open martin-lindstrom opened 3 years ago

martin-lindstrom commented 3 years ago

Hi

I've been looking into the CBOR encoding more in detail and se that if you use CBORObject.FromJSONString(json) all elements that should be coded as a CBOR dateTime type (tag 0 or 1) will be coded as ordinary strings (plain or UTF-8). The reason is of course that a date in JSON is a string and there is no way for the CBORObject.FromJSONString to know that some of the strings should be coded as a CBOR dateTime type.

    private byte[] getCborBytes(String json) {
        CBORObject cborObject = CBORObject.FromJSONString(json);

        return cborObject.EncodeToBytes();
    }

See https://github.com/ehn-digital-green-development/hcert-schema/issues/17.

Also check out my tests at: https://github.com/DIGGSweden/hcert-impl/blob/main/src/test/java/se/digg/hcert/eu_hcert/v1/MapperUtilsTest.java

Just want to make sure that we are interoperable ...

jkiddo commented 3 years ago

@martin-lindstrom there's a lot of great stuff in your impl, really great stuff. I almost feel I should either copy 80% of your project or discard this repo, with a couple of exceptions. The reason why I feel I cannot discard this repo are the following:

-anyways, your stuff looks good

martin-lindstrom commented 3 years ago

Many thank for your kind words.

Regarding your comments:

martin-lindstrom commented 3 years ago

I tried to build with Java 8. Won't be possible since we include a dependency to the credentials-support library that is essential for our HSM signing capabilities. See https://github.com/DIGGSweden/hcert-impl/issues/5

But what are you still doing using Java 8? It was long ago it was declared EOL.

jkiddo commented 3 years ago

Tell that to my customer ...

jkiddo commented 3 years ago

Ill see if i can change their minds

martin-lindstrom commented 3 years ago

While your at it, tell them that Lombok is only used at compile time ...

jkiddo commented 3 years ago

I know ... But that is another thing

jkiddo commented 3 years ago

Many thank for your kind words.

Regarding your comments:

  • Lombok: I can remove the Lombok-dependency. Right now I am only using the @Slf4j annotation.
  • What if I make the dependencies to the zxing-jars optional?
  • I can make a Maven-profile where I build using Java 8 ...

@martin-lindstrom If you can remove the use of lombok and move the use of the zxing-jars to test scope (which should be possible - and it would still illustrate how you could transform the certificate to a QR) it would certainly fit my case

martin-lindstrom commented 3 years ago

I will mark the zxing-jars as optional which makes it possible for you to use the library (as long as you don't create or decode barcodes). I can't have them in test-scope since creating a barcode will be one of the features of the library.

martin-lindstrom commented 3 years ago

The stuff checked in to master is now written in a way that you can use the DGCEncoder and DGCDecoder without barcodes.

jkiddo commented 3 years ago

Great @martin-lindstrom - do you still think it is nessecary with the swedish dependency in https://github.com/ehn-digital-green-development/dgc-java/blob/173c4dfc380ff19ff105b3439282554f6d1160d1/pom.xml#L135 since it is now a fork that probably should be usable by others than the swedish produced solutions (which it ofc. is but I don't see the dep usable by other than the swedish manufactures). As far as I can see, it could be abstracted away

martin-lindstrom commented 3 years ago

Great @martin-lindstrom - do you still think it is nessecary with the swedish dependency in https://github.com/ehn-digital-green-development/dgc-java/blob/173c4dfc380ff19ff105b3439282554f6d1160d1/pom.xml#L135 since it is now a fork that probably should be usable by others than the swedish produced solutions (which it ofc. is but I don't see the dep usable by other than the swedish manufactures). As far as I can see, it could be abstracted away

That dependency is to a generic library for PKI credentials (private key/certificate) and it is nothing Swedish about it other than it is released from se.swedenconnect. I will not remove that dependency since it has a very good support for PKCS#11 (HSM) which I hope more than Sweden will use for their signers.

jkiddo commented 3 years ago

Point taken