-
Notifications
You must be signed in to change notification settings - Fork 136
Fix for Update-Module fails with ModuleAuthenticodeSignature error for modules with signed PSD1 #12
Conversation
Hi @bmanikm, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
$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 |
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.
ValidateAndGet-AuthenticodeSignature [](start = 28, length = 36)
it looks like ValidateAndGet-AuthenticodeSignature may throws invalid auth sign... Any reasons not verbose or error here? #Closed
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.
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 ` |
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.
What is the reason by changing error ignore to SilentlyContinue? #Closed
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.
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) | ||
{ |
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.
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
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.
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)
|
@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 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. |
@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? |
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.