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

Update hotp-verification (fixes #746) #748

Merged
merged 3 commits into from
Jul 23, 2020

Conversation

alex-nitrokey
Copy link
Contributor

@alex-nitrokey alex-nitrokey commented Jun 10, 2020

  • use newer version of hotp-verification module to avoid patch
  • testing new version on board

What do you think, shall I make two separate PRs instead?

(will fix #746)

@alex-nitrokey alex-nitrokey changed the title WIP: Update hotp-verification WIP: Update hotp-verification (fixes #746) Jun 10, 2020
@MrChromebox
Copy link
Contributor

I'm a fan of separate PRs

@techge
Copy link
Contributor

techge commented Jun 20, 2020

Tested and working.

Does it? It had some problems with it during initializing HOTP (hence still WIP).

@tlaurion
Copy link
Collaborator

tlaurion commented Jun 20, 2020

@techge @AUX234 : This does: #756

@MrChromebox
Copy link
Contributor

@tlaurion I'll need to retest to be sure, but IIRC this failed verification when I tested on a Librem13v4

@tlaurion
Copy link
Collaborator

#756 is just CBFS fix for x230-hotp-verification board which was reported not boot

@alex-nitrokey
Copy link
Contributor Author

It boots and builds indeed, but for me it is not able to do the HOTP verification. Doesn't you had the problem?

Steps to reproduce:

  • generate new HOTP secrets
  • succeeds and key blinks green
  • after that, every check let key blink red and invalid code message appears

@tlaurion
Copy link
Collaborator

Still not working.

@alex-nitrokey
Copy link
Contributor Author

Still not working.

Just to make sure: you mean, you are experiencing the same, right?

@alex-nitrokey
Copy link
Contributor Author

I found the issue as described here. It is very embarrasing, so let's not talk about it.

As soon as it is merged into master of hotp-verification I will update the version and hash here and thus the PR is ready to merge then.

@szszszsz
Copy link
Contributor

@alex-nitrokey Just merged

@alex-nitrokey alex-nitrokey changed the title WIP: Update hotp-verification (fixes #746) Update hotp-verification (fixes #746) Jun 24, 2020
@alex-nitrokey
Copy link
Contributor Author

Builds and runs as intended, initialization and verification works fine.

@tlaurion
Copy link
Collaborator

#756 was merged meanwhile to fix #737

@tlaurion
Copy link
Collaborator

tlaurion commented Jun 28, 2020

@tlaurion tlaurion closed this Jun 28, 2020
@tlaurion tlaurion reopened this Jun 28, 2020
@tlaurion
Copy link
Collaborator

Sorry closed by error and reopened.

@MrChromebox
Copy link
Contributor

looks like we just need to update libremkey-hotp-verification_version to d4adfc58e53e61519adafc3dce2ddf729e7ebc86 (or later; master at 03a198c418a60c54ef3ec67ea8a9a2d29b675b9b currently) and adjust hash accordingly

@MrChromebox
Copy link
Contributor

@stmc-droid see my comment above, this PR needs updating to fix the issue

@alex-nitrokey
Copy link
Contributor Author

looks like we just need to update libremkey-hotp-verification_version to d4adfc58e53e61519adafc3dce2ddf729e7ebc86 (or later; master at 03a198c418a60c54ef3ec67ea8a9a2d29b675b9b currently) and adjust hash accordingly

I thought I updated this PR as well, but apparently I did it only in #761 hence the confusion, sorry.

@alex-nitrokey
Copy link
Contributor Author

38ba257 is the corresponding commit which I just forgot to push last week.

@MrChromebox
Copy link
Contributor

pushed #774 since this seems to have stalled, and wanted something working we can merge to get master back to functional ASAP

@techge
Copy link
Contributor

techge commented Jul 8, 2020

Commit 38ba257 that is already in this PR should work and uses newer version of hotp-verification... Thus, I do not see the need for #744.

In the end I would suggest to merge #761 instead of the other two PRs anyway (they should not conflict anyway)

@MrChromebox
Copy link
Contributor

MrChromebox commented Jul 8, 2020

if #761 is ready then correct no need, but the commit/PR message lists two items under 'things still needed' so my understanding was that it was not ready, and why I pushed a separate PR to fix the issue independently.

@techge
Copy link
Contributor

techge commented Jul 8, 2020

but the commit/PR message lists two items under 'things still needed'

Mh, that is strange, aren't they ticked? I ticked them last week or so and they are ticked for me...

@MrChromebox
Copy link
Contributor

they show as grey checkboxes for me, which contrasts with the green checkmarks used elsewhere on github so it certainly appears they are not completed

@szszszsz
Copy link
Contributor

szszszsz commented Jul 8, 2020

#748 (comment) is ticked both for me as well. Maybe you mean different one?

Edit: for the green check marks you need UTF-8 glyph AFAIK

@MrChromebox
Copy link
Contributor

if that's ticked, then it's just horrible design

@techge
Copy link
Contributor

techge commented Jul 8, 2020

if that's ticked, then it's just horrible design

Then it is :D

@tlaurion
Copy link
Collaborator

tlaurion commented Jul 8, 2020

Checked
?!?

@MrChromebox
Copy link
Contributor

Screenshot from 2020-07-08 20-12-51

guess it's a Chrome(ium)/Linux issue?

@techge
Copy link
Contributor

techge commented Jul 9, 2020

Well, at least there are the marks 😂

@tlaurion tlaurion merged commit 73c9d6e into linuxboot:master Jul 23, 2020
@daringer daringer deleted the hotp-verification-update branch November 15, 2023 12:37
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.

Upgrade nitrokey-hotp-verification module
5 participants