Add support for credential persistence

[cansuerdogan]

PR Summary

• Add an -Authentication parameter to the Register-PSResourceRepository
• Add an authentication dimension to the PSRespositoryInfo object
• The -Authentication parameter will take a hashtable which specifies the vault and secret where the credential is stored
• Detailed error messaging to indicate why authentication failed (SecretManagement wasn't installed, no vault was available, the vault couldn't be reached etc.)
• Since not all repositories requires authentication (principally PSGallery) PowerShellGet will not take a dependency
  on SecretManagement
• When <x>-PSResource is called and the prioritized repository is determined, a check will also be made to the Authentication property

PR Context

Add functionality for supporting credential persistence as discussed in issue #330.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Documentation needed
    • Issue filed:
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
    • Cmdlets have been tested manually due to the private feed requirement
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and enumerated concerns in the summary.

[ghost]

All CLA requirements met.

[alerickson]

Thanks @cansuerdogan for this PR! It's very thorough and looks good so far, @anamnavi and I are reviewing and will update you shortly. Just adding a couple of small nitpicks right now.

[cansuerdogan]

Thank you @alerickson! Please let me know if you have any questions.

[PaulHigin]

Just started reviewing the code, and have a few initial comments.

[PaulHigin]

I didn't get to this PR this week, but will make it a high priority to review early next week. Sorry for the delay.

[PaulHigin]

Overall I think this is a good start. However, I feel a number of changes should be made and/or considered.

The cmdlet parameter should be -AuthenticationInfo instead of -Authentication. The latter usually refers to authentication type, whereas the former indicates information used for authentication.

The -AuthenticationInfo parameter should take a concrete type (AuthenticationInformation) instead of a hashtable type. This way we don't have to repeatedly check the hashtable for the required name/value pairs. Basic validation can be done at binding (non-null, not-empty values). Also, hashtable syntax can still be used to pass the authentication information, since PowerShell will automatically convert to the concrete type during parameter binding.

# This still works, even though -AuthenticationInfo takes a 'AuthenticationInformation' class type.
Register-PSResourceRepository -Name MyRepo -URL https://MyRepo -AuthenticationInfo @{ VaultName='MyVault'; SecretName='MyRepoCreds' }

I think we should also consider a parameter set option that automatically persists credentials when they are provided. Something like:

Register-PSResourceRepository -Name MyRepo -URL https://MyRepo -ProxyCredential $credentials -PersistCredentials @{ VaultName='MyVault'; SecretName='MyRepoCreds' }

We should be consistent and persist PSCredential type in the SecretManagement vault (since we pass in PSCredential type for -ProxyCredential parameter.

We should use PSCmdlet.InvokeCommand.InvokeScript() API to invoke the SecretManagement Get-Secret, Set-Secret commands. This will run the command in the current cmdlet context and allow the vault to prompt the user for a passkey if needed. For example the SecretStore vault defaults to 'password required' configuration and user prompting for password when needed.

We should try to do AuthenticationInfo validation as early as possible.

We need to make sure reported errors are useful and not confusing, in cases where SecretManagement is not installed/configured correctly.

• SecretManagement not installed
• Vault not found
• Secret not found

Not sure, but we may want to catch errors and wrap them in our own to provide better context.

[cansuerdogan]

Thank you @PaulHigin for the feedback! Sorry I was away for a few days but I will try to get your suggestions implemented and pushed hopefully by EOD tomorrow.

• I already verified PSCmdlet.InvokeCommand.InvokeScript() works last week but I still need to spend some time on error messages.
• I will rename the parameter and change the type it expects.
• Just to make sure I'm understanding the -PersistCredential proposal:
  • Have a parameter set for the combination of -ProxyCredential and -PersistCredential
  • If -ProxyCredential is provided by itself, proceed as it is now, i.e. don't save the credential anywhere
  • If both are provided, call SecretManagement\Set-Secret with the credential and authentication info passed in

Is this correct? If so, do we want to maybe take in only the VaultName and use the username from PSCredential as the secret name? Because when it's time to pass credential into the Nuget library call, we are assuming SecretName is the username.

[PaulHigin]

@cansuerdogan
I think we still need to talk through the proposals, and decide on how to proceed. We hope to do this internally (early) next week, and then I think it would be good to have a live chat with you, if you are available. In the mean time it may not be worthwhile to make too many code changes. My aim is to soon have a list of action items that we all agree on.

[cansuerdogan]

Hi @alerickson, @anamnavi and @PaulHigin,

It has been a while sorry! I can finally get back to working on this pull request. Do you still have the notes from our meeting by any chance?

Here is what I remember:

• Parameter name will be changed to CredentialInfo
• I will be adding a concrete type for CredentialInfo which will have the following:
  • String VaultName
  • String SecretName
  • PSCredential Credential
• Register-PSResourceRepository will at the very least take VaultName and SecretName as part of CredentialInfo, but optionally, users can also pass in a PSCredential to save the secret in the provided vault
  • If only VaultName and SecretName are passed in, PowerShellGet will NOT print out a warning regarding SecretManagement setup since this can come later
  • If additionally a Credential is passed in and SecretManagement setup is wrong or incomplete, PowerShellGet will throw a terminating error and will not register the repository
• We will keep the PowerShell Invoker implementation for importing and using SecretManagement as part of PowerShellGet but try to add more meaningful error messages where things may go wrong

Are these accurate? Please let me know if I'm missing anything or misremembering any part.

Thank you!

[alerickson]

Hi @cansuerdogan just wanted to check in. We are targeting mid December for our next release, with all changes in by the end of the month so we can have a couple weeks to do manual testing and validations. This PR certainly doesn't need to be in by then, but I just wanted to keep you updated.

[cansuerdogan]

Hi @alerickson! I had some time off in between so I wasn't able to get back to this PR sorry. Can you please confirm if what I remembered from our discussion (summarized in this comment) was correct? If so I'll get started on those changes and we can sync up after I have them implemented. Thank you!

[SydneyhSmith]

Thanks @cansuerdogan your notes from the meeting all look correct, thanks for following up! It would be awesome to know when you may have a chance to return to this work--thanks so much!

[cansuerdogan]

@SydneyhSmith Sorry for the delay on this! I will try to finish this up by end of next week.

[StevenBucher98]

Thanks for the update @cansuerdogan, we are planning for a new release on Wednesday (12/8, so it would need to be in this week) so we might not be able to fit it in that release but will shoot to include it in the next release earlier in the new year! Thanks so much!

[cansuerdogan]

Thank you for the heads-up @StevenBucher98 ! Good luck with the new release!

[PaulHigin]

Sorry for the delay in the review. Thanks for doing this work! Overall changes look good, most comments are just style changes.

[PaulHigin]

LGTM. Again, thanks for your contribution!

[anamnavi]

had 2 minor nitpicks, but LGTM. Thank you for your contribution, this looks great :)

[anamnavi]

is this necessary or were you creating this file locally to output the tests to and trying to ignore?

[cansuerdogan]

I was running the tests locally to verify and did not want to commit this file that gets generated. I can remove it if you'd like.

[anamnavi]

That makes sense, if you can remove it though that would be appreciated :)

[anamnavi]

please add a newline; nitpick

[cansuerdogan]

Thank you all for your guidance and collaboration!