Enable signing validation of .pkg files using SignCheck #15489
Conversation
ellahathaway
force-pushed
the
signcheck-pkg
branch
from
0672fa2 to
31a4325
Compare
|
Current output from test run is: [File] test.pkg, Signed: True [Error] Timestamp (2/7/2025 6:54:02 PM) outside validity period (2/1/2027 10:12:15 PM to 2/1/2027 10:12:15 PM)., Timestamp (2/7/2025 6:54:02 PM) outside validity period (2/1/2027 10:12:15 PM to 2/1/2027 10:12:15 PM)., Timestamp (2/7/2025 6:54:02 PM) outside validity period (2/9/2035 9:40:36 PM to 2/9/2035 9:40:36 PM).
[File] 6741a098e863e0ff41f9bddf2c67747dbd9adde100e3deca6e308852287baddd.pkg, Signed: False, Virtual path: test.pkg/NestedPkg.pkg, Full Name: NestedPkg.pkg [Error] Timestamp (2/7/2025 6:54:02 PM) outside validity period (2/1/2027 10:12:15 PM to 2/1/2027 10:12:15 PM)., Timestamp (2/7/2025 6:54:02 PM) outside validity period (2/1/2027 10:12:15 PM to 2/1/2027 10:12:15 PM)., Timestamp (2/7/2025 6:54:02 PM) outside validity period (2/9/2035 9:40:36 PM to 2/9/2035 9:40:36 PM).Need to figure out two things:
|
ellahathaway
force-pushed
the
signcheck-pkg
branch
from
31a4325 to
ccbd244
Compare
| @@ -1,7 +1,7 @@ | |||
| <!-- Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. --> | |||
| <Project> | |||
|
|
|||
| <Import Project="..\DefaultVersions.Generated.props"/> | |||
There was a problem hiding this comment.
This looks suspect. IIRC, the generated file contains the just built versions of the tooling packages. Without this, you end up with N-1, I think?
There was a problem hiding this comment.
DefaultVersions.props imports DefaultVersions.Generated.props, so I was thinking that this would still allow us to end up with the "just build" versions of the tooling packages. But I think you're right, this would just import the N-1 versions.
The reason that I switched the imported project was to get access to MicrosoftDotNetMacOsPkgVersion which is in DefaultVersions.props and not DefaultVersions.Generated.props. That said, I should be able to add the pkg version prop to generated content specified in the _GenerateSdkVersionFile target, which should make the prop available in DefaultVersions.Generated.props
| @@ -25,7 +25,7 @@ public static string Run(string command, string arguments = "", string workingDi | |||
| process.Start(); | |||
| output = process.StandardOutput.ReadToEnd(); | |||
| process.WaitForExit(60000); // 60 seconds | |||
| if (process.ExitCode != 0) | |||
| if (process.ExitCode != 0 && throwOnError) | |||
There was a problem hiding this comment.
I would suggest refactoring this to return the exit code (or bool) from the method, with an out parameter for stdout/stderr, then allow the caller to decide whether to throw or not.
| @@ -9,7 +9,6 @@ | |||
| <IsPackable>true</IsPackable> | |||
| <Description>The MacOsPkg tool is used for unpacking and packing MacOS .pkg files and nested .app bundles.</Description> | |||
| <PackageTags>Arcade Build Tool MacOS Pkg</PackageTags> | |||
| <DevelopmentDependency>false</DevelopmentDependency> | |||
There was a problem hiding this comment.
I'm guessing you're doing this so that SigningValidation ends up with a package ref to this, and it gets restored? This kind of makes me think we should factor out the MacOsPkg functionality into a library, with a command line tool that uses the library. Then you can avoid passing around the path to the pkg tooling and just redist the library with the task.
There was a problem hiding this comment.
Makes sense. I think that this would be better done in a separate PR, so I'll work on those changes first then update this PR later.
| foreach (var path in Directory.EnumerateFiles(extractionPath, "*.*", SearchOption.AllDirectories)) | ||
| { | ||
| var relativePath = path.Substring(extractionPath.Length + 1).Replace(Path.DirectorySeparatorChar, '/'); | ||
| using var stream = (Stream)File.Open(path, FileMode.Open); |
There was a problem hiding this comment.
I thiiink this is okay but I am not sure. The using statement means that the stream would be disposed at the end of the for loop iteration. It doesn't look like the entry.Content member ends up getting used outside of the current iteration.
There was a problem hiding this comment.
I agree. I looked more closely at the way that SignTool does this, and I think the method there is better... it opens the stream, yield returns the entry, then closes the stream. That feels a lot safer.
I'm going to open a separate PR that includes this change, along with other modifications to the ArchiveVerifier logic that have been included in this current PR.
| protected virtual IEnumerable<ArchiveEntry> ExtractArchiveEntries(string archivePath, string extractionPath) | ||
| { | ||
| ZipFile.ExtractToDirectory(archivePath, extractionPath); | ||
| foreach (var path in Directory.EnumerateFiles(extractionPath, "*.*", SearchOption.AllDirectories)) |
There was a problem hiding this comment.
The end of method is common with PkgVerifier.ExtractArchiveEntries. Pull out to a common helper?
|
Converting this to a draft PR. There are a few things to do first that will clean up the logic in this PR:
|
Closes dotnet/source-build#4857
Pipeline run