search

Nitrokey / nitrokey-start-firmware

A mirror of Gnuk's 1.0.x and 1.2.x branches.
56 stars 15 forks source link

Add error detection for authentication (fixes #12) #13

Open alex-nitrokey opened 5 years ago

szszszsz commented 5 years ago

Looks good! On which firmware version have you tested it?

alex-nitrokey commented 5 years ago

From 1.2.6 to RTM.6

alex-nitrokey commented 5 years ago

I can not imagine any difference in other firmware versions. However, do you think I should test for others (which one)?

szszszsz commented 5 years ago

No, this should be sufficient. Please cross-compare with latest GNUK: https://salsa.debian.org/gnuk-team/gnuk/gnuk/blob/master/tool/upgrade_by_passwd.py Edit: and if the patch is applyable there as well (in Git understanding).

alex-nitrokey commented 5 years ago

Please cross-compare with latest GNUK: https://salsa.debian.org/gnuk-team/gnuk/gnuk/blob/master/tool/upgrade_by_passwd.py Edit: and if the patch is applyable there as well (in Git understanding).

Sorry, I do not understand, what exactly you mean by cross-compare. Should I merge? Or just have a look if there are changes which contradict?

szszszsz commented 5 years ago

Sorry for confusion. Yes, I meant compare the both files in case we have different version, and try to apply patch on the current GNUK's master, if they are different.

alex-nitrokey commented 5 years ago

The only differences are those you and I did for the last 8 months. See blame or history for an overview. I checked it locally with the upstream file.

I try to get all of the changes upstream then?

jans23 commented 5 years ago

Yes, please try to get changes merged in upstream. Thanks.

alex-nitrokey commented 5 years ago

@szszszsz is there a reason that some of your commits from 04 July 2018 are included upstream already and some aren't? Was there a discussion about it?

szszszsz commented 5 years ago

@alex-nitrokey Yes, AFAIR another solution was suggested for one of the issues back then, hence the partial merge. It should be archived and available online. If not, please drop me an email if you are interested.

alex-nitrokey commented 5 years ago

I found it. I try to refactor the code and address the concerns.

szszszsz commented 5 years ago

Friendly ping

alex-nitrokey commented 5 years ago

You and NIIBE talked about the fact that it would be nice to have something like a 'gnuk-tool' which handles all the tasks which are now separated in some tools. As this is a more tedious task than just fixing this issue, I would suggest to make just the upgrade script right. The minor task regarding the upgrade script were about how imports are handled and stuff like this.

IMHO I can not do this properly without cleaning up the code a bit. On the other hand this might not get included upstream, because of to heavy changes. So... what do you think? I already began changing the code and was only then wondering if this is a good idea...

szszszsz commented 5 years ago

Yes, would be nice to develop a general gnuk-tool for such task. However first the current code needs to work correctly.

On the other hand this might not get included upstream, because of the changes.

Just do a small commits, with well described intents in the message, to ease the review. We can diverge for a while, and perhaps just replace later the original GNUK scripts with ours. I think NIIBE has meant these tools to just demonstrate and test use cases, and that was not meant for the not advanced user, hence the lack of UI care. I encourage to adjust it and make it as much user friendly as possible. Later let's create the gnuk-tool package.

szszszsz commented 5 years ago

Please focus first on releasing asap the update script changes, which would prevent locking the Start device on giving the wrong PIN, while it has disabled factory-reset (edit).

szszszsz commented 5 years ago

Update: I have added a temporary quick-fix for the original issue (as in https://github.com/Nitrokey/nitrokey-start-firmware/pull/13#issuecomment-486614315 https://github.com/Nitrokey/nitrokey-start-firmware/issues/12#issue-413682342): https://github.com/Nitrokey/nitrokey-start-firmware/commit/c66c2386269f2de4972478e23e58646d99752e00.

alex-nitrokey commented 5 years ago

Please focus first on releasing asap the update script changes, which would prevent locking the Start device on giving the wrong PIN, while it has disabled factory-reset (edit).

This is what the very first commit already was capable of. It exits if wrong pin is provided.