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

hold ref to temp keychain on OSX to avoid premature cleanup #41787

Merged
merged 2 commits into from
Sep 4, 2020

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Sep 3, 2020

This is fix for issue originally reported on macOS 11.0 when we fail to add certificates to a store.
The root cause if fact that when importing, we put certificate and key to temporary keyChain and that can be deleted before we use the bits. This used to work with 3.1 but it is failing now on Catalina+.

The fix is to grab reference when temporary keychain is used to make sure keychain file stays around.
Since the chain can now linger longer, all the changes in CreateTemporaryKeychain are to ensure we have better one time encryption password. Technically, that is not needed to avoid the issue.

As far as testing, this is little bit tricky. We already have tests that would expose the behavior.
However, the X509StoreMutableTests are guarded by PermissionsAllowStoreWrite.
With the bug adding to store fails and the tests therefore do not run.
I think improving detection of SSH with combination of kPOSIXErrorBase may be long term fix.
Alternatively, we can create new Keychain for the test so it is always writable independently of the the default one.
For now, I verified that the tests run again when keychain is unlocked and the repro code runs on 10.15 & 11.0

fixes #39603

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@wfurt wfurt self-assigned this Sep 3, 2020
@ghost
Copy link

ghost commented Sep 3, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
See info in area-owners.md if you want to be subscribed.

@vcsjones
Copy link
Member

vcsjones commented Sep 3, 2020

Did you confirm that the temp keychain files are getting cleaned up when the cert is disposed, or is that tested somewhere?

@wfurt
Copy link
Member Author

wfurt commented Sep 3, 2020

yes, I did. I modified you repro and added ReadLine after the import and added dispose followed by another ReadLine.
And the file is gone after dispose. I did not check any more complicated case when somebody would for example get and ref certificate handle or do some other tricks.

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

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

Will let somebody more familiar with Apple Crypto sign off. But the modified logic in CreateTemporaryKeychain looks good to me!

@wfurt wfurt merged commit 5463301 into dotnet:master Sep 4, 2020
@wfurt
Copy link
Member Author

wfurt commented Sep 4, 2020

/backport to release/5.0-rc2

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2020

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/239878923

@jeffhandley jeffhandley changed the title hold ref to temp keychain on OSX to avoild premature cleanup hold ref to temp keychain on OSX to avoid premature cleanup Sep 4, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macOS: Cannot add to X509Store [regression from 3.1]
5 participants