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

update osx apis #119

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

update osx apis #119

wants to merge 3 commits into from

Conversation

gattjoe
Copy link

@gattjoe gattjoe commented Jan 9, 2023

Hi @rayluo,

I took a stab at updating the macOS Security APIs so that they are no longer using the recently deprecated ones. Once this library is updated, I am hoping the Azure CLI will turn on encryption by default (or as an opt-in). I'm happy to amend this change as necessary. Looking forward to your feedback. cc @jiasli

Joe

@gattjoe
Copy link
Author

gattjoe commented Apr 16, 2024

@rayluo @bgavrilMS @jiasli I see that a new version of MSAL EX was released. Can someone put eyes on this PR?

@bgavrilMS bgavrilMS requested a review from ashok672 April 17, 2024 16:18
@bgavrilMS
Copy link
Member

Hi @gattjoe - @rayluo and me no longer own public client scenarios. Adding @ashok672 for guidance.

May I ask what's wrong with the deprecated APIs? Moving to new APIs would require a lot of testing.

@gattjoe
Copy link
Author

gattjoe commented Apr 17, 2024

@bgavrilMS @ashok672

The main background is this PR in the Azure Cli, where it was discussed using the encrypted persistence cache, but that the Apple APIs are legacy. There was an assertion that the way the Azure Cli was packaged would preclude us from using the newest keychain API, which is not the case.

The main source of Apple keychain API information I used is here: https://developer.apple.com/documentation/technotes/tn3137-on-mac-keychains, where it basically says to use the SecItem keychain API. You can sere here and here how Apple has labeled the keychain APIs that MSAL EX is currently using as deprecated.

There is no difference in behavior with using the new API vs. the old. In reviewing some of your conversations with your peers (on GitHub), I can tell that maintaining compatibility is important for MSAL and by extension MSAL EX. As a result, I spent time to make sure the inputs/outputs of the functions are the same.

I have not spoken to anyone on the Azure Cli team. In the PR referenced above, it seems they are aware that the keychain API is old. I am hoping that if we can update the keychain API with this change, I can perhaps influence them to turn on encryption support with a command line flag before making it the default.

Any feedback is appreciated.

EDIT: I didn't mean to ignore statement about the amount of testing required. Perhaps one thing we could do is make it a feature flag on what version to use (old version first, opt-in to new).

@bgavrilMS
Copy link
Member

Yes, we know the API is deprecated and Apple doesn't offer support for it. But it works and there is no indication that Apple is removing this support anytime soon.

We tried to move to the SecItem API on the .NET equivalent project, only to find out that it requires a bundle id. So the new APIs are more for apps which are notarized / distributed through the Apple Store. This is not the case of Azure CLI. I may be wrong.

@gattjoe
Copy link
Author

gattjoe commented Apr 18, 2024

We wouldn't need a bundle id in this context as we are just using the Python bindings to access the APIs. Maybe you can help me convince @jiasli to turn on opt in support for encrypting the persistence cache in OS X.

@DanielOverdevestPZH
Copy link

@jiasli extra security on OSX would be nice

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.

3 participants