@lzybkr @SteveL-MSFT Is there something more I need to do for this PR?

LGTM but we need review from MSFT experts.

@PowerShell/powershell-committee reviewed the topic of params or automatic variables passed to the script block delegate. There was concerns about new automatic variables scoped only to this usage but would needed to be documented in the about_automaticvariables topic which may create some confusion. In addition, params seems to be a general pattern already in use across PowerShell, so the request is to use params for the script block (and update the doc draft accordingly).

There were other concerns we didn't have time to discuss such as which runspace the delegate ran in. For now, continue that discussion in this PR.

Well that's baffling. It's not a surprise that the certificate validation doesn't work on macOS. I ran into similar issues enabling -Certificate because macOS (and some linux distors) use a different implementation than openssl that CoreFX doesn't support but, how did these tests manage to pass on macOS in previous commits?

• working: https://travis-ci.org/PowerShell/PowerShell/jobs/282050275
• not working: https://travis-ci.org/PowerShell/PowerShell/jobs/287024709

It should have failed before.

There was concerns about new automatic variables scoped only to this usage but would needed to be documented in the about_automaticvariables topic which may create some confusion.

@SteveL-MSFT Was the prospect of providing the callback parameters to the scriptblock by way of properties on $_ contemplated? That feels like the most PowerShell-ey style to me. There is precedent for non-pipeline use of $_ instead of param() in ValidateScript().

I've been favoring $_ over param() for scriptblock predicates like ServerCertificateCustomValidationCallback mainly because the param() block ends up being repeated, often overshadows the predicate in character count, and usually doesn't provide much useful information to a reader.

Here's a mock-up of param() vs $_ for illustration:

{
    param($Message,$Cert,$Chain,$PolicyErrors)
    Assert-CertificateCompliance -Policy Strict -Message $Message -Cert $Cert -Chain $Chain -PolicyErrors $PolicyErrors
}

{$_ | Assert-CertificateCompliance -Policy Strict}

For me the point of this PR is to be able to impose per-URL certificate requirements. So I'll end up with policy data structures looking something like this:

@{
    Uri = 'https://github.com'
    CertificateValidator = {
        $_ | Assert-CertificateCompliance -Policy Strict
        $_ | Assert-Certificate Server -Thumbprint d79f076110b39293e349ac89845b0380c19e2f8b
    }
}

@{
    Uri = 'https://www.powershellgallery.com'
    CertificateValidator = {
        $_ | Assert-CertificateCompliance -Policy Strict
        $_ | Assert-Certificate Intermediate -Thumbprint ‎98af62baa639b0827f887c75a77f4afd4eff2685
    }
}

The validation scriptblock gets repeated for each uri so noise from param() adds up.

@alx9r no, using members of $_ wasn't something that was considered and is an interesting approach. Would like some others to chime in on this proposal. cc @lzybkr @JamesWTruher @HemantMahawar

@SteveL-MSFT I had an early draft that did it that way. I had dropped it in favor of the automatic variables before submitting the PR.

@markekraus sorry, must have missed it. Getting myself involved in too many discussions on here to keep track of them (something I'm learning not to do!)

@SteveL-MSFT I don't think the draft ever made it's way to my public repo so I don't think you missed anything, actually. My point was more that I'm on board with the idea of a $_ with members if that lends any weight in the decision making.

After much trial and error I determined that the only safe way to do this where any kind of event triggers were in place was run the ScriptBlock in a completely new session state and runspace. Variables could easily be imported, but I did tests where the event triggers updated a variable at that same time as callback and that resulted in a hang. Importing functions and modules leaves them in a disconnected state from the user's context (module scoped variables are lost).

I have incorporated the $_. I'm open for suggestions on renaming the members. Right now they are Request, Certificate, CertificateChain, and SslErrors. This current implementation does not support param(). I'm assuming we want to go with one or the other and not both.

I have tested the current code with the event triggers @lzybkr provided and it works. no hangs.

The down side is that the ScriptBlock is now devoid of the calling scope. I was originally under the impression it would need to be done this way and I think that so long as it is documented it should be acceptable. Any of the other means of making the scope available results in either unpredictable behavior or hangs. I think no scope is better than some broken scope.

...I did tests where the event triggers updated a variable at that same time as callback and that resulted in a hang.

I'm curious what these tests looked like. It doesn't seem like Register-ObjectEvent naturally accepts PSVariables in its parameters. It also doesn't seem like the Register-ObjectEvent -Action scriptblock has access the call site's variables. In the tests that hung how did the event handler get a reference to the same variable as the callback?

I have incorporated the $_. I'm open for suggestions on renaming the members. Right now they are Request, Certificate, CertificateChain, and SslErrors.

I see that the current documentation for HttpClientHandler.ServerCertificateCustomValidationCallback doesn't name its parameters. For that matter, there is very little documentation about the semantics of that callback altogether. dotnet/corefx#24774 might improve that documentation and that might lead to parameter names. If ServerCertificateCustomValidationCallback does end up with documented parameter names, it seems like the properties on $_ should have identical names. The closest documented callback interface seems to be full .net's System.Net.Security.RemoteCertificateValidationCallback. I propose adopting the corresponding names from that callback interface in the meantime.

The down side is that the ScriptBlock is now devoid of the calling scope. I was originally under the impression it would need to be done this way and I think that so long as it is documented it should be acceptable. Any of the other means of making the scope available results in either unpredictable behavior or hangs. I think no scope is better than some broken scope.

Is there a reason that the scope rules wouldn't be the same as for other scriptblocks using remote variables? Invoke-Command -Session -ScriptBlock, for example, provides access to local variables in the remote session by way of the $using: expression.

FWIW, I think this would be a pretty awkward interface without having some way of making user objects available in the callback scriptblock. A natural use would be as follows:

@{
    u='https://github.com/some/important/file'
    thumbprint = 'd79f076110b39293e349ac89845b0380c19e2f8b'
},
@{
    u='https://powershellgallery.com/some/other/important/file'
    thumbprint = '98af62baa639b0827f887c75a77f4afd4eff2685'
} |
    % { 
        $thumbprint = $_.thumbprint
        # ... do some prep things
        Invoke-WebRequest $_.u -CertificateValidationScript {
            # ... a bunch of tests that are identical for every certificate
            $_.Certificate.Thumbprint -eq $using:thumbprint
        }
        # ... do some cleanup things
    }

Without $using:, arguments, or some other means of passing data, it will be tough to separate the concern of which thumbprints are acceptable for which url from the concern of checking for certificate revokation, for example.

I'm curious what these tests looked like.

I just used the same variable in @lzybkr's example and used it

$t = [System.Timers.Timer]::new(10)
$t.AutoReset = $true
Register-ObjectEvent -InputObject $t -EventName Elapsed -Action { Write-Host 'event' }
$t.Enabled = $true
$script = {$t.Enabled = $false; $true}
irm -uri https://expired.badssl.com
#hangs

$t was being populated as a PSVarabile in the iss.

I propose adopting the corresponding names from that callback interface in the meantime.

I disagree. Unless it is documented on this API, we should use something generic that is easier for PowerShell users to use. Sender is a terrible choice when it's an HttpRequestMessage. If Sender were the documented name for this API, then I would concede that's probably better.

Is there a reason that the scope rules wouldn't be the same as for other scriptblocks using

$using in every instance I've seen is a unique implementation. If there were a generic API for adding this capability to a ScriptBlock, then $using: would be awesome to have here. but I see no point in adding yet-another-using-implimentation for validation callbacks on cmdlets that don't normally do any work with ScriptBlocks.

Without $using:, arguments, or some other means of passing data,..

Erm, not really? I mean yea, not on the ease of use level that using variables from the calling scope or $using:, but, most validation logic is fairly static, in my experience. Create module, Import it in the ScriptBlock, magic. or use CSV, Clixml, JSON files for confgis if you don't wants it hard coded in the the PS Script.

I'm not saying this is ideal. What I am saying is that I can't see a safe way to get the calling scope available in the remote scope with out severely over-complicating the implementation.

Also, these validations scripts are "per call". For 6.1 I want to look at adding session persistent options. One of them will be add your own .NET delegates for cert validation, So if you want to have some variable magic then you can. The goal in this PR is to provide the basic implementation that will work for most people on a "per call" basis. I think most will just need to ensure a static thumbprint or 2 against URLs. Complex validation scenarios are not the goal of the implementation in this PR, but are on my radar for the futre.

Complex validation scenarios are not the goal of the implementation in this PR, but are on my radar for the futre.

OK. Got it. Thanks @markekraus

rebased to fix merge conflict.

@markekraus Another merge conflict.

rebased.

@PowerShell/powershell-committee reviewed this and recommends using the param() format. Specifically, $_ provides no context on what it represents. Typically $_ means the current object and reusing it in the scriptblock changes that expectation. Using param() makes the code more readable and therefore maintainable.

@markekraus Please rebase with next new commit to get latest updates and pass CIs.

@iSazonov I still need to incorporate the changes per the committee's decision. I will hopefully have free time this week. I will rebase before I commit the changes.

FYI, I'm waiting on #5518 as this will need to rebased again and more drastically.

Due to the issues of rebasing this and that the code still needs to be updated to fit the review comitee's decision, I'm going to close this PR. I will attempt to address this feature before 6.1.0.