Skip to content
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

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

Open
wants to merge 4 commits into
base: gnuk1.2-regnual-fix
Choose a base branch
from

Conversation

alex-nitrokey
Copy link
Contributor

No description provided.

@szszszsz
Copy link
Member

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

@alex-nitrokey
Copy link
Contributor Author

From 1.2.6 to RTM.6

@alex-nitrokey
Copy link
Contributor Author

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

@szszszsz
Copy link
Member

szszszsz commented Feb 25, 2019

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

jans23 commented Feb 27, 2019 via email

@alex-nitrokey
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

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

@szszszsz
Copy link
Member

Friendly ping

@alex-nitrokey
Copy link
Contributor Author

alex-nitrokey commented Apr 24, 2019

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
Copy link
Member

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
Copy link
Member

szszszsz commented Apr 25, 2019

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 added a commit that referenced this pull request Apr 25, 2019
In case the wrong PIN is supplied, and the factory-reset is set to no, this would lock the device entirely.
Temporary fix for #12, while waiting for #13.

Signed-off-by: Szczepan Zalega <[email protected]>
@szszszsz
Copy link
Member

Update: I have added a temporary quick-fix for the original issue (as in #13 (comment) #12 (comment)): c66c238.

@alex-nitrokey
Copy link
Contributor Author

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.

@szszszsz szszszsz added this to the RTM.8 milestone Apr 30, 2020
@szszszsz szszszsz self-assigned this Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants