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.

Sign up for GitHub

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

Jump to bottom

Use trussed-auth #125

Merged
merged 13 commits into from
Apr 4, 2023
Merged

Use trussed-auth #125

merged 13 commits into from
Apr 4, 2023

Conversation

sosthene-nitrokey
Copy link
Collaborator

No description provided.

@sosthene-nitrokey sosthene-nitrokey changed the title Trussed auth Use trussed-auth Mar 3, 2023
@sosthene-nitrokey sosthene-nitrokey mentioned this pull request Mar 20, 2023
Closed
@daringer daringer linked an issue Mar 23, 2023 that may be closed by this pull request
Use Trussed-Auth for pin handling and encryption #128
Closed
@daringer daringer removed a link to an issue Mar 23, 2023
Use Trussed-Auth for pin handling and encryption #128
Closed
@daringer daringer linked an issue Mar 23, 2023 that may be closed by this pull request
Use Trussed-Auth for pin handling and encryption #128
Closed
robin-nitrokey
robin-nitrokey reviewed Mar 27, 2023
View reviewed changes
Copy link
Member

@robin-nitrokey robin-nitrokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. But as far as I see, init_pins is only called if there is no valid state. Couldn’t this lead to problems when upgrading from an older version?

Cargo.toml Show resolved Hide resolved
src/state.rs Outdated Show resolved Hide resolved
@sosthene-nitrokey sosthene-nitrokey mentioned this pull request Mar 30, 2023
Closed
@sosthene-nitrokey
Return 0 remaining tries when pin is not set
78665c2 
 If try_syscall is Err, the PIN is not set for some reason, so it cannot be verified.
@sosthene-nitrokey
Copy link
Collaborator Author

The changes look good to me. But as far as I see, init_pins is only called if there is no valid state. Couldn’t this lead to problems when upgrading from an older version?

I added pin deletion to the factory reset. What do you see that would go wrong?

@robin-nitrokey
Copy link
Member

I think the path I saw was: Suppose you initialized the state with an old opcard version and stored the PINs in the FS. Now you update to this version. There are no PINs in trussed-auth, but there is a valid state. Factory reset is only allowed if a) verified as admin or b) if the admin PIN is locked. a) is not possible without a PIN, and b) was not possible because the retries would default to 3 if the PIN is not set. This should be fixed now.

@robin-nitrokey
Copy link
Member

I added pin deletion to the factory reset.

Shouldn’t it reset the PINs to the defaults?

@sosthene-nitrokey
Copy link
Collaborator Author

On factory reset, the state and pins are deleted.

On state load, if no state is present the pins are set to their default values.

@sosthene-nitrokey sosthene-nitrokey merged commit 69dc03a into main Apr 4, 2023
@sosthene-nitrokey sosthene-nitrokey deleted the trussed-auth branch April 4, 2023 15:46
@sosthene-nitrokey sosthene-nitrokey mentioned this pull request Apr 17, 2023
Closed
@robin-nitrokey robin-nitrokey mentioned this pull request Apr 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robin-nitrokey robin-nitrokey robin-nitrokey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use Trussed-Auth for pin handling and encryption
2 participants
@sosthene-nitrokey @robin-nitrokey