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 basic config mechanism for fido2 #337

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Add basic config mechanism for fido2 #337

merged 1 commit into from
Oct 2, 2023

Conversation

robin-nitrokey
Copy link
Member

@robin-nitrokey robin-nitrokey commented Sep 12, 2023

This patch adds a basic config mechanism using the admin app. The configuration is loaded and applied during initialization. The admin app now has a special role, is constructed first and can no longer be disabled. Other applications can receive a reference to the configuration loaded by the admin app.

To avoid overwriting configuration values for apps that are not enabled in the current firmware, the components of the Config struct are not feature-gated. It could be simplified by using a derive macro but that seems overcomplicated for a first implementation.

The first use case for the config mechanism is enabling or disabling skipping the FIDO2 user presence check directly after boot.

To do

  • Add test for serialized config size

@robin-nitrokey robin-nitrokey force-pushed the config branch 2 times, most recently from 6c60d09 to 7b45985 Compare September 12, 2023 13:54
@daringer daringer linked an issue Sep 19, 2023 that may be closed by this pull request
@robin-nitrokey
Copy link
Member Author

One open question is how to deal with default values and what should happen if we decide to change them in future firmware versions. We could skip serializing the application config if it is equal to the default. This would mean that a firmware update with changed default settings would change the existing configuration.

@daringer
Copy link
Collaborator

Generally we should not alter configuration values if they've been set by the user. So skipping serialization if it's the default is ok, once the user changes it, it would automatically persist, right?

@robin-nitrokey
Copy link
Member Author

I was thinking about cases where the user deliberately sets the default config. We could add an additional field to indicate manually written configuration, but that would increase the serialized size.

@daringer
Copy link
Collaborator

Ultimately, this is a (client) UI issue. Means I would vote to keep the firmware implementation as simple as possible (as you suggested). Our client software (pynitrokey / nitrokey-app2) should on top then make sure that the user is always well informed. For nkapp2 this means the configuration has to be shown after updating and pynitrokey should do something similar after updating.

@robin-nitrokey
Copy link
Member Author

Changes since the initial version:

  • Don’t write default values.
  • Shorten field names to reduce config size.
  • Test that the maximum configuration is smaller than 1/4 IFS block size.
  • Change enable_skip_up_timeout to disable_skip_up_timeout: enable would make more sense, but using disable allows us to have false as the default value, i. e. this field can be skipped during serialization.

@robin-nitrokey robin-nitrokey marked this pull request as ready for review September 27, 2023 13:27
Copy link
Collaborator

@sosthene-nitrokey sosthene-nitrokey left a comment

Choose a reason for hiding this comment

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

LGTM

@sosthene-nitrokey
Copy link
Collaborator

Can we merge this? That way I can rebase #335 on top of it.

Since the admin-app changes have already been merged this makes it easier to update #335

This patch adds a basic config mechanism using the admin app.  The
configuration is loaded and applied during initialization.  The admin
app now has a special role, is constructed first and can no longer be
disabled.  Other applications can receive a reference to the
configuration loaded by the admin app.

To avoid overwriting configuration values for apps that are not enabled
in the current firmware, the components of the Config struct are not
feature-gated.  It could be simplified by using a derive macro but that
seems overcomplicated for a first implementation.

The first use case for the config mechanism is enabling or disabling
skipping the FIDO2 user presence check directly after boot.

Fixes: #344
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.

Add init-time configuration options
3 participants