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

Set reasonable maximum numbers for persistent data #207

Closed
daringer opened this issue Mar 21, 2023 · 4 comments
Closed

Set reasonable maximum numbers for persistent data #207

daringer opened this issue Mar 21, 2023 · 4 comments

Comments

@daringer
Copy link
Collaborator

In particular + my suggestions:

  • maximum number of FIDO2 RKs (20)
  • maximum number of secrets-app entries (100)

These are very conservative entries, which shall be higher in the future once we can promise better performance for e.g., secrets-app entries.

Best would be that these are passed as options into the app(s) instead of hard-coding them inside the apps - so later we can configure them through the admin-app

@robin-nitrokey
Copy link
Member

For FIDO2, there are two approaches for implementing this:

  1. Checking the number of existing resident keys when creating a new one. Downside: This is potentially a slow operation if there are many keys.
  2. Keeping track of the number of resident keys in the state. Downside: Requires initial setup for old states, more error-prone because it is easier to miss an execution path, additional writes on the filesystem, can be problematic if e. g. deleting a key is successful but writing the state fails.

So to keep things simple, I’d prefer 1.

@szszszsz
Copy link
Member

Why is there a limit on FIDO RKs? Is it due to listing performance?

@robin-nitrokey
Copy link
Member

Yes. Currently, there is no limit, but we wanted to add one to make sure that the key does not become unusable due to the listing issue.

robin-nitrokey added a commit that referenced this issue Apr 20, 2023
This patch introduces a limit of ten resident credentials, see:
	#207
robin-nitrokey added a commit that referenced this issue Apr 20, 2023
This patch introduces a limit of ten resident credentials, see:
	#207
@robin-nitrokey robin-nitrokey removed their assignment Apr 20, 2023
@robin-nitrokey
Copy link
Member

Implemented for fido-authenticator in Nitrokey/fido-authenticator#12 and #239.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants