-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend FIDO2 BLE support also for Linux #700
base: main
Are you sure you want to change the base?
Conversation
2790ef6
to
be35428
Compare
Thank you for your contribution! Before having a more in-depth look, may I ask what authenticator you are testing this against? |
This was tested with: eSecu FIDO2 pro (passkey printed on device, 20 byte control point size, fragmentation often needed, bluetooth turned off after a short time |
about the ci-failures:
How to debug this? sd_bus_default_system just takes a pointer to a variable on the stack to write some output into this, so nothing scary. Can this be resolved to source code lines? |
MemorySanitizer requires the whole application to be instrumented -- libsystemd included. We'd likely need to mock a lot of these calls to sensibly fuzz your implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is starting to take shape!
I have left a set of comments which mostly aim to get this PR into a shape that is consistent with our existing code base. Note that some of the comments apply to more than one occurrence. I'd like to generally mention:
- We try to follow a basic KNF/style(9) format for our code. While not critical, I left a couple of nitpicking comments on this.
- Some of the functions have conditionals/loops that nest very deeply, we'd prefer if we could split some of the logic out to separate functions to alleviate this.
- We'd like for most return values to be checked and bubbled up to the calling function. Where it makes sense, we also like to log a debug message indicating what went wrong.
Once again, I'd like to thank you for your work so far!
seems like a problem in alpine + libelogind in the ci run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates. I've added a set of new comments, and also left a set of (potentially?) unresolved comments from the previous round.
7e34f64
to
35baeda
Compare
For Windows it was already added via gh#336, so let's also add it for Linux. Unpaired devices are ignored, the user has to pair independently of libfido use using the bluetooth manager provided by the desktop environment.
opened an issue for the alpine trouble: By adding the include path, the alpine build got broken. Don't ask me questions... |
A summary of the alpine problem: So what can we do now:
|
I'm very interested in that functionality and would be willing to help fixing the remaining issues. The alpine issue referenced above should be fixed in the current stable (as of the new release of alpine v.3.19 released last December 7, 2023). As the pipeline refers to latest (
Unfortunately, I can't inspect the fuzzing issues, as the log expired. |
For Windows it was already added via gh#336,
so let's also add it for Linux.
Unpaired devices are ignored, the user has to pair independently of libfido use using the bluetooth manager provided by the desktop environment.