Skip to content

Enable force repack for binaries that were signed externally #2140

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 11, 2019
Merged

Enable force repack for binaries that were signed externally #2140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
793f344
Enable force repack for binaries that were signed externally
weshaggard Feb 28, 2019
f7783bb
Update logging level to reduce noise at normal levels
weshaggard Mar 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions src/Microsoft.DotNet.SignTool/src/BatchSignUtil.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,11 @@ private void VerifyAfterSign(FileSignInfo file)
{
if (!_signTool.VerifySignedPEFile(stream))
{
_log.LogError($"Assembly {file.FullPath} is not signed properly");
_log.LogError($"Assembly {file.FullPath} is NOT signed properly");
}
else
{
_log.LogMessage(MessageImportance.Low, $"Assembly {file.FullPath} is signed properly");
}
}
}
Expand Down Expand Up @@ -321,9 +325,16 @@ private void VerifyAfterSign(FileSignInfo file)
}
}

if (!SkipZipContainerSignatureMarkerCheck && (file.IsNupkg() || file.IsVsix()) && !signedContainer)
if (!SkipZipContainerSignatureMarkerCheck)
{
_log.LogError($"Container {file.FullPath} does not have signature marker.");
if ((file.IsNupkg() || file.IsVsix()) && !signedContainer)
{
_log.LogError($"Container {file.FullPath} does not have signature marker.");
}
else
{
_log.LogMessage(MessageImportance.Low, $"Container {file.FullPath} has a signature marker.");
}
}
}
}
Expand All @@ -342,6 +353,10 @@ private void VerifyStrongNameSigning()
{
_log.LogError($"Assembly {file.FullPath} is not strong-name signed correctly.");
}
else
{
_log.LogMessage(MessageImportance.Low, $"Assembly {file.FullPath} strong-name signature is valid.");
}
}
}

Expand Down
37 changes: 22 additions & 15 deletions src/Microsoft.DotNet.SignTool/src/Configuration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using System.Reflection.Metadata;
using System.Reflection.PortableExecutable;
using System.Runtime.Versioning;
using Microsoft.Build.Framework;
using Microsoft.Build.Utilities;

namespace Microsoft.DotNet.SignTool
Expand Down Expand Up @@ -131,17 +132,23 @@ internal void ReadExistingContainerSigningCache()
// might have changed the hash but we want to still use the same hash of the unsigned
Copy link
Contributor

@JohnTortugo JohnTortugo Mar 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment still accurate?

Copy link
Member Author

@weshaggard weshaggard Mar 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is still accurate. The has might be different.

// file that originally built the cache.
string stringHash = cacheRelative.Substring(0, indexOfHash);

ImmutableArray<byte> contentHash;
try
{
ImmutableArray<byte> contentHash = ContentUtil.StringToHash(stringHash);
contentHash = ContentUtil.StringToHash(stringHash);
}
catch {
catch
{
_log.LogMessage($"Failed to parse the content hash from path '{file}' so skipping it.");
continue;
}

TrackFile(file, ContentUtil.StringToHash(stringHash), false);
// if the content of the file doesn't match the hash in file path than the file has changed
// which indicates that it was signed so we need to ensure we repack the binary with the signed version
string actualFileHash = ContentUtil.HashToString(ContentUtil.GetContentHash(file));
bool forceRepack = stringHash != actualFileHash;

TrackFile(file, contentHash, false, forceRepack);
}
_log.LogMessage("Done loading existing files from cache");
}
Expand Down Expand Up @@ -193,10 +200,10 @@ internal BatchSignInput GenerateListOfFiles()
return new BatchSignInput(_filesToSign.ToImmutableArray(), _zipDataMap.ToImmutableDictionary(ByteSequenceComparer.Instance), _filesToCopy.ToImmutableArray());
}

private FileSignInfo TrackFile(string fullPath, ImmutableArray<byte> contentHash, bool isNested)
private FileSignInfo TrackFile(string fullPath, ImmutableArray<byte> contentHash, bool isNested, bool forceRepack = false)
{
_log.LogMessage($"Tracking file '{fullPath}' isNested={isNested}");
var fileSignInfo = ExtractSignInfo(fullPath, contentHash);
var fileSignInfo = ExtractSignInfo(fullPath, contentHash, forceRepack);

var key = new SignedFileContentKey(contentHash, Path.GetFileName(fullPath));

Expand All @@ -220,18 +227,18 @@ private FileSignInfo TrackFile(string fullPath, ImmutableArray<byte> contentHash
}
}

_log.LogMessage($"Caching file {key.FileName}");
_log.LogMessage(MessageImportance.Low, $"Caching file {key.FileName} {key.StringHash}");
_filesByContentKey.Add(key, fileSignInfo);

if (fileSignInfo.SignInfo.ShouldSign || fileSignInfo.IsZipContainer())
if (fileSignInfo.SignInfo.ShouldSign || fileSignInfo.ForceRepack || fileSignInfo.IsZipContainer())
{
_filesToSign.Add(fileSignInfo);
}

return fileSignInfo;
}

private FileSignInfo ExtractSignInfo(string fullPath, ImmutableArray<byte> hash)
private FileSignInfo ExtractSignInfo(string fullPath, ImmutableArray<byte> hash, bool forceRepack = false)
{
// Try to determine default certificate name by the extension of the file
var hasSignInfo = _fileExtensionSignInfo.TryGetValue(Path.GetExtension(fullPath), out var signInfo);
Expand Down Expand Up @@ -286,8 +293,8 @@ private FileSignInfo ExtractSignInfo(string fullPath, ImmutableArray<byte> hash)
// If has overriding info, is it for ignoring the file?
if (SignToolConstants.IgnoreFileCertificateSentinel.Equals(explicitCertificateName, StringComparison.OrdinalIgnoreCase))
{
_log.LogMessage($"File configurated to not be signed: {fileName}{fileSpec}");
return new FileSignInfo(fullPath, hash, SignInfo.Ignore);
_log.LogMessage($"File configured to not be signed: {fileName}{fileSpec}");
return new FileSignInfo(fullPath, hash, SignInfo.Ignore, forceRepack:forceRepack);
}

// Do we have an explicit certificate after all?
Expand All @@ -301,7 +308,7 @@ private FileSignInfo ExtractSignInfo(string fullPath, ImmutableArray<byte> hash)
{
if (isAlreadySigned && !_dualCertificates.Contains(signInfo.Certificate))
{
return new FileSignInfo(fullPath, hash, SignInfo.AlreadySigned);
return new FileSignInfo(fullPath, hash, SignInfo.AlreadySigned, forceRepack:forceRepack);
}

// TODO: implement this check for native PE files as well:
Expand All @@ -323,7 +330,7 @@ private FileSignInfo ExtractSignInfo(string fullPath, ImmutableArray<byte> hash)
}
}

return new FileSignInfo(fullPath, hash, signInfo, (peInfo != null && peInfo.TargetFramework != "") ? peInfo.TargetFramework : null);
return new FileSignInfo(fullPath, hash, signInfo, (peInfo != null && peInfo.TargetFramework != "") ? peInfo.TargetFramework : null, forceRepack:forceRepack);
}

if (SignToolConstants.SignableExtensions.Contains(extension) || SignToolConstants.SignableOSXExtensions.Contains(extension))
Expand All @@ -339,7 +346,7 @@ private FileSignInfo ExtractSignInfo(string fullPath, ImmutableArray<byte> hash)
_log.LogMessage($"Ignoring non-signable file: {fullPath}");
}

return new FileSignInfo(fullPath, hash, SignInfo.Ignore);
return new FileSignInfo(fullPath, hash, SignInfo.Ignore, forceRepack: forceRepack);
}

private void LogWarning(SigningToolErrorCode code, string message)
Expand Down Expand Up @@ -519,7 +526,7 @@ private bool TryBuildZipData(FileSignInfo zipFileSignInfo, out ZipData zipData)
fileSignInfo = TrackFile(tempPath, contentHash, isNested: true);
}

if (fileSignInfo.SignInfo.ShouldSign)
if (fileSignInfo.SignInfo.ShouldSign || fileSignInfo.ForceRepack)
{
nestedParts.Add(new ZipPart(relativePath, fileSignInfo));
}
Expand Down
5 changes: 4 additions & 1 deletion src/Microsoft.DotNet.SignTool/src/FileSignInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ internal readonly struct FileSignInfo
// optional file information that allows to disambiguate among multiple files with the same name:
internal readonly string TargetFramework;

internal readonly bool ForceRepack;

internal static bool IsPEFile(string path)
=> Path.GetExtension(path) == ".exe" || Path.GetExtension(path) == ".dll";

Expand Down Expand Up @@ -64,7 +66,7 @@ internal static bool IsZipContainer(string path)

internal bool IsPowerShellScript() => IsPowerShellScript(FileName);

internal FileSignInfo(string fullPath, ImmutableArray<byte> contentHash, SignInfo signInfo, string targetFramework = null)
internal FileSignInfo(string fullPath, ImmutableArray<byte> contentHash, SignInfo signInfo, string targetFramework = null, bool forceRepack = false)
{
Debug.Assert(fullPath != null);
Debug.Assert(!contentHash.IsDefault && contentHash.Length == 256 / 8);
Expand All @@ -75,6 +77,7 @@ internal FileSignInfo(string fullPath, ImmutableArray<byte> contentHash, SignInf
FullPath = fullPath;
SignInfo = signInfo;
TargetFramework = targetFramework;
ForceRepack = forceRepack;
}

public override string ToString()
Expand Down