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

Do not include extensions sequence if no extensions #40944

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

vcsjones
Copy link
Member

Fix for macOS Big Sur rejecting X509 certificates that have an empty extension sequence.

Fixes #40939

When encoding an X509 certificate, omit the extensions sequence if
the effective number of extensions is zero. Per RFC 5280, this sequence
is "one or more certificate extensions".
@ghost
Copy link

ghost commented Aug 17, 2020

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

@bartonjs
Copy link
Member

Test failures look unrelated.

@bartonjs bartonjs merged commit e1cacbd into dotnet:master Aug 17, 2020
@vcsjones vcsjones deleted the fix-40939 branch August 17, 2020 22:17
@danmoseley
Copy link
Member

@bartonjs do we need a backport for this to 2.1 and/or 3.1?

@vcsjones
Copy link
Member Author

@danmosemsft my 2c: it's a bit of a corner case. It will only occur when doing request.CertificateExtensions.Add(null);, and adding null isn't something someone would typically do. I would consider holding off on until a customer reports it. If one does, they have a work around: don't try to add null to the extensions list. null gets skipped anyway, so they wouldn't be losing anything.

@bartonjs
Copy link
Member

I wouldn't. The test was largely codifying an API (mis)usage and maintaining some corner-case compat. As @vcsjones said while I was writing an equivalent statement: anyone who encounters this can do their own "don't add null to this collection".

The nullability annotations for the type say that null isn't supported in context, anyways (so we're really dealing with garbage-in, garbage-out; but we're now limiting how smelly the garbage is).

@danmoseley
Copy link
Member

OK, thanks @bartonjs . What about #39603 is that likely to need backport? Since we have only today/tomorrowish if we want to do that and be in the patch before Big Sur ships.

@vcsjones
Copy link
Member Author

Re: #39603, I can't reproduce it in netcoreapp3.1 or 2.1, only in 5 previews, so there isn't anything to fix in a backport that I've identified.

@danmoseley
Copy link
Member

OK excellent thanks for the update!

@vcsjones
Copy link
Member Author

Will the corefx repo get CI legs for Big Sur? If so, the test that prompted this change will start failing. We can either fix the test or port this if we need to.

@danmoseley
Copy link
Member

I expect we will switch out a CI leg for Big Sur when it is released.

@vcsjones
Copy link
Member Author

Should we preemptively try to address this then? It might make more sense to just change the test to wrap that part of the test in if (PlatformDetection.IsMacOSCatalinaOrLower) rather than changing product code. Not sure if that makes it easier for you on servicing paperwork.

@bartonjs
Copy link
Member

I think on 2.1/3.1 we'd just guard the test.

@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
4 participants