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

Support access control options #178

Merged

Conversation

Mikescops
Copy link
Contributor

@Mikescops Mikescops commented Mar 6, 2023

Hello,

I'm trying to implement the support of the Keychain Item Accessibility (see Apple documentation here).

The goal is for instance to ask for user presence (faceId, touchId) when requesting a password from the keychain. This options are widely supported on ios/macos/tvos/... so I thought it makes sense to include it in this library.

Disclaimer: I'm not familiar with Rust at all, so I did my best to get into it, here is a draft so let me know if I'm heading in the right direction and any guidance/help is welcomed!

I didn't added tests yet, didn't want to spend too much time if I'm already not doing things right

@kornelski
Copy link
Owner

Thanks for the PR.

I'm happy to add more of the security framework functionality, but I'd prefer not to make breaking changes to existing Rust APIs. If you need to modify existing tests to compile the code, that's a breaking change.

security-framework-sys/src/access_control.rs Show resolved Hide resolved
security-framework-sys/src/base.rs Outdated Show resolved Hide resolved
security-framework/src/passwords.rs Outdated Show resolved Hide resolved
security-framework/src/passwords.rs Outdated Show resolved Hide resolved
@Mikescops
Copy link
Contributor Author

If you need to modify existing tests to compile the code, that's a breaking change.

Totally agree, thanks for the tips!

@Mikescops
Copy link
Contributor Author

@kornelski I tried to make your suggestions, I think I got something decent

It's not touching the function signatures, and the tests are passing, i restructured the password options (hope it's what you wanted).

Now I'm not sure exactly how to expose my new set_access_control_options method to the outside, looking for your advice

@Mikescops Mikescops requested a review from kornelski March 9, 2023 16:40
@dmolokanov
Copy link

@Mikescops / @kornelski Do you guys have plans to merge this PR.

@Mikescops
Copy link
Contributor Author

@dmolokanov I'd love to do so yes, but i need feedback first

@kornelski kornelski force-pushed the main branch 3 times, most recently from 9e76127 to 85421c8 Compare May 11, 2023 18:42
@kornelski kornelski force-pushed the feature/add-access-control-options branch from 601e14d to 1aa4832 Compare May 11, 2023 18:51
@kornelski kornelski merged commit 6efe5a9 into kornelski:main May 12, 2023
@kornelski
Copy link
Owner

Looks good, thank you.

@gibfahn
Copy link

gibfahn commented Nov 23, 2023

The goal is for instance to ask for user presence (faceId, touchId) when requesting a password from the keychain. This options are widely supported on ios/macos/tvos/... so I thought it makes sense to include it in this library.

Came here looking for the same thing, biometric support for keychain passwords.

Seems that this PR added PasswordOptions, but I can't find a way to actually plumb them in to the generic password creation API.

use security_framework::passwords_options::{AccessControlOptions, PasswordOptions};

fn main() {
    let access_control_options = AccessControlOptions::USER_PRESENCE;
    let mut entry = PasswordOptions::new_generic_password("my_service3", "test1");
    entry.set_access_control_options(access_control_options);
    // How to actually use this?
}

This example also errors with:

called Result::unwrap() on an Err value: Error { code: -50, message: "One or more parameters passed to a function were not valid." }

which comes from security-framework-2.9.2/src/passwords_options.rs:124:13

@Mikescops Mikescops changed the title Draft: Support access control options Support access control options Nov 23, 2023
ancwrd1 pushed a commit to ancwrd1/rust-security-framework that referenced this pull request Sep 14, 2024
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.

4 participants