Add S.S.C.Cose with COSE Sign1 support by jozkee · Pull Request #64461 · dotnet/runtime

jozkee · 2022-01-28T18:18:06Z

Fixes #32121.
Contributes to #62600.

ghost · 2022-01-28T18:18:12Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-01-28T18:18:16Z

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #32121.
Contributes to #62600.

Author:	Jozkee
Assignees:	Jozkee
Labels:	`area-System.Security`
Milestone:	7.0.0

...s/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseHeaderLabel.cs

...ies/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseHeaderMap.cs

...aries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseMessage.cs

src/libraries/System.Security.Cryptography.Cose/ref/System.Security.Cryptography.Cose.csproj

src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj

...s/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseHeaderLabel.cs

* Fix ThreadStatic properties * Switch from SkipOnPlatformAttribute to IgnoreForCI in csproj

.../System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseSign1Message.cs

...ies/System.Private.CoreLib/src/System/Runtime/Versioning/RequiresPreviewFeaturesAttribute.cs

src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj

...s/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseHeaderLabel.cs

src/libraries/System.Security.Cryptography.Cose/tests/CoseHeaderLabelTests.cs

bartonjs

As long as someone who knows about building things signs off on ProjectRef vs PackageRef, and @tannergooding or someone can give a grunt of assent regarding the RequiresPreview attribute, I think that we can call this good enough for first partner preview.

src/libraries/System.Security.Cryptography.Cose/ref/System.Security.Cryptography.Cose.cs

...aries/System.Security.Cryptography.Cose/tests/System.Security.Cryptography.Cose.Tests.csproj

safern

Other than a few comments/questions, build changes look good.

tests: add test project to exclusion list of local browser runs

buyaa-n

Left a NIT, overall attributes annotations LGTM

buyaa-n · 2022-02-07T18:26:49Z

src/libraries/System.Security.Cryptography.Cose/ref/System.Security.Cryptography.Cose.cs

+        public bool Verify(System.Security.Cryptography.ECDsa key, System.ReadOnlySpan<byte> content) { throw null; }
+        [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]
+        public bool Verify(System.Security.Cryptography.RSA key) { throw null; }
+        [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]


NIT: probably better to apply the attribute at the type level as all membrs have it except internal constructor

Given that is a nit and the CI finished mostly clean, I will address this in a future PR.

One a second thought, we shouldn't move the attribute to the type since the internal .ctor is used in an API (CoseMessage.DecodeSign1) that can be called from browser.

buyaa-n · 2022-02-07T18:47:30Z

.../System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseSign1Message.cs

+        internal CoseSign1Message(CoseHeaderMap protectedHeader, CoseHeaderMap unprotectedHeader, byte[]? content, byte[] signature, byte[] protectedHeaderAsBstr)
+            : base(protectedHeader, unprotectedHeader, content, signature, protectedHeaderAsBstr) { }
+
+        [UnsupportedOSPlatform("browser")]


browser-compat is supposed to be opt-in, so I wouldn't expect this to trip.

That is right, by defualt it should not warn for cross platform builds, but browser team wanted to see/propogate the warning for cross-platform builds in runtime repo

bartonjs · 2022-02-07T19:00:35Z

That is right, by defualt it should not warn for cross platform builds, but browser team wanted to see/propogate the warning for cross-platform builds in runtime repo

Ahhh. Thanks for the explanation :)

Add S.S.C.Cose

facccf2

jozkee added the area-System.Security label Jan 28, 2022

jozkee added this to the 7.0.0 milestone Jan 28, 2022

jozkee requested a review from bartonjs January 28, 2022 18:18

jozkee self-assigned this Jan 28, 2022

ghost added the new-api-needs-documentation label Jan 28, 2022

jozkee added 2 commits January 28, 2022 10:51

Override Equals in CoseHeaderLabel

611aa65

Add SystemFormatsCborVersion to eng/Versions.props

e89936d