Menu revamp

[mikelevenick]

[mikelevenick]

Dope changes, but I'm getting weird behavior with the keychain. Once I cleared the keychain via the settings menu it would no longer allow writing the password with this error:

2023-01-05 12:15:48 [ERROR ]: Error writing credentials to keychain. unhandledError(status: -25299)

It's also confusing having both the remember me and settings options controlling what is saved. The settings options also look to be off by default, while remember me is on by default. It would be cool if those settings used the same source or were more intuitive.

Interesting, I've not seen an error like that on the keychain. Are you able to consistently recreate it? What steps exactly take you there?

As for the remember me vs the options, the "remember me" is specifically saving Username, Password, and Server into Keychain; the stuff in the Menu is to store the other options into Defaults if you don't use Keychain stuff.

I agree it's a bit confusing, and would love suggestions on how to clarify it. I'd like to keep the options to save username/server for those who don't want to store their password in keychain, but not sure how to make it more clear what's happening without using techy details that end-users don't care about in the UI.

[apirkl]

Dope changes, but I'm getting weird behavior with the keychain. Once I cleared the keychain via the settings menu it would no longer allow writing the password with this error:

2023-01-05 12:15:48 [ERROR ]: Error writing credentials to keychain. unhandledError(status: -25299)

It's also confusing having both the remember me and settings options controlling what is saved. The settings options also look to be off by default, while remember me is on by default. It would be cool if those settings used the same source or were more intuitive.

I looked into this further and using https://developer.apple.com/documentation/security/1394686-seccopyerrormessagestring the error is the specified item already exists in the keychain

So the issue is probably with the delete section and the primary keys in the query not returning all possible results that could cause a duplicate. A bit about that is here: https://stackoverflow.com/questions/11614047/what-makes-a-keychain-item-unique-in-ios and here: https://developer.apple.com/documentation/security/keychain_services/keychain_items/updating_and_deleting_keychain_items

It makes sense given that a lot of users are going to have credentials for their jamf pro instance that could collide with the pimary keys for a kSecClassInternetPassword. I'd recommend both changing the query used to find the item to delete and changing the kSecClass of the stored credentials.

However, that is not functionality added here so I'll create a bug ticket to deal with it.

For the ambiguity between keychain and defaults I think to work on it the first step would be to rework the storage mechanism to handle both keychain and defaults storage, then make some new logic. As it's written now it's hard to disentangle them, so that's probably a task for another time.

So nothing to change here it looks like, I'll re-review shortly

[mikelevenick]

Dope changes, but I'm getting weird behavior with the keychain. Once I cleared the keychain via the settings menu it would no longer allow writing the password with this error:

2023-01-05 12:15:48 [ERROR ]: Error writing credentials to keychain. unhandledError(status: -25299)

It's also confusing having both the remember me and settings options controlling what is saved. The settings options also look to be off by default, while remember me is on by default. It would be cool if those settings used the same source or were more intuitive.

I looked into this further and using https://developer.apple.com/documentation/security/1394686-seccopyerrormessagestring the error is the specified item already exists in the keychain

So the issue is probably with the delete section and the primary keys in the query not returning all possible results that could cause a duplicate. A bit about that is here: https://stackoverflow.com/questions/11614047/what-makes-a-keychain-item-unique-in-ios and here: https://developer.apple.com/documentation/security/keychain_services/keychain_items/updating_and_deleting_keychain_items

It makes sense given that a lot of users are going to have credentials for their jamf pro instance that could collide with the pimary keys for a kSecClassInternetPassword. I'd recommend both changing the query used to find the item to delete and changing the kSecClass of the stored credentials.

However, that is not functionality added here so I'll create a bug ticket to deal with it.

For the ambiguity between keychain and defaults I think to work on it the first step would be to rework the storage mechanism to handle both keychain and defaults storage, then make some new logic. As it's written now it's hard to disentangle them, so that's probably a task for another time.

So nothing to change here it looks like, I'll re-review shortly

Interesting. That shouldn't be happening. The queries that are being used are looking for kSecAttrLabel as String: KeyVars.key Which should be defined static let key = "com.jamf.mut.credentials"

So it should not ever be matching multiple keys, unless somehow there are two keys in the keychain with com.jamf.mut.credentials as their label.

If you open your keychain and search for 'mut' do you see multiple versions in there at all?

[apirkl]

Interesting. That shouldn't be happening. The queries that are being used are looking for kSecAttrLabel as String: KeyVars.key Which should be defined static let key = "com.jamf.mut.credentials"

So it should not ever be matching multiple keys, unless somehow there are two keys in the keychain with com.jamf.mut.credentials as their label.

If you open your keychain and search for 'mut' do you see multiple versions in there at all?

We talked about this on the side, but to record it here the kSecAttrLabel is not a primary key for a kSecClassInternetPassword, so there can be other keychain entries of the type kSecClassInternetPassword that collide with the primary keys being set. A query that encompasses just the primary keys or a different password type would fix this.

[mikelevenick]

Hey @apirkl any chance the changes to the keychain helper resolved the issues you were seeing?