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 keychain synchronisation and authentication context #221

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

crashdump
Copy link
Contributor

Added the ability to pass an LAContext to the keychain key operations as well as the kSecAttrSynchronizable flag during the creation of keys.

@crashdump crashdump force-pushed the use-authentication-context branch from 6f33289 to c3630bb Compare January 6, 2025 14:31
@kornelski
Copy link
Owner

kornelski commented Jan 6, 2025

Please check the build failures on iOS.

It's unfortunate that GenerateKeyOptions has exhaustive public fields. This will need some workaround :/

@kornelski kornelski force-pushed the use-authentication-context branch from c3630bb to ac1909a Compare January 6, 2025 15:14
@crashdump crashdump force-pushed the use-authentication-context branch from ac1909a to dd45957 Compare January 6, 2025 16:06
@crashdump
Copy link
Contributor Author

crashdump commented Jan 6, 2025

@kornelski, I fixed the MR & tests (I had forgotten to include a cfg) but I had not realised you had pushed your own changes and overwrote your push :( -- Would you mind re-applying?

Thoughts welcome on GenerateKeyOptions, it's indeed unfortunate!

@kornelski kornelski force-pushed the use-authentication-context branch from dd45957 to 6a650bc Compare January 7, 2025 02:14
@crashdump
Copy link
Contributor Author

crashdump commented Jan 7, 2025

Thanks @kornelski, I can see the CI / Tests macos-latest tests are also falling on the main branch, so I'll ignore that for now.

As for the GenerateKeyOptions, we can either accept this as a breaking change (albeit not a major one since it's an Option which can be set to None by default) or introduce a new builder, similar to what was suggested in MR #220. I'll take your steer.

@kornelski
Copy link
Owner

I think the cleanest solution would be to hide the new field in GenerateKeyOptions behind a feature flag. Although theoretically flags are supposed to be purely additive, this breakage is small enough that won't be a problem in practice.

@crashdump crashdump force-pushed the use-authentication-context branch from 6a650bc to 88ed949 Compare January 7, 2025 12:26
@crashdump
Copy link
Contributor Author

Alright, it's done. Let me know your thoughts.

@crashdump crashdump force-pushed the use-authentication-context branch from 88ed949 to 8864ece Compare January 7, 2025 13:21
@crashdump
Copy link
Contributor Author

I'm an idiot 🤦 - it's fixed now.

@kornelski kornelski force-pushed the use-authentication-context branch from 8864ece to 77392cf Compare January 7, 2025 15:00
@kornelski kornelski merged commit d139535 into kornelski:main Jan 7, 2025
5 of 6 checks passed
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.

2 participants