Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

carlossanlop
Copy link

@carlossanlop carlossanlop commented Nov 1, 2019

Approved API Proposal: #41662
Related PR for EventWaitHandle: #42213

Description

We don't currently have a way to create a Mutex with a given ACL in .NET Core. We can modify the ACL, but it would be more secure to have the proper ACL on the object from the start.

Customer impact

Before this change, customers had to create a Mutex, then set its ACLs. This presents a few problems:

  • Potential security hole as mutexes can be accessed between creation and modification.
  • Porting difficulties as there isn't a 1-1 API replacement
    This change addresses those problems by adding a new extension method that allows creating a Mutex and ensuring the provided ACLs are set during creation.

If time permits it, we would try to backport this change to 3.1.

@carlossanlop carlossanlop added this to the 3.1 milestone Nov 1, 2019
@carlossanlop carlossanlop self-assigned this Nov 1, 2019
@maryamariyan maryamariyan requested a review from kouvel November 1, 2019 16:38
@carlossanlop
Copy link
Author

Thanks for your review, @stephentoub . I'll be addressing the comments in this PR as we improve the one for EventHandle. I'll submit a new commit for this soon.

@maryamariyan
Copy link

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

  1. In your corefx repository clone, create patch by running git format-patch origin
  2. In your runtime repository clone, apply the patch by running git apply --directory src/corefx <path to the patch created in step 1>

@carlossanlop carlossanlop merged commit 46edc58 into dotnet:master Nov 7, 2019
@carlossanlop carlossanlop modified the milestones: 3.1, 5.0 Nov 7, 2019
@danmoseley
Copy link
Member

@carlossanlop please make a 3.1 PR with template, you can reuse some of it from #42228. Because just about the only change that could affect existing codepaths is the project file changes, I would describe the risk (to existing functionality) as very low. It's worth a try.

@danmoseley
Copy link
Member

Actually, scratch that. @ericstj pointed out that we can add this post 3.1 because it's in a separate package and does not need changes to existing netcoreapp code. So we don't need to jam this into 3.1.0.

@carlossanlop
Copy link
Author

Actually, scratch that. @ericstj pointed out that we can add this post 3.1 because it's in a separate package and does not need changes to existing netcoreapp code. So we don't need to jam this into 3.1.0.

So should I do anything additional or is this merge enough? @danmosemsft

@carlossanlop carlossanlop deleted the MutexAcl branch November 7, 2019 18:52
@ericstj
Copy link
Member

ericstj commented Nov 7, 2019

If we take it in servicing, we will need to bump the version and include the package in the build. Dan are you suggesting we take in servicing, or just let it ship out of master for 5.0 (which can be used downlevel)?

@danmoseley
Copy link
Member

I figured that this means we don't have to decide yet -- we can see what feedback we get, including from the 1st party converter that originally motivated all this work -- and we have options. (So yes @carlossanlop I suggest to simply get everything into master at this point.)

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.

6 participants