search

ministero-salute / it-dgc-verificac19-sdk-android

Digital Covid Certificate SDK
Apache License 2.0
57 stars 30 forks source link

fix: check uvci and blacklist before signing certificate #110

Closed astagi closed 2 years ago

astagi commented 2 years ago

How to reproduce

  1. Try scan this testing certificate (from EU testing DCCs repository) with VerificaC19 for Android
  1. Try to scan it using VerificaC19 for iOS

You'll get two different results! Only VerificaC19 for iOS correctly reads certificate's info saying that's not valid cause it's not signed by a trusted DSC. Current PR fixes this behaviour.

Light2288 commented 2 years ago

@astagi can you please explain better the issue? i tried with the current develop branch built app both from ios and android, and the certificate is not valid in both cases do you mean the not valid result is obtained from two different conditions in ios and android?

astagi commented 2 years ago

@astagi can you please explain better the issue? i tried with the current develop branch built app both from ios and android, and the certificate is not valid in both cases do you mean the not valid result is obtained from two different conditions in ios and android?

@Light2288 the difference between iOS and Android versions is that in the former I get person and dob data during verification, in the latter I get not valid DGC message

Kaizen-7 commented 2 years ago

@astagi The fix seems right to me, the problem was caused by the fact that the certificateIdentifier was not set due to the "return @WithContext".

The only thing I would change is the check on the blackList, that we could leave it where it was, since if the certificate is not signed correctly then it cannot be blacklisted. So we would avoid calling a method

Light2288 commented 2 years ago

@astagi I was using the validation environment app, with that one the result is the same (as the problem is linked to the signing certificate not found). With the production environment app, i confirm the behaviour you described, we analyzed the code with @Kaizen-7 so apart from the small changed @Kaizen-7 pointed out in his comment (move only the certificateIdentifier setting line, and leave the blacklist check where it is) your pr is ok :)

astagi commented 2 years ago

@Kaizen-7 @Light2288 I agree with changes requested! done with commit amen! Can I merge it now?

Light2288 commented 2 years ago

approved and merged ;) thanks @astagi

astagi commented 2 years ago

Thanks! YW @Light2288 @Kaizen-7