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

[release/5.0] Handle inconsistent \0 in Pkcs9Document{Name|Description} #45153

Merged
merged 2 commits into from
Nov 25, 2020

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 24, 2020

Backport of #45040 to release/5.0

/cc @bartonjs

Customer Impact

Customers migrating from .NET Framework get different answers for Pkcs9DocumentName.Name and Pkcs9DocumentDescription.Description when the contents contain embedded NUL characters or lack the canonical trailing NUL character.

Testing

New tests added for both embedded NUL and missing NUL

Risk

Low

@ghost
Copy link

ghost commented Nov 24, 2020

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

Issue Details

Backport of #45040 to release/5.0

/cc @bartonjs

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Security

Milestone: -

@bartonjs
Copy link
Member

@joperezr Hm, this is in an OOB... I probably need to do things like manually bump the package version and add it to packages to rebuild, instead of just letting the port bot port the change, right?

@joperezr
Copy link
Member

Yes that is correct you will want to manually add those two changes as extra commits to this PR as the bot won’t know it needs to do that. If you want an example, just check how it was done on this already merged servicing PR https://github.com/dotnet/runtime/pull/43903/files

Basically you want to pay attention to the change made on the library’s directory.build.props that bumps the package and assembly versions, and also the change into libraries-packages.proj where the build of this package is manually added to the build. Finally, if you manually build this locally you can use the UpdatePackageIndex target to maje the required uodates on the package index by building: dotnet msbuild yourlibrary.pkgproj /t:UpdatePackageIndex

@bartonjs
Copy link
Member

dotnet msbuild yourlibrary.pkgproj /t:UpdatePackageIndex seemed to make a whole lot of unrelated, and suspicious-looking, changes, so I only included the Pkcs library parts.

It found that we hadn't indexed version 4.7.0, but it didn't also add an entry in the assembly-version-to-package-version part. I'm not sure if that's sensible or not. (it had 4.7.0 as the last item in that list, I moved it to be sorted)

@bartonjs bartonjs added the Servicing-consider Issue for next servicing release review label Nov 24, 2020
@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 24, 2020
@leecow leecow modified the milestones: 5.0.x, 5.0.1 Nov 24, 2020
@danmoseley
Copy link
Member

@bartonjs there is a relevant failure on the net48 leg.

@joperezr
Copy link
Member

OOB packaging changes looks good to me.

seemed to make a whole lot of unrelated, and suspicious-looking, changes, so I only included the Pkcs library parts.

Mmm that is weird, it would be interested to check what other changes were made by our task.

It found that we hadn't indexed version 4.7.0, but it didn't also add an entry in the assembly-version-to-package-version part. I'm not sure if that's sensible or not.

It is not really an issue. The tool can't find that information very trivial (it would need to download all packages that we have ever shipped for that library) since that table tells you the first package version where a new assembly version was included. We use that table in case some other library references Pkcs and specifically reference an older assemblyVersion like 4..0.4.0 for example, and so our packaging infrastructure would emit a package reference to version 4.6.0 of the Pkcs package, which is the lowest one where that assembly version appeared. All that said, this condition breaks whenever the baseline version is higher, and today we have it set to 5.0.0 so it doesn't really matter to get any new assembly version that showed up in 4.7.0 package.

I hope at least some part of the above explanation made sense, but the TLDR version is: The changes on the packageIndex you made look correct.

@bartonjs
Copy link
Member

@bartonjs there is a relevant failure on the net48 leg.

This looks like it's a machine issue, I have a ticket with engineering to get it worked out. It passed in master, and then a later run hit the same machine it failed on here.

#45168 is our side of tracking the failure, though it'll hopefully just stop happening once any machines with problems are cleared up.

@danmoseley danmoseley merged commit ce2b755 into release/5.0 Nov 25, 2020
@danmoseley danmoseley deleted the backport/pr-45040-to-release/5.0 branch November 25, 2020 01:19
@danmoseley
Copy link
Member

Oops, I didn't see your last comment @bartonjs - I just merged based on signoff, the machine issue and Jose's signoff

@ghost ghost locked as resolved and limited conversation to collaborators Dec 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants