Repository navigation
fix: take the identity claims from the chip, not from MRZ OCR - #84
Conversation
The credential's identity fields came from FaceTec's OCR of the MRZ in the camera image (mrzValues overriding scannedValues). An OCR misread, typically a '<' filler read as a letter, put an extra letter in a tester's name. The chip was read and verified, but only used to cross-check the OCR; a mismatch made the chip untrusted, which refused a passport under trust.required but let an ID card (or any document without trust enforcement) through with the misread. Once local passive authentication has verified the SOD signature and the data-group hashes, emrtd.Verify now returns the holder's identity as signed on the chip: names, document number, dates of birth and expiry, nationality, issuing state and sex from DG1, refined by DG11 (full, untruncated name; date of birth with century) when the chip presents it. assessChip overwrites the OCR'd fields with it, whether or not the PDP trusts the signer: signed chip data is never worse than OCR of the page printed next to it. Country codes are normalised to alpha-3, so ICAO's "D" (Germany) now maps to DE in the credential. The OCR->DG1 comparison becomes a diagnostic: differing fields are reported by name (never values) and logged. Only the document type must still agree with the signed document code, since it selects the policy rule. The SOD signing time is bounded by the chip's expiry date instead of the OCR'd one. Documents without an ICAO chip keep the OCR values. Fixes #83
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ptions Codecov reported 75% patch coverage: the new emrtdtest options (DG11, MRZ sex) were only exercised from other packages, which per-package coverage does not count, and a few branches had no test. emrtdtest now has tests of its own that round-trip DG1 and DG11 through gmrtd, and there are tests for the M/F/X sex mapping, an impossible DG1 expiry date (fails closed, dg_malformed) and an ICAO country code with no ISO 3166 equivalent (kept as signed). emrtdtest gains a DG1ExpiryYYMMDD option for the expiry case.
|
Haven't had time to look through the PR yet, but a question arose based on the PR description:
and
Is there any scenario where we'd feel okay issuing a credential just based (trivially falsifiable) OCR values? Apologies if I've misunderstood anything or read to much into these text snippets. |
in practice, we are not issuing any credentials for documents without a verified chip. |
Good! What is the reason for doing anything with the OCR data in that case? |
Legacy. So we can try to see what we can prune, if you wish. |
|
@s-jairl
Requiring chip-signed identity for every credential would close these, but would also exclude driving licences until we support ISO 18013 chips. That’s a team decision, so I’ll keep it out of this PR. This PR only makes passport and ID-card claims come from the chip. |
I agree that it would be a bigger change and probably best kept out of scope for this PR - I'll create a separate issue :-) |
applyChipIdentity no longer keeps an OCR'd value where the chip leaves a field empty: a single-name holder has no given name, an MRZ date of birth with unknown parts gives no birth_date or age claims, and an unspecified sex is ISO/IEC 5218 0. None of these is filled in from OCR. Refs #83
|
Not sure if this is strictly within scope for the PR, but it looks like we fall back to a cropped portrait from a photo of the identity document if it's missing from the chip: facetec-api/internal/facetec/process_request.go Lines 141 to 154 in 457f80d Probably wise to confirm before merging or at least creating a separate issue/PR if this is the case. |
bindPortraitToChip only ran for a PDP-trusted chip, so a chip that verified locally without PDP trust (e.g. an ID card) got the chip's identity next to FaceTec's photoIDFaceCrop, cropped from the camera image of the document. The portrait now follows the identity: the SOD-verified DG2 image, or none. Refs #83
|
Thanks, confirmed, and fixed in 7155f65.
The portrait now follows the identity. Whenever the chip's identity is applied, the portrait is the DG2 image, or none if DG2 was not verified. The new Without a verified chip at all, FaceTec's portrait still stays, like the OCR'd fields. That case is part of #85. |
|



Fixes #83
A tester's Photo ID had an extra letter in her name. The identity claims came from FaceTec's OCR of the MRZ in the camera image (
mrzValuesoverridingscannedValues). An OCR misread, typically an MRZ<filler read as a letter, went straight into the credential. This PR takes those fields from the document's chip instead.Before
The chip was read and verified (#64), but only used as a check:
trust.required: a misread name meant a refusal (chip_untrusted).After
Once local passive authentication has verified the SOD signature and the data-group hashes,
emrtd.Verifyreturns the holder's identity as signed on the chip (Result.Identity):<sex is "unspecified".assessChipoverwrites the OCR'dDocumentDatawith it. Everything downstream then uses the chip values: the expiry gate, the policy and the credential.85<<<<) gives nobirth_dateor age claims, and an unspecified sex is ISO/IEC 52180. None of these is filled in from OCR.photoIDFaceCrop(cropped from the camera image of the document) is replaced. If DG2 was not verified, no portrait is issued. Before, this binding ran only for a PDP-trusted chip, so an ID card could get the chip's name next to a camera crop.D(Germany) now becomesDEin the credential. It used to stayD.chip_identity_applied.The OCR→DG1 comparison is now a diagnostic:
Result.Mismatches) and logged without values;ReasonMRZMismatchis removed, since nothing produces it any more.Scope
Not in this PR:
birthplace. That's a data-minimisation decision.Merge note
#80 also adds an audit field. Whichever merges second needs
TestAssessChip_FillsScanResult's field count (assert.Len(t, fields, 6)) bumped to 7.Tests
emrtdpackage:K) now verify, are reported by field name, and the identity is the chip's;mrzDateToISOcentury cases.emrtdtest) can now emit a DG11, set the MRZ sex and date of birth, and encode a single-name holder.apiv1package:trust.requiredis no longer refused aschip_untrusted;Dmaps toDE;go vetis clean,go test -race ./...passes, andgovulncheckwith Go 1.27.2 finds no vulnerabilities.