Enlighten SignFile for multithreaded mode#13621
Enlighten SignFile for multithreaded mode#13621jankratochvilcz wants to merge 2 commits intomainfrom
Conversation
Add [MSBuildMultiThreadableTask] and implement IMultiThreadableTask. Absolutize SigningTarget.ItemSpec before passing to SecurityUtilities.SignFile(). Fixes #13618 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the built-in SignFile task to be compatible with MSBuild’s multithreaded execution model by ensuring file I/O uses project-directory-relative path resolution rather than process-wide Environment.CurrentDirectory.
Changes:
- Marks
SignFileas[MSBuildMultiThreadableTask]and implementsIMultiThreadableTaskwith aTaskEnvironmentproperty. - Absolutizes
SigningTarget.ItemSpecviaTaskEnvironment.GetAbsolutePath(...)before callingSecurityUtilities.SignFile(...).
| AbsolutePath signingTargetPath = TaskEnvironment.GetAbsolutePath(SigningTarget.ItemSpec); | ||
| SecurityUtilities.SignFile( | ||
| CertificateThumbprint, | ||
| TimestampUrl == null ? null : new Uri(TimestampUrl), | ||
| SigningTarget.ItemSpec, | ||
| signingTargetPath, |
There was a problem hiding this comment.
Because SigningTarget.ItemSpec is now converted to an absolute path before calling SecurityUtilities.SignFile, any downstream FileNotFoundException.FileName will also be absolute. The task currently logs ex.FileName for MSB3484, which will leak the absolutized path to the user and change existing error text. Consider logging SigningTarget.ItemSpec (or signingTargetPath.OriginalValue) instead of ex.FileName so the message preserves the original input.
There was a problem hiding this comment.
Good catch — Sin 2. Fixed in the latest commit: hoisted signingTargetPath above the try (Sin 4 also) and the FileNotFoundException catch now logs signingTargetPath.OriginalValue instead of ex.FileName.
| SecurityUtilities.SignFile( | ||
| CertificateThumbprint, | ||
| TimestampUrl == null ? null : new Uri(TimestampUrl), | ||
| SigningTarget.ItemSpec, | ||
| signingTargetPath, | ||
| TargetFrameworkVersion, | ||
| TargetFrameworkIdentifier, | ||
| DisallowMansignTimestampFallback); |
There was a problem hiding this comment.
Passing an absolute path into SecurityUtilities.SignFile changes the contents of exceptions thrown by SecurityUtilities (e.g., signtool failure/warning messages include the {path} argument). The task logs ex.Message.Trim() verbatim for MSB3482/MSB3483, so this will now surface the absolute path (and may violate the "no absolute path in user-visible output" requirement). Consider rewriting/normalizing the exception message before logging (e.g., replace the absolutized path with the original input) or emitting a task-local error message that uses the original path string.
There was a problem hiding this comment.
Good catch — also Sin 2 territory. Addressed in the latest commit by sanitizing ex.Message before logging: any embedded absolutized path is replaced with OriginalValue. The replacement is a no-op when the input was already absolute, so no behavior change for existing callers.
…sages - Hoist signingTargetPath declaration above try so it's in scope of all catch blocks (Sin 4). - MSB3484 (FileNotFoundException): log signingTargetPath.OriginalValue instead of ex.FileName, which would otherwise leak the absolutized path. - MSB3482/MSB3483: sanitize ex.Message from SecurityUtilities by replacing the absolutized path with OriginalValue, so signtool error text continues to display the user's original input (Sin 2). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
JanProvaznik
left a comment
JanProvaznik
left a comment
There was a problem hiding this comment.
#13173 I had an attempt here, found it's not so simple and then context switched to do something else. the SecurityUtils library has nested problems with using process global state, which either need to be refactored or analyzed why they're not an issue.
┌────────────────────────────────────────────────────┬─────────────────────────┬───────────────────────────────────────────────┐
│ Issue │ Location │ Risk │
├────────────────────────────────────────────────────┼─────────────────────────┼───────────────────────────────────────────────┤
│ Directory.GetCurrentDirectory() fallback in │ SecurityUtil.cs:875 │ Low — signtool rarely found via CWD │
│ GetPathToTool │ │ │
├────────────────────────────────────────────────────┼─────────────────────────┼───────────────────────────────────────────────┤
│ SetDllDirectoryW unsynchronized process-global │ SecurityUtil.cs:728,760 │ Real race — concurrent SignFile tasks could │
│ mutation │ │ interfere │
├────────────────────────────────────────────────────┼─────────────────────────┼───────────────────────────────────────────────┤
│ ProcessStartInfo without explicit WorkingDirectory │ SecurityUtil.cs:785 │ Medium — child process inherits wrong CWD in │
│ │ │ MT mode │
└────────────────────────────────────────────────────┴─────────────────────────┴───────────────────────────────────────────────┘
| string SanitizeMessage(string msg) => msg?.Replace((string)signingTargetPath, signingTargetPath.OriginalValue) ?? msg; | ||
| try | ||
| { | ||
| SecurityUtilities.SignFile( |
There was a problem hiding this comment.
the insides of SecurityUtilities.SignFile depend on process global state
Adds
[MSBuildMultiThreadableTask]andIMultiThreadableTasktoSignFile. AbsolutizesSigningTarget.ItemSpecbefore passing toSecurityUtilities.SignFile().Fixes #13618
Parent epic: #11834