PKISolutions / PSPKI

PowerShell PKI Module
Microsoft Public License
379 stars 57 forks source link

Populate FunctionsToExport with explicit list of commands #205

Open FriedrichWeinmann opened 8 months ago

FriedrichWeinmann commented 8 months ago

Hi,

thanks for the awesome project! I've got a small feature request: Could you - going forward - update your PSPKI.psd1 on release to no longer include FunctionsToExport = '*' and instead list each command individually? The wildcard is convenient as you develop, but it has some problems when trying to run the module in a hardened environment and it makes discovery a pain.

For example, I can search for all modules on the gallery that contain a given command:

Find-Module -Command Revoke-Certificate

As of now, there's one module that's not going to be found ...

Crypt32 commented 8 months ago

The module determines exported commands in runtime depending on client capabilities to avoid exporting functions that technically cannot be executed in a particular session. As the result, the list of exported functions is dynamic and cannot be statically populated in .psd1 file.

FriedrichWeinmann commented 8 months ago

Would it not be more useful then to publish all commands and fail the ones whose requirements are not met? As of now, running a command on a incompatible client would result in a "Command not found" error, rather than a "Bad Client, do XYZ to fix" error. I can't count the number of times I've had to help an admin with Exchange Tooling where they were looking for the correct version when it was just a matter of insufficient permissions (EX/EXO only give you the commands you have permissions to use).

hmiller10 commented 8 months ago

Exporting functions with an '*' in the manifest file will cause PowerShell not recognize the function if PowerShell is running in constrained language mode. Each function must be explicitly listed as is currently being done.

Crypt32 commented 8 months ago

Would it not be more useful then to publish all commands and fail the ones whose requirements are not met?

That's what I wanted to avoid. Otherwise, I will have to write platform/dependency support check in every function and this will add confusion: the command exist, but cannot run.

FriedrichWeinmann commented 8 months ago

I'd argue on the confusion part - not whether it will happen, but which of the two possible points of confusion are worse (since you don't get to pick an option that is 100% confusion free for everybody).

Given that the target audience is likely to be CA admins rather than PowerShell experts, I believe that the "Command cannot be used" problem is a lot easier to explain or understand.

As for the implementation headache - I strongly recommend implementing assertions for that kind of situation:

One internal function per environment condition:

function Assert-Something {
    <#
        .SYNOPSIS
                Assert that something is true.

        .DESCRIPTION
                Assert that something is true.

        .PARAMETER Cmdlet
                The $PSCmdlet variable of the calling command.

        .EXAMPLE
                PS C:\> Assert-Something -Cmdlet $PSCmdlet

                Assert that something is true.
#>
    [CmdletBinding()]
    param (
        [Parameter(Mandatory = $true)]
        $Cmdlet
    )

    process {
        if ("Something" -is $true) {
            return
        }

        $exception = [System.InvalidOperationException]::new("Invalid environment! This command can only run if XYZ is true!")
        $record = [System.Management.Automation.ErrorRecord]::new($exception, "InvalidEnvironment", [System.Management.Automation.ErrorCategory]::InvalidOperation, $null)
        $Cmdlet.ThrowTerminatingError($record)
    }
}

First statement in the begin block of each function thus constrained:

Assert-Something -Cmdlet $PSCmdlet

With that you have one central place where you implement both code and error message, the actual impact on your individual commands' complexity is minimal. The internas are hidden from the user and you can use the error message to usefully complain what is the issue, so the confusion should be minimal.

Thank you for taking the time on this issue, whether I managed to convince you or not (it is a matter of perspectives and priorities, so I'm hardly going to judge either way). Your work on this project is appreciated :)