Update OpenSSL, variable renaming, formatting, enforce CA #263
Conversation
ospfranco
changed the title
| // | ||
|
|
||
| import Foundation | ||
| import OSLog |
There was a problem hiding this comment.
Here formatting changes make everything hard to read :S I'm sorry. Let me know if this is still ok to review/merge
There was a problem hiding this comment.
Hi, could you undo the formatting please?
I'll manually include the updates to OpenSSL-Package and updates for CocoaPods (not the bump to latest as that requires more testing) but won't include the other code changes in this release (just lack of time) but once the PR is updated then I'll take a look.
I've already added custom AA (as per previous release) and that will be going up too
There was a problem hiding this comment.
I'm trying to match the old formatting style via swift-format but it seems paranthesis spaces and other weird character spacing is not part of the official spec. Which tool are you using to format the code?
There was a problem hiding this comment.
Looking more carefully maybe you are not using a formatter? There are small random spaces in a few spots here and there?
There was a problem hiding this comment.
I tried reducing my changes to the minimum but at this point no formatter can replicate the style that was there before. Let me know if you can review the code in this format. Basically at this point besides the code formatting the only other change is the enforceCA flag that forces chip authentication to prevent replay attacks. Though I would strongly encourage the use of a formatter.
There was a problem hiding this comment.
If you are using xcode you can set up auto format on save. It's not the most straightforward process but better than nothing
https://medium.com/@jozott/format-on-save-xcode-swift-8133d049b3ac
or set up some git hooks to run auto format on commit
ospfranco
changed the title
ospfranco
force-pushed
the
contributions
branch
3 times, most recently
from
9f5f610 to
e2b1241
Compare
ospfranco
force-pushed
the
contributions
branch
from
fdf5d6a to
5a5d844
Compare
Update package Update readme Update to latest OpenSSL version, accept aa challenge and change name Fix missing passing aaChallenge Clean up code, correct typo and move logger calls to PassportReader.swift Remove DS_Store restore podspec name Add option to enforce CA check Revive cocoapods app Revert team change and other xcode changes Try to match previous formatting Reset unrelated files Restore readme Try different formatting format format format Fix typos Get rid of duplicated property One more try at undo formatting Restore old formatting Typos chip authentication
ospfranco
force-pushed
the
contributions
branch
from
ce1b58d to
312fe76
Compare
| } | ||
|
|
||
| func tryChipAuthentication(tagReader : TagReader, DGsToRead : inout [DataGroupId]) async throws { | ||
| if !DGsToRead.contains( .DG14 ) { |
There was a problem hiding this comment.
Can you explain the changes here and why you think they are needed at the framework level rather than the app level?
Currently, if DG14 was requested to be explicitly read (passed in on the requested list), we just remove it from our list of ones to read as we will read it explicitly (for CA).
I don't think it should be a cause of failure if its been explicity requested.
Also, if CA is not supported, then again it I'm not sure is should be a failure - maybe a different status flag would be required, but I still think we should read the passport if we can and then the application can decide what to do later - open to thoughts here but I'm leaning more on the application make those decisions that the framework as that would be very application specific.
There was a problem hiding this comment.
Though question. I guess I see NFCPassportReader more of a library that abstracts all the details, in comparison with JMRTD which only provides the low level functionality to write the passport reading oneself. NFCPassportReader provides some low level mechanisms (e.g. DG groups to read) in combination with some higher level abstractions (skipCA) so the line is hard to draw. The current logic and states are complex and it just seamed cleaner to me, instead of having multiple states and hidden control paths at the end of the day any consumer of the library should(?) just care about the main security feature: is the chip authenticated yes/no
It also gets confusing with the skipCA flag, because the default value is false, but there is no failure if the CA was not a success. It's a quirky API that unless one reads the source code doesn't clearly communicate what boolean flags do (boolean flags are always kinda bad in that way) but didn't want to deviate to much from the existing codebase, just provide a consistent state that can be guaranteed after the readPassport call has been made.
Another quirk of the API is specifying a non-existent group. E.g. I tried adding DG15 to the requested groups but was getting no data. It was only after attaching the debugger and stepping line by line, that I realized the passport I'm testing with has no DG15... in this case the return is opaque to the calling context, but I might not have checked all the fields in the response, so I could be in the wrong here.
There was a problem hiding this comment.
I just checked the returned model and indeed I did not see the chipAuthenticationStatus property. So indeed this logic can be moved to the application level. I guess there is no real strong technical argument, just for the sake of clarity and trying to avoid too many hidden paths I would rather reduce the states of CA to .success/.failure. Whatever the reason it was performed or not, I as a consumer of the library with strong security requirements I just need to know that chip authentication passed.
Feel free to dismiss the PR if you don't like this final change. The other stuff we required is working and I can move this logic to the application level.
2 participants
Uh oh!
There was an error while loading. Please reload this page.