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

Recommend that data should be no longer than REASONABLE_SECRET_DATA_SIZE #248

Merged
merged 2 commits into from
Sep 21, 2020

Conversation

dfed
Copy link
Collaborator

@dfed dfed commented Sep 14, 2020

See this file for the constant. Follow up to #246 and #247.

Given that we have the known upper desired limit of keychain secret size (thank you @KhaosT!), I think it's reasonable for us to create a new KeychainError case valueTooLarge, and check that the entered data isn't above that limit before interacting with the securityd. If and when we do that, we'd want to change this /// - Important to a /// - Precondition.

Open to someone putting out a PR that does that (or I can get to it at a future date). We'd want to do this check within Keychain's setObject(_:forKey:options:).

But for now, let's make sure our documentation is telling our customers the right thing per #246 (comment).

@@ -119,7 +119,7 @@ public final class SecureEnclaveValet: NSObject {
/// - object: A Data value to be inserted into the keychain.
/// - key: A key that can be used to retrieve the `object` from the keychain.
/// - Throws: An error of type `KeychainError`.
/// - Important: Inserting data larger than 1mb in size may sporadically fail.
/// - Important: Inserted data should be smaller than 4kb.
Copy link
Collaborator

Choose a reason for hiding this comment

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

supernit: It looks like it supports up to 4kb, rather than requiring it be smaller than that.

Suggested change
/// - Important: Inserted data should be smaller than 4kb.
/// - Important: Inserted data should be no larger than 4kb.

@NickEntin
Copy link
Collaborator

create a new KeychainError case keyTooLarge

I think the REASONABLE_SECRET_DATA_SIZE is referring to the value, not the key. Or maybe both? Might be worth adding both keyTooLarge and valueTooLarge errors?

@dfed
Copy link
Collaborator Author

dfed commented Sep 21, 2020

@NickEntin 🤦‍♂️ I def meant valueTooLarge not keyTooLarge. Good catch. I've updated the description.

Regarding key size: I haven't found any explicit key-size comparisons in the linked code. We can cross that bridge if we find it.

@dfed dfed merged commit 4286426 into master Sep 21, 2020
@dfed dfed deleted the dfed--better-documentation branch September 21, 2020 01:13
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.

None yet

2 participants