eu-digital-green-certificates / dgc-certlogic-ios

This repository contains the source code of the EU Digital COVID Certificate Certlogic for iOS.
Apache License 2.0
7 stars 9 forks source link

Different results for the same certificate under Android and iOS (Corona-Warn-App and CovPass-App) #49

Closed Ein-Tim closed 2 years ago

Ein-Tim commented 3 years ago

Describe the bug

Checking the same certificate under iOS and Android against the Swiss leas to two different results:

 Android  iOS
twitter_2qSdoXWn

Same goes for CovPass under iOS and under Android, iOS shows fail, Android shows the open status.

Expected behaviour

The result should be the same under iOS and Android.

Steps to reproduce the issue

  1. Add a EU DCC to the Corona-Warn-App under iOS & under Android.
  2. Select "Check Validity"
  3. Select "Switzerland"
  4. See the result

Technical details

Additional context

I hope this is the right place for this issue.

vaubaehn commented 3 years ago

Some additional information: There are currently (as of October 4th, 2021) errors within the Swiss business rules -> see https://github.com/corona-warn-app/cwa-documentation/issues/711#issuecomment-933701107 for two suspects.

These errors lead to certificate status "open" in Android when validated with CertLogic - which is apparently corrrect: due to technical problems with the rules of Switzerland the DCC can not be validated.

This means, the results returned by iOS CertLogic are wrong - they should be "open" instead of "fail".

vaubaehn commented 3 years ago

And more additional information by @Ein-Tim here: https://github.com/corona-warn-app/cwa-documentation/issues/711#issuecomment-934683161

German CovPass app iOS also shows "fail", while CovPass Android shows "open".

vaubaehn commented 3 years ago

ping @martinreichart and @alexchornyi

mlenkeit commented 3 years ago

@alexchornyi the rules VC-CH-0004 and VC-CH-0006 refer to the non-existing parameter external.validationClockAtStartOfDay, so it looks like CertLogic on iOS is handling non-existing parameters differently than Android.

EDIT: Or more likely, CertLogic is handling them as null/undefined internally on both iOS and Android, but because they are passed to plusTime, it might also be that operation that's treating null/undefined differently.

vaubaehn commented 3 years ago

@alexchornyi For the rule VR-CH-0003I'm suspecting an invalid if-then-else structure when validating the existence of a second field within the DCC. CertLogic iOS seems to validate it as "pass" while Android validates it as "open".

Ein-Tim commented 3 years ago

Little update: The problem with the Swiss was fixed on 18.10.2021 on 12:00 UTC, so to reproduce this issue one now has to select a date & time before 18.10.2021 12:00 UTC.

kerstin-oppermann-tsi commented 3 years ago

@Ein-Tim I cannot reproduce this issue anymore. Checkin my certificate for Swiss works on Android and iOS in both apps correct. (A date before today I cannot select) Can you please clarify if this ticket can be closed or provide some further infos if it doesn't further works for you.

Ein-Tim commented 3 years ago

@kerstin-oppermann-tsi

Yes, this works now (as Swiss fixed this issue).

I guess we can close this, however, please note that there was and maybe still is an underlying bug in the certlogic.

vaubaehn commented 3 years ago

Hi @kerstin-oppermann-tsi and @Ein-Tim ,

you cannot reproduce the issue anymore, because the Swiss team fixed their business rules! See your own comment https://github.com/eu-digital-green-certificates/dgc-certlogic-ios/issues/49#issuecomment-949006306

However, the bug that was filed in this issue is, that iOS-CertLogic falsely evaluates broken business rules as "invalid" while they should be evaluated as "open" (see https://github.com/eu-digital-green-certificates/dgc-certlogic-ios/issues/49#issuecomment-934672924). As far as I can see, that iOS bug has not been fixed yet.

Please re-open this issue.

@alexchornyi Could you give an update on the process of fixing this bug? For testing the fix you now would need to create a set of broken business rules. There is just a small chance that France currently has one broken rule in production... (last checked three days ago).

vaubaehn commented 3 years ago

@alexchornyi If you need the old buggy Swiss rules, I could provide these to you here from a backup upon request.

vaubaehn commented 3 years ago

thanks @kerstin-oppermann-tsi

vaubaehn commented 3 years ago

@Hendrik-Schmidt-Schierhorn-TSI and @alexchornyi ,

two different errors in Swiss business rules led to a deviating evaluation in iOS-CertLogic, see here for details: https://github.com/corona-warn-app/cwa-documentation/issues/711#issuecomment-933701107

the comment of @mlenkeit in https://github.com/eu-digital-green-certificates/dgc-certlogic-ios/issues/49#issuecomment-935533733 only covers the wrong parameter external.validationClockAtStartOfDay . This error was falsely validated as "fail" on iOS-CertLogic and should be "open".

The other error is a broken/incomplete if-then-else structure, that was falesly validated as "pass" on iOS-CertLogic and should also be "open".

vaubaehn commented 3 years ago

Hi @mlenkeit or @thomasaugsten , could you please check business rule VR-FR-0009 (currently in prod) where validation has different results in iOS (pass) and Android (CWA 2.6.1 & CWA 2.13.2: open):

   {
      "AffectedFields":[
         "v.0",
         "v.0.ma"
      ],
      "CertificateType":"Vaccination",
      "Country":"FR",
      "Description":[
         {
            "desc":"Only manufacturers in the allowed valueset that have been approved by the EMA are allowed.",
            "lang":"en"
         },
         {
            "desc":"Seuls les fabicants approuv\u00e9s par l'AEM (Value set UE) sont accept\u00e9s en France.",
            "lang":"fr"
         }
      ],
      "Engine":"CERTLOGIC",
      "EngineVersion":"0.7.5",
      "Identifier":"VR-FR-0009",
      "Logic":{
         "if":[
            {
               "var":"payload.v.0"
            },
            {
               "in":[
                  {
                     "var":"payload.v.0.ma"
                  },
                  {
                     "var":"external.valueSets.vaccines-covid-19-auth-holders"
                  }
               ]
            },
            true
         ]
      },
      "SchemaVersion":"1.0.0",
      "Type":"Acceptance",
      "ValidFrom":"2021-10-31T00:00:00Z",
      "ValidTo":"2030-10-31T00:00:00Z",
      "Version":"1.0.0"
   }

At first glance structure/syntax of the rule seems to be correct, and also the pointer to the value set looks appropriate (https://github.com/ehn-dcc-development/ehn-dcc-schema/blob/release/1.3.0/valuesets/vaccine-mah-manf.json) Do you have any idea, why results are different?

Side note: The manufacturers in this value set are not all approved by the EMA - does the French team know this, @SchulzeStTSI ?

thomasaugsten commented 3 years ago

Fixed with https://github.com/corona-warn-app/cwa-app-android/pull/4344 in 2.13.3

vaubaehn commented 3 years ago

Thanks, @thomasaugsten . Having some space is nice in general, but not everywhere...

Ein-Tim commented 2 years ago

Will someone eventually address this issue? Or was it decided to not fix this issue?

mlenkeit commented 2 years ago

Will someone eventually address this issue? Or was it decided to not fix this issue?

@Ein-Tim entering Switzerland is currently possible without providing any kind of DCC-related proof. They have changed their rules accordingly to effectively pass for any certificate:

https://distribution.dcc-rules.de/rules/CH/a68f9b6940fbbd71706293838bfe0bd7561f0203ea2437e81f9a398fdfff181b (that's the only rule for CH at the moment)

Ein-Tim commented 2 years ago

@mlenkeit Thanks, but as @vaubaehn said above https://github.com/eu-digital-green-certificates/dgc-certlogic-ios/issues/49#issuecomment-961837045, the underlying issue was never fixed. But for me it would be OK to close this issue.

BTW, don't we have the same entry rules as Swiss currently? Were our entry rules (Germany) updated in the same way?

kerstin-oppermann-tsi commented 2 years ago

@Ein-Tim The decision about the change of the german rules hopefully will be discussed soon with BMG. We made the same proposal like you mentioned.

The other thing, for the underlying issue within here, maybe the newest change of the certlogic has fixed this, but nevertheless we should close this issue here for now.

Ein-Tim commented 2 years ago

@kerstin-oppermann-tsi

Thanks! I'm closing this issue, could you please keep us updated reg. the business rules situation via https://github.com/Digitaler-Impfnachweis/covpass-ios/issues/190?