Add SecurityTransparentAttribute to System.Collections.Immutable

[AArnott]

I'm currently soliciting verification results from @NArnott

Fix #3293
Fix #1512

[stephentoub]

@weshaggard, @joshfree, are there any other corefx assemblies we should be doing this with? I'm wondering if we should move this file to src/Common.

[weshaggard]

I'm not entirely sure what the implications of this are so I cannot say definitively. Unless someone does some proper test validation and understands the scope I don't think I'm comfortable with blindly doing this.

[stephentoub]

I don't think I'm comfortable with blindly doing this

Thanks... just to be clear, are you referring to this specific change to immutable collections, or are you referring to my question about doing it elsewhere as well?

[weshaggard]

I was referring to the general case. I'm OK with individual libraries adding this if the right testing and security audits are done (i.e. we know we don't call critical code without proper checks and annotations).

[weshaggard]

Actually thinking a little more I kind of recall @jkotas suggesting we remove all security annotations for our libraries so perhaps he has an opinion on whether we should be marking this transparent or not.

[stephentoub]

This library (and some of the others in corefx) seem like special cases, though, as they are meant to be usable with runtimes that do enforce security annotations.

[jkotas]

Supporting partial trust is not as simple as simple as adding a security transparent annotation.

You have to also make sure that the assembly actually works in partial trust, ie. verify that the security transparent code is not calling security critical code - both statically using secanotate tool, and dynamically (by running tests in environment that enforces security transparency).

Paying for this extra burden would be prohibitive across the board for corefx, paying for it in a few special cases should be ok.

[AArnott]

Thanks, @jkotas. I'll read up on that tool and try to run it against this particular library.

[AArnott]

The secannotate report is clean:

Annotating 'System.Collections.Immutable'.
Beginning pass 1.
Pass complete, 0 new annotation(s) generated.

If I was more comfortable with the build process and knew it would work either cross platform or could constrain it to only happen on Windows, I could wire up secannotate to run as part of the build or test process. But as I think this library is unlikely to make any calls to Critical code, it's probably overkill to guard against introducing such unless the cost is low.

[stephentoub]

LGTM

[AArnott]

@stephentoub I was waiting for @NArnott to sign off on the effectiveness of this fix in a partial trust application prior to merging this.

[stephentoub]

Oops, my bad. I can revert it out if you'd like.

[AArnott]

Let's wait and see what @NArnott says before backing it out. I'm hopeful it will work. :)