GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add missing timestamper cert to the newer catalog signing APIs by PaulHigin · Pull Request #4061 · PowerShell/PowerShell

Conversation

Paul Higinbotham (PaulHigin) commented Jun 20, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

This fixes Issue #4060

Code to obtain the file signature time stamp cert was missing from the newer catalog signing APIs, with the result that Get-AuthenticodeSignature would not include the time stamp cert in the Signature object.

Fix is to refactor and use the older API code for obtaining this.

I didn't add a test for this because no catalog signing tests are in GitHub yet and porting them will be a separate work item.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Fixed missing functionality to add timestamper cert for the newer cat…

34b34ff

…alog signing APIs

Paul Higinbotham (PaulHigin) added WG-Cmdlets

general cmdlet issues

Issue-Bug

Issue has been identified as a bug in the product

OS-Windows labels

Jun 20, 2017

Paul Higinbotham (PaulHigin) added this to the 6.0.0-Pending milestone

Jun 20, 2017

Paul Higinbotham (PaulHigin) assigned Lee Holmes (LeeHolmes)

Jun 20, 2017

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label

Jun 20, 2017

Mike Richmond (mirichmo) requested a review from Lee Holmes (LeeHolmes)

June 20, 2017 22:56

Mike Richmond (mirichmo) assigned Mike Richmond (mirichmo) and unassigned Lee Holmes (LeeHolmes)

Jun 20, 2017

Paul Higinbotham (PaulHigin) requested a review from Travis Plunk (TravisEz13)

June 21, 2017 18:40

Travis Plunk (TravisEz13) reviewed

Jun 22, 2017

View reviewed changes

Comment thread

src/System.Management.Automation/security/Authenticode.cs

		}
		}

		Diagnostics.Assert(((error == 0) && (signature != null)) \|\| (error != 0), "GetSignatureFromWintrustData: general crypto failure");

Travis Plunk (TravisEz13) Jun 22, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Question about this case, will sync offline

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Travis Plunk (TravisEz13) approved these changes

Jun 22, 2017

View reviewed changes

Paul Higinbotham (PaulHigin) changed the title ~~Fixed missing functionality to add timestamper cert for the newer catalog signing APIs~~ Add missing timestamper cert to the newer catalog signing APIs

Jun 22, 2017

Paul Higinbotham (PaulHigin) requested a review from Dongbo Wang (daxian-dbw)

June 22, 2017 21:32

Dongbo Wang (daxian-dbw) reviewed

Jun 22, 2017

View reviewed changes

Comment thread

src/System.Management.Automation/security/Authenticode.cs Outdated

		}

		[ArchitectureSensitive]
		private static bool TryGetProviderSigner(IntPtr wvtStateData, ref IntPtr pProvSigner, ref X509Certificate2 timestamperCert)

Dongbo Wang (daxian-dbw) Jun 22, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It seems to me both pProvSigner and timestamperCert are better to be an out parameter instead of a ref parameter.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) reviewed

Jun 22, 2017

View reviewed changes

Comment thread

src/System.Management.Automation/security/Authenticode.cs

		else
		{
		signature = new Signature(filename, error, cert);
		}

Dongbo Wang (daxian-dbw) Jun 22, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Once the parameters pProvSigner and timestamperCert are changed to out parameters, you can use the new language feature in C# for out parameters:

if (int.TryParse(input, out int result))
    WriteLine(result);
else
    WriteLine("Could not parse input");

So this code block can be rewritten to:

if (TryGetProviderSigner(phStateData, out IntPtr pProvSigner, out X509Certificate2 timestamperCert) && timestamperCert != null)
{
    signature = new Signature(filename, error, cert, timestamperCert);
}
else
{
    signature = new Signature(filename, error, cert);
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) reviewed

Jun 22, 2017

View reviewed changes

Comment thread

src/System.Management.Automation/security/Authenticode.cs Outdated

		X509Certificate2 signerCert = null;
		IntPtr pProvSigner = IntPtr.Zero;
		X509Certificate2 timestamperCert = null;
		if (TryGetProviderSigner(wtd.hWVTStateData, ref pProvSigner, ref timestamperCert))

Dongbo Wang (daxian-dbw) Jun 22, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Same here. Once change to out parameters, you can use the new feature in C# to make the code cleaner.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Paul Higinbotham (PaulHigin) Jun 22, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good idea. Will change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Code review changes

0c663b4

Dongbo Wang (daxian-dbw) approved these changes

Jun 22, 2017

View reviewed changes

Mike Richmond (mirichmo) mentioned this pull request

Jun 23, 2017

Port Catalog Signing Tests to GitHub #4087

Closed

Mike Richmond (mirichmo) commented Jun 23, 2017

Copy link

Copy Markdown

Member

I created issue #4087 to document the lack of tests

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Paul Higinbotham (PaulHigin) commented Jun 27, 2017

Copy link

Copy Markdown

Contributor Author

Mike Richmond (@mirichmo) Is there anything preventing this from being merged? I also need to port this to internal branches. Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mike Richmond (mirichmo) merged commit 2ee9705 into PowerShell:master

Jun 27, 2017

Paul Higinbotham (PaulHigin) deleted the GetAuthSigFix branch

June 27, 2017 22:02

Paul Higinbotham (PaulHigin) mentioned this pull request

Jul 11, 2017

Get-AuthenticodeSignature -FilePath does not provide the TimeStamperCertificate in the returned signature #4060

Closed

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Add missing timestamper cert to the newer catalog signing APIs for Ge…

9f44c5b

…t-AuthenticodeSignature (PowerShell#4061)

* Fixed missing functionality to add timestamper cert for the newer catalog signing APIs for Get-AuthenticodeSignature

Reviewers

Dongbo Wang (daxian-dbw)

daxian-dbw approved these changes

Travis Plunk (TravisEz13)

TravisEz13 approved these changes

Lee Holmes (LeeHolmes)

Awaiting requested review from LeeHolmes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Mike Richmond (mirichmo)

Labels

Issue-Bug

Issue has been identified as a bug in the product

OS-Windows WG-Cmdlets

general cmdlet issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Paul Higinbotham (PaulHigin) commented Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Travis Plunk (TravisEz13) Jun 22, 2017

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) Jun 22, 2017

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) Jun 22, 2017

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) Jun 22, 2017

Choose a reason for hiding this comment

Uh oh!

Paul Higinbotham (PaulHigin) Jun 22, 2017

Choose a reason for hiding this comment

Uh oh!

Mike Richmond (mirichmo) commented Jun 23, 2017

Uh oh!

Paul Higinbotham (PaulHigin) commented Jun 27, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Paul Higinbotham (PaulHigin) commented Jun 20, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.