[release/3.0]Handle `UnparseableExtension` status code when building X509Chain on … by buyaa-n · Pull Request #40117 · dotnet/corefx

buyaa-n · 2019-08-07T21:12:12Z

Fixes #39988
Port of #40063

Description

Handle UnparseableExtension status code when building X509Chain on OSX 10.15

Impact

Allow building X509Chain to support UnparseableExtension status and gracefully fail on latest OSX

Regression?

No

Risk

Low

…OSX (dotnet#40063) Setting related error flag for newly introduced error status

krwq

You might want to wait for @bartonjs's approval since he'll be back before preview9's cutoff date (around 8/20)

buyaa-n · 2019-08-07T22:51:12Z

You might want to wait for @bartonjs's approval since he'll be back before preview9's cutoff date (around 8/20)

Sure will wait

danmoseley · 2019-08-07T23:46:20Z

@krwq based on what I undrestand of this issue, it seems like the right fix and ideally we would put it in now rather than waiting, that gives more time for any possible issue to emerge upstack. Do you feel OK with doing that? If you think it's important to wait for @bartonjs that's fine.

krwq · 2019-08-08T01:46:11Z

@danmosemsft I'm good with that

danmoseley · 2019-08-08T02:07:03Z

K I’ll take to tactics

bartonjs · 2019-08-08T05:07:42Z

If this code is not reported by Windows then mapping it to nothing is a better fix.

Just because the code sounds like the right answer, if Windows isn't reporting it, we shouldn't from macOS either.

bartonjs · 2019-08-08T05:08:13Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_x509chain.c

    else if (CFEqual(keyString, CFSTR("MissingIntermediate")))
        *pStatus |= PAL_X509ChainPartialChain;
+    else if (CFEqual(keyString, CFSTR("UnparseableExtension")))
+        *pStatus |= PAL_X509ChainInvalidExtension;


The InvalidAia test must be changed to show that this code is reported by both Windows and macOS (and, ideally, Linux),

We tried to add assert that, but it failed on windows, didn't checked on Linux
Assert.Contains(chain.ChainStatus, (status) => status.Status.HasFlag(X509ChainStatusFlags.InvalidExtension));

bartonjs

Shouldn't be merged into release/3.0 without confirmation it's decreasing, vs increasing, Windows behavior deltas.

danmoseley · 2019-08-08T11:05:22Z

Thanks @bartonjs

danmoseley · 2019-08-08T17:08:59Z

Removing ask mode pending addressing @bartonjs feedback. @buyaa-n please when fixed set ask-mode label for tactics on Tuesday. (Eric E will be running it that week)

@buyaa-n you said this does not apply to 2.x right? if it does it will need a port there.

buyaa-n · 2019-08-08T17:19:29Z

If this code is not reported by Windows then mapping it to nothing is a better fix.
Just because the code sounds like the right answer, if Windows isn't reporting it, we shouldn't from macOS either.

You mean ignoring it better for now?

Shouldn't be merged into release/3.0 without confirmation it's decreasing, vs increasing, Windows behavior deltas.

I might didn't get this comment correctly. If we check this status in TestInvalidAia its failing for Windows. So i assume the requested change is to ignore this status

buyaa-n · 2019-08-08T17:21:30Z

Removing ask mode pending addressing @bartonjs feedback. @buyaa-n please when fixed set ask-mode label for tactics on Tuesday. (Eric E will be running it that week)

Sure i will put back the tag after getting approval from @bartonjs

@buyaa-n you said this does not apply to 2.x right? if it does it will need a port there.

Yes It is not failing in 2.x. no need to port

krwq · 2019-08-08T23:46:02Z

@buyaa-n are you sure it's not supported on Windows vs current implementation ignoring the code?

buyaa-n · 2019-08-09T00:06:30Z

@krwq I was not sure, thats why i asked about it from you at lunch, but haven't checked windows code yet

krwq · 2019-08-09T08:18:55Z

@buyaa-n Windows does this: https://github.com/dotnet/corefx/blob/master/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/ChainPal.BuildChain.cs#L75

so you will need to read https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certgetcertificatechain to figure out if there is some easy way to get the error

buyaa-n · 2019-08-09T22:26:32Z

@bartonjs I have compared X509ChainStatus-es produced from Windows and Mac with ignored solution. Windows produces:

PartialChain
RevocationStatusUnknown
OfflineRevocation

Mac produces:

RevocationStatusUnknown
PartialChain

And if i map new status UnparseableExtension into PAL_X509ChainOfflineRevocation i see same result. Now i understood what you meant by

Shouldn't be merged into release/3.0 without confirmation it's decreasing, vs increasing, Windows behavior deltas.

So the correct mapping must be PAL_X509ChainOfflineRevocation, but its just sound very different than UnparseableExtension. Anyways am gonna add this mapping and add test case asserting this value

buyaa-n · 2019-08-10T00:22:31Z

@bartonjs added test case is failing for Linux and old Mac OS, seems better remove the test case

bartonjs · 2019-08-13T17:49:26Z

So the correct mapping must be PAL_X509ChainOfflineRevocation, but its just sound very different than UnparseableExtension.

No, OfflineRevocation is a different code (and I'm surprised Windows is returning it). Windows might be returning it as a way of saying "due to inputs I couldn't complete this operation", but generally it means "you used X509RevocationMode.Offline and this value isn't in the cache". If it was an extension other than Authority Information Access we'd probably see different results.

Anyways am gonna add this mapping and add test case asserting this value

I don't see the test case as part of this PR.

Based on the data in this thread I believe the correct action is to map it to nothing.

It might make sense to just make the test say it doesn't care about revocation (since it doesn't):

    X509Chain chain = new X509Chain();
+   chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
    Assert.False(chain.Build(ee));

Then we'll reduce the red herring of OfflineRevocation, and we can then just add

    Assert.Equals(1, chain.ChainElements.Count);
+   Assert.Equals(X509ChainStatusFlags.PartialChain, chain.AllStatusFlags());

buyaa-n · 2019-08-13T20:00:07Z

@bartonjs updated accordingly, please re review

bartonjs

LGTM (as long as the test run backs up my assertions).

buyaa-n · 2019-08-13T22:11:57Z

Tests passed, good to go, thank you @bartonjs @krwq

stephentoub · 2019-08-14T01:01:12Z

@buyaa-n, was this approved by shiproom for ask mode?
cc: @eerhardt

krwq · 2019-08-14T01:15:09Z

@buyaa-n also we need to merge this change back to master since there was some additional feedback from @bartonjs addressed here

buyaa-n · 2019-08-14T04:27:34Z

@buyaa-n, was this approved by shiproom for ask mode?

oops, sorry forgot that, added ask mode, even its too late now, in case its not approved maybe raise reverse PR?

@buyaa-n also we need to merge this change back to master since there was some additional feedback from @bartonjs addressed here

Yes i'm having that ready, just left all tests run again, will raise PR after this one got approved

buyaa-n · 2019-08-14T16:44:49Z

@krwq created backport as it is the correct update for master even no matter 3.0 approval #40313

eerhardt · 2019-08-14T18:32:11Z

was this approved by shiproom for ask mode?

Unfortunately, no it wasn't. I'll bring it up at the next shiproom to see if they wouldn't have approved it. And if so, if we should back it out.

But let's wait for shiproom approval for any future changes in 3.0.

eerhardt · 2019-08-15T17:27:09Z

This was approved in shiproom today since it affects macOS 10.15.

buyaa-n · 2019-08-15T17:35:52Z

Thank you @eerhardt

* [release/3.0]Handle `UnparseableExtension` status code when building X509Chain on … (#40117) Ignore `UnparseableExtension` status code when building X509Chain on OSX * [release/3.0] Update dependencies from 3 repositories (#40308) * Update dependencies from https://github.com/dotnet/core-setup build 20190814.02 - Microsoft.NETCore.App - 3.0.0-preview9-19414-02 - Microsoft.NETCore.DotNetHostPolicy - 3.0.0-preview9-19414-02 - Microsoft.NETCore.DotNetHost - 3.0.0-preview9-19414-02 * Update dependencies from https://github.com/dotnet/arcade build 20190812.7 - Microsoft.DotNet.XUnitExtensions - 2.4.1-beta.19412.7 - Microsoft.DotNet.XUnitConsoleRunner - 2.5.1-beta.19412.7 - Microsoft.DotNet.VersionTools.Tasks - 1.0.0-beta.19412.7 - Microsoft.DotNet.ApiCompat - 1.0.0-beta.19412.7 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19412.7 - Microsoft.DotNet.Build.Tasks.Configuration - 1.0.0-beta.19412.7 - Microsoft.DotNet.Build.Tasks.Feed - 2.2.0-beta.19412.7 - Microsoft.DotNet.Build.Tasks.Packaging - 1.0.0-beta.19412.7 - Microsoft.DotNet.CodeAnalysis - 1.0.0-beta.19412.7 - Microsoft.DotNet.CoreFxTesting - 1.0.0-beta.19412.7 - Microsoft.DotNet.GenAPI - 1.0.0-beta.19412.7 - Microsoft.DotNet.GenFacades - 1.0.0-beta.19412.7 - Microsoft.DotNet.Helix.Sdk - 2.0.0-beta.19412.7 - Microsoft.DotNet.RemoteExecutor - 1.0.0-beta.19412.7 * Update dependencies from https://github.com/dotnet/standard build 20190814.3 - NETStandard.Library - 2.1.0-prerelease.19414.3 * fixing ZipPackagePart.GetStreamCore crashes with NotSupportedException (#40355) ZipArchiveEntry only ever supports opening once when the backing archive is in Create mode, and the backing stream is non-seekable, so we shouldn't call SetLength in that case. You could still open an archive in Update mode then call part.GetStream(FileMode.Create), in which case we'll want this call to SetLength, so we only avoid this call when the backing Archive is in Create mode. updating test to explicitly test the Update path for ZipPackage skip UAP since we don't have access to the file system to create the .zip undo accidental change to existing test removing unnecessary variable * Support custom converters that treat non-null input as null (#40287) (#40357)

Handle UnparseableExtension status code when building X509Chain on …

4cd3e8e

…OSX (dotnet#40063) Setting related error flag for newly introduced error status

buyaa-n added os-mac-os-x OS-X aka Mac OS ask-mode labels Aug 7, 2019

buyaa-n added this to the 3.0 milestone Aug 7, 2019

buyaa-n requested review from bartonjs, danmoseley and krwq August 7, 2019 21:12

krwq approved these changes Aug 7, 2019

View reviewed changes

bartonjs reviewed Aug 8, 2019

View reviewed changes

bartonjs suggested changes Aug 8, 2019

View reviewed changes

danmoseley removed the ask-mode label Aug 8, 2019

Ignoring new status code as its not supported in windows

15eb0b7

Mapping Mac error status same as in Windows, added test

48acf70

Removing assert as it is not supported on all platform

42ce1e6

Ignoring unparseable status code

2c25304

bartonjs approved these changes Aug 13, 2019

View reviewed changes

buyaa-n merged commit 3694ef6 into dotnet:release/3.0 Aug 13, 2019

buyaa-n added the ask-mode label Aug 14, 2019

buyaa-n added the area-System.Security label Aug 14, 2019

buyaa-n mentioned this pull request Aug 14, 2019

Backport from 3.0 #40313

Merged

buyaa-n deleted the port_x509_fix branch November 1, 2019 20:59

vcsjones mentioned this pull request May 1, 2020

[release/2.1] Handle additional chain statuses for X509Chain on macOS #42916

Closed

Conversation

buyaa-n commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Impact

Regression?

Risk

Uh oh!

krwq left a comment

Choose a reason for hiding this comment

Uh oh!

buyaa-n commented Aug 7, 2019

Uh oh!

danmoseley commented Aug 7, 2019

Uh oh!

krwq commented Aug 8, 2019

Uh oh!

danmoseley commented Aug 8, 2019

Uh oh!

bartonjs commented Aug 8, 2019

Uh oh!

bartonjs Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

buyaa-n Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartonjs left a comment

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Aug 8, 2019

Uh oh!

danmoseley commented Aug 8, 2019

Uh oh!

buyaa-n commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buyaa-n commented Aug 8, 2019

Uh oh!

krwq commented Aug 8, 2019

Uh oh!

buyaa-n commented Aug 9, 2019

Uh oh!

krwq commented Aug 9, 2019

Uh oh!

buyaa-n commented Aug 9, 2019

Uh oh!

buyaa-n commented Aug 10, 2019

Uh oh!

bartonjs commented Aug 13, 2019

Uh oh!

buyaa-n commented Aug 13, 2019

Uh oh!

bartonjs left a comment

Choose a reason for hiding this comment

Uh oh!

buyaa-n commented Aug 13, 2019

Uh oh!

stephentoub commented Aug 14, 2019

Uh oh!

krwq commented Aug 14, 2019

Uh oh!

buyaa-n commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buyaa-n commented Aug 14, 2019

Uh oh!

eerhardt commented Aug 14, 2019

Uh oh!

eerhardt commented Aug 15, 2019

Uh oh!

buyaa-n commented Aug 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

buyaa-n commented Aug 7, 2019 •

edited

Loading

buyaa-n Aug 8, 2019 •

edited

Loading

buyaa-n commented Aug 8, 2019 •

edited

Loading

buyaa-n commented Aug 14, 2019 •

edited

Loading