Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SecureString to plain text conversion using NetWorkCredential instead of marshalling #59

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

santisq
Copy link
Contributor

@santisq santisq commented Feb 29, 2024

PR Summary

Context

Sent the PR to the wrong repo 😅. Also added a few more changes to Add-EnvPath (the logic could be simplified).

Changes

Adding tools/Modules and output folder to .gitignore, otherwise the modules needed for CI/CD and the released module would get pushed to repository.

Added launch file for easier debugging, i.e.: Ctrl+Shift+Enter opens a new pwsh with the module already loaded.

Added tasks file, easier building, i.e.: Ctrl+Shift+B runs build.ps1 and F1 -> Run Test Task runs build.ps1 -Task Test.

Using NetworkCredential to decrypt the secure string instead of marshalling. This method properly handles the unmanaged memory, makes it simpler and correctly frees the unmanaged memory.

Otherwise, if marshalling:

  1. PtrToStringAuto should be PtrToStringBSTR instead to avoid encoding issues.
  2. There should be a finally block in the function that calls [System.Runtime.InteropServices.Marshal]::ZeroFreeBSTR($BSTR) to free the unmanaged memory, otherwise the plain text password lives in unmanaged memory while the pwsh process is alive.

Both points above are correctly managed by NetworkCredential.

Using [System.IO.Path]::PathSeparator instead of ; to split the path in case you want to make this module compatible with Linux in the future (its separator is :). Trim() to remove any extra trailing or leading whitespace from each token and then filtering any token that could be purely white space, i.e.:

PS /> 'foo;; bar; baz ;;'.Split([System.IO.Path]::PathSeparator).Trim() -ne '' -join ';'
foo;bar;baz

Same change to split using [System.IO.Path]::PathSeparator.

Checklist

  • Pull Request has a meaningful title.
  • Summarised changes.
  • Pull Request is ready to merge & is not WIP.
  • Added tests / only testable interactively.
    • Make sure you add a new test if old tests do not effectively test the code changed.
  • Added documentation / opened issue to track adding documentation at a later date.

@hjorslev hjorslev merged commit 37afdd8 into hjorslev:master Mar 1, 2024
5 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants