Skip to content
This repository was archived by the owner on Jun 13, 2024. It is now read-only.

Fix for Update-Module fails with ModuleAuthenticodeSignature error for modules with signed PSD1 #12

Merged
merged 1 commit into from
Oct 12, 2016

Conversation

bmanikm
Copy link
Contributor

@bmanikm bmanikm commented Oct 12, 2016

Fix for 'Update-Module fails with ModuleAuthenticodeSignature error for modules with signed PSD1' #8
Also includes fix for 'Properties of AdditionalMetadata are case-sensitive' #7
Changed the ErrorAction to Ignore for few cmdlet usages as they should not show up in ErrorVaraible. For example, error returned by 'Get-Command Test-FileCatalog' should be ignored.

…or modules with signed PSD1' #8

Fix for 'Properties of AdditionalMetadata are case-sensitive'  #7
Changed the ErrorAction to Ignore for few cmdlet usages as they should not show up in ErrorVaraible. For example, error returned by 'Get-Command Test-FileCatalog' should be ignored.
@msftclas
Copy link

msftclas commented Oct 12, 2016

Hi @bmanikm, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;
#Closed

$InstalledModuleDetails = Get-InstalledModuleAuthenticodeSignature -InstalledModuleInfo $InstalledModuleInfo `
-InstallLocation $InstallLocation
}

# Validate the catalog signature for the current module being installed.
$ev = $null
$CurrentModuleDetails = ValidateAndGet-AuthenticodeSignature -ModuleInfo $CurrentModuleInfo -ErrorVariable ev
Copy link

@jianyunt jianyunt Oct 12, 2016

Choose a reason for hiding this comment

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

ValidateAndGet-AuthenticodeSignature [](start = 28, length = 36)

it looks like ValidateAndGet-AuthenticodeSignature may throws invalid auth sign... Any reasons not verbose or error here? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

terminating errors are returned to the user and verbose messages are written by ValidateAndGet-AuthenticodeSignature


In reply to: 83076681 [](ancestors = 83076681)

@@ -14122,7 +14123,7 @@ function Validate-ModuleCommandAlreadyAvailable
$CommandNamesWithWildcards = $CommandNames | Microsoft.PowerShell.Core\Foreach-Object { "$_*" }

$AvailableCommand = Microsoft.PowerShell.Core\Get-Command -Name $CommandNamesWithWildcards `
-ErrorAction SilentlyContinue `
Copy link

@jianyunt jianyunt Oct 12, 2016

Choose a reason for hiding this comment

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

What is the reason by changing error ignore to SilentlyContinue? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the ErrorAction to Ignore for few cmdlet usages as they should not show up in ErrorVaraible. For example, error returned by 'Get-Command Test-FileCatalog' should be ignored.


In reply to: 83077138 [](ancestors = 83077138)

$AuthenticodeSignature = Microsoft.PowerShell.Security\Get-AuthenticodeSignature -FilePath $ModuleInfo.Path

if($AuthenticodeSignature)
{
Copy link

@jianyunt jianyunt Oct 12, 2016

Choose a reason for hiding this comment

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

Can users run Test-ModuleManifest to get know whether catalog sign is valid? it will be handy we can do it before publish-mdoule. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the proposal, please open a tracking issue for this in PS\PS. Also please note, that change will not be available on down-level PS versions and PowerShellGet will have to use Test-FileCatalog cmdlet for the validation.


In reply to: 83077731 [](ancestors = 83077731)

@jianyunt
Copy link

:shipit:

@bmanikm bmanikm merged commit 8c8463e into PowerShell:master Oct 12, 2016
@bmanikm bmanikm deleted the psget_manikb branch October 12, 2016 19:41
@kilasuit
Copy link

@bmanikm @jianyunt Where where/are the tests that validate the changes made do not cause any issues in back compatibility? Where is/was the ability for additional community feedback before the merge occurred?

Lets get this right going forward - i.e

no new tests = no merge
failing tests = no merge
no multi-platform testing = no merge (this has to be supportable down to Win7/2008R2 and PSv3)
no chance for community feedback & testing ( 1 hour between PR opened and merged isn't realistic nor viable) = no merge

so that I'm not raising this same point on all PR's going forward which

Same goes for the OneGet / PackageManagement Repo too.

This has to be done right and merging in code like this with no additional tests for the new/updated/changed functionality is not acceptable for a Microsoft and a PowerShell repository especially where there is an older version of this code being shipped in the 2016 & Win10 OSes

As I said lets get this right from here on in and if that means that bugfixes or new functionality gets put on hold instead of just being blindly merged in then that is the price that needs to be paid for not doing it right in the first place.

@jianyunt
Copy link

@kilasuit you raised good points. We will consider your suggestions. We are working in progress making the tests successfully running via appveyor, travisCI, etc. . In addition to do it, we manually perform tests on other Oss before PR. Regarding community feedback & testing, do you have any suggestions how we can collaborate together and perform it effectively, for example, not wait for feedback too long but still be able to hear from the community?

@bmanikm bmanikm changed the title Fixes for #8 and #7 Fix for Update-Module fails with ModuleAuthenticodeSignature error for modules with signed PSD1 Nov 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants