diff --git a/README.md b/README.md index 075321a..6da83d3 100644 --- a/README.md +++ b/README.md @@ -64,8 +64,9 @@ Import-Module PSProxmoxVE $cred = Get-Credential -UserName 'root@pam' Connect-PveServer -Server 'pve.example.com' -Credential $cred -SkipCertificateCheck -# Using API token -Connect-PveServer -Server 'pve.example.com' -ApiToken 'root@pam!mytoken=xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx' +# Using API token (SecureString) +$token = Read-Host -AsSecureString -Prompt 'API token (root@pam!mytoken=...)' +Connect-PveServer -Server 'pve.example.com' -ApiToken $token # Verify connection Test-PveConnection -Detailed @@ -167,7 +168,9 @@ API tokens provide persistent, non-expiring authentication. They are the recomme 4. Copy the token value — it is shown only once ```powershell -Connect-PveServer -Server 'pve.example.com' -ApiToken 'root@pam!automation=12345678-abcd-efgh-ijkl-123456789012' +# Read-Host keeps the token out of shell history and transcripts. +$token = Read-Host -AsSecureString -Prompt 'API token (root@pam!automation=...)' +Connect-PveServer -Server 'pve.example.com' -ApiToken $token ``` ## Multi-Cluster Usage diff --git a/docs/cmdlets/Connect-PveServer.md b/docs/cmdlets/Connect-PveServer.md index d2d730b..e7b3ef6 100644 --- a/docs/cmdlets/Connect-PveServer.md +++ b/docs/cmdlets/Connect-PveServer.md @@ -20,7 +20,7 @@ Connect-PveServer [-Server] [-Port ] -Credential ### ApiToken ``` -Connect-PveServer [-Server] [-Port ] -ApiToken [-SkipCertificateCheck] +Connect-PveServer [-Server] [-Port ] -ApiToken [-SkipCertificateCheck] [-TimeoutSeconds ] [-PassThru] [-Quiet] [-ProgressAction ] [] ``` @@ -39,10 +39,13 @@ PS C:\> {{ Add example code here }} ## PARAMETERS ### -ApiToken -API token in USER@REALM!TOKENID=UUID format. +API token in USER@REALM!TOKENID=UUID format, as a SecureString. + +Build one with `Read-Host -AsSecureString`, or read it from a secret vault. `ConvertTo-SecureString 'root@pam!mytoken=...' -AsPlainText -Force` also works, but a token written as a literal lands in shell history and in any transcript. +A plain string is still accepted in this release and emits a deprecation warning; it is removed in the next major release. ```yaml -Type: String +Type: SecureString Parameter Sets: ApiToken Aliases: diff --git a/src/PSProxmoxVE.Core/Authentication/PveSession.cs b/src/PSProxmoxVE.Core/Authentication/PveSession.cs index 648d4b1..4fdbf61 100644 --- a/src/PSProxmoxVE.Core/Authentication/PveSession.cs +++ b/src/PSProxmoxVE.Core/Authentication/PveSession.cs @@ -28,16 +28,16 @@ namespace PSProxmoxVE.Core.Authentication public PveAuthMode AuthMode { get; } /// The API token string, when using API token authentication. - public string? ApiToken { get; } + internal string? ApiToken { get; } /// The user (user@realm) the ticket was issued to; null for API token sessions. public string? Username { get; } /// The session ticket cookie value, when using ticket authentication. - public string? Ticket => ReadTicket()?.Ticket; + internal string? Ticket => ReadTicket()?.Ticket; /// The CSRF prevention token, when using ticket authentication. - public string? CsrfToken => ReadTicket()?.CsrfToken; + internal string? CsrfToken => ReadTicket()?.CsrfToken; /// The UTC expiry time for the session ticket. public DateTime TicketExpiry => ReadTicket()?.Expiry ?? DateTime.MaxValue; diff --git a/src/PSProxmoxVE/Cmdlets/Connection/ApiTokenTransformationAttribute.cs b/src/PSProxmoxVE/Cmdlets/Connection/ApiTokenTransformationAttribute.cs new file mode 100644 index 0000000..7fdaa6f --- /dev/null +++ b/src/PSProxmoxVE/Cmdlets/Connection/ApiTokenTransformationAttribute.cs @@ -0,0 +1,59 @@ +using System; +using System.Management.Automation; +using System.Runtime.CompilerServices; +using System.Security; + +namespace PSProxmoxVE.Cmdlets.Connection +{ + /// + /// Converts a plain API token argument to a so + /// that scripts written against the pre-SecureString parameter keep binding for one minor + /// release. A argument is returned unchanged. + /// + [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter)] + public sealed class ApiTokenTransformationAttribute : ArgumentTransformationAttribute + { + /// + /// The SecureStrings this attribute built from a plain string, held weakly so an argument + /// bound by an invocation that never reached its cmdlet is not kept alive by this table. + /// + private static readonly ConditionalWeakTable ConvertedFromString = + new ConditionalWeakTable(); + + private static readonly object Marker = new object(); + + public override object Transform(EngineIntrinsics engineIntrinsics, object inputData) + { + var value = inputData is PSObject wrapper ? wrapper.BaseObject : inputData; + + if (value is SecureString secure) + return secure; + + if (value is string plain) + { + var converted = new SecureString(); + foreach (var c in plain) + converted.AppendChar(c); + converted.MakeReadOnly(); + ConvertedFromString.Add(converted, Marker); + return converted; + } + + return inputData; + } + + /// + /// Reports whether is a SecureString this attribute built from a + /// plain string argument, and forgets it either way. A true answer also means the module + /// owns the instance and may dispose it. + /// + internal static bool WasConvertedFromString(SecureString? value) + { + if (value == null || !ConvertedFromString.TryGetValue(value, out _)) + return false; + + ConvertedFromString.Remove(value); + return true; + } + } +} diff --git a/src/PSProxmoxVE/Cmdlets/Connection/ConnectPveServerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Connection/ConnectPveServerCmdlet.cs index f60af2c..c6e910b 100644 --- a/src/PSProxmoxVE/Cmdlets/Connection/ConnectPveServerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Connection/ConnectPveServerCmdlet.cs @@ -1,5 +1,7 @@ using System; using System.Management.Automation; +using System.Runtime.InteropServices; +using System.Security; using PSProxmoxVE.Core.Authentication; namespace PSProxmoxVE.Cmdlets.Connection @@ -39,10 +41,12 @@ namespace PSProxmoxVE.Cmdlets.Connection /// /// Proxmox VE API token in the format USER@REALM!TOKENID=UUID, /// e.g. root@pam!mytoken=xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx. + /// A plain string is still accepted for this release and warns; it is removed in the next major. /// [Parameter(Mandatory = true, ParameterSetName = ParameterSetApiToken, HelpMessage = "API token in USER@REALM!TOKENID=UUID format.")] - [ValidateNotNullOrEmpty] - public string? ApiToken { get; set; } + [ApiTokenTransformation] + [ValidateNotNull] + public SecureString? ApiToken { get; set; } /// When specified, skips TLS certificate validation for the server. [Parameter(Mandatory = false, HelpMessage = "Skip TLS certificate validation.")] @@ -111,10 +115,37 @@ namespace PSProxmoxVE.Cmdlets.Connection case ParameterSetApiToken: { + var moduleOwnsToken = ApiTokenTransformationAttribute.WasConvertedFromString(ApiToken); + if (moduleOwnsToken) + WriteWarning( + "Passing -ApiToken as a plain string is deprecated and will be removed in the next major release. " + + "Pass a SecureString instead, for example (Read-Host -AsSecureString), a token retrieved from a " + + "secret vault, or (ConvertTo-SecureString 'USER@REALM!TOKENID=UUID' -AsPlainText -Force)."); + + if (ApiToken!.Length == 0) + ThrowTerminatingError(new ErrorRecord( + new ArgumentException("API token cannot be empty.", nameof(ApiToken)), + "ApiTokenEmpty", + ErrorCategory.InvalidArgument, + null)); + + string apiToken; + var ptr = Marshal.SecureStringToGlobalAllocUnicode(ApiToken); + try + { + apiToken = Marshal.PtrToStringUni(ptr) ?? string.Empty; + } + finally + { + Marshal.ZeroFreeGlobalAllocUnicode(ptr); + if (moduleOwnsToken) + ApiToken.Dispose(); + } + try { session = PveAuthenticator.AuthenticateWithApiToken( - Server, Port, SkipCertificateCheck.IsPresent, ApiToken!, timeout); + Server, Port, SkipCertificateCheck.IsPresent, apiToken, timeout); } catch (Exception ex) { diff --git a/src/PSProxmoxVE/PSProxmoxVE.dll-Help.xml b/src/PSProxmoxVE/PSProxmoxVE.dll-Help.xml index 84d303d..c0c1251 100644 --- a/src/PSProxmoxVE/PSProxmoxVE.dll-Help.xml +++ b/src/PSProxmoxVE/PSProxmoxVE.dll-Help.xml @@ -708,11 +708,12 @@ ApiToken - API token in USER@REALM!TOKENID=UUID format. + API token in USER@REALM!TOKENID=UUID format, as a SecureString. + Build one with `Read-Host -AsSecureString`, or read it from a secret vault. `ConvertTo-SecureString 'root@pam!mytoken=...' -AsPlainText -Force` also works, but a token written as a literal lands in shell history and in any transcript. A plain string is still accepted in this release and emits a deprecation warning; it is removed in the next major release. - String + SecureString - String + SecureString None @@ -888,11 +889,12 @@ ApiToken - API token in USER@REALM!TOKENID=UUID format. + API token in USER@REALM!TOKENID=UUID format, as a SecureString. + Build one with `Read-Host -AsSecureString`, or read it from a secret vault. `ConvertTo-SecureString 'root@pam!mytoken=...' -AsPlainText -Force` also works, but a token written as a literal lands in shell history and in any transcript. A plain string is still accepted in this release and emits a deprecation warning; it is removed in the next major release. - String + SecureString - String + SecureString None diff --git a/src/PSProxmoxVE/PSProxmoxVE.format.ps1xml b/src/PSProxmoxVE/PSProxmoxVE.format.ps1xml index a0a6044..9f69752 100644 --- a/src/PSProxmoxVE/PSProxmoxVE.format.ps1xml +++ b/src/PSProxmoxVE/PSProxmoxVE.format.ps1xml @@ -1357,7 +1357,6 @@ - PSProxmoxVE.Core.Authentication.PveSession diff --git a/tests/PSProxmoxVE.Core.Tests/Authentication/PveSessionTests.cs b/tests/PSProxmoxVE.Core.Tests/Authentication/PveSessionTests.cs index ab7bbea..5dd8c96 100644 --- a/tests/PSProxmoxVE.Core.Tests/Authentication/PveSessionTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Authentication/PveSessionTests.cs @@ -1,4 +1,6 @@ using System; +using System.Linq; +using System.Reflection; using Xunit; using PSProxmoxVE.Core.Authentication; @@ -184,6 +186,49 @@ namespace PSProxmoxVE.Core.Tests.Authentication Assert.True(session.SkipCertificateCheck); } + [Theory] + [InlineData("ApiToken")] + [InlineData("Ticket")] + [InlineData("CsrfToken")] + public void CredentialProperties_AreNotPubliclyReadable(string propertyName) + { + var property = typeof(PveSession).GetProperty( + propertyName, + BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance); + + Assert.NotNull(property); + Assert.False(property!.GetMethod!.IsPublic); + } + + [Fact] + public void PublicProperties_ExposeNoApiToken() + { + const string token = "root@pam!mytoken=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; + var session = new PveSession(TestHostname, TestPort, false, token); + + Assert.DoesNotContain(RenderPublicProperties(session), v => v.Contains(token)); + } + + [Fact] + public void PublicProperties_ExposeNoTicketOrCsrfToken() + { + const string ticket = "PVE:root@pam:SECRETTICKET"; + const string csrf = "SECRETCSRFTOKEN"; + var session = new PveSession(TestHostname, TestPort, false, "root@pam", ticket, csrf, + DateTime.UtcNow.AddHours(2)); + + var rendered = RenderPublicProperties(session); + Assert.DoesNotContain(rendered, v => v.Contains(ticket)); + Assert.DoesNotContain(rendered, v => v.Contains(csrf)); + } + + private static string[] RenderPublicProperties(PveSession session) => + typeof(PveSession) + .GetProperties(BindingFlags.Public | BindingFlags.Instance) + .Select(p => p.GetValue(session)?.ToString()) + .Where(v => v != null) + .ToArray()!; + [Fact] public void Timeout_DefaultIs100Seconds() { diff --git a/tests/PSProxmoxVE.Tests/Connection/Connect-PveServer.Tests.ps1 b/tests/PSProxmoxVE.Tests/Connection/Connect-PveServer.Tests.ps1 index 3ea4c19..44c9529 100644 --- a/tests/PSProxmoxVE.Tests/Connection/Connect-PveServer.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Connection/Connect-PveServer.Tests.ps1 @@ -35,12 +35,73 @@ Describe 'Connect-PveServer' { Connect-PveServer ` -Server 'pve.example.com' ` -Credential $cred ` - -ApiToken 'root@pam!mytoken=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' ` + -ApiToken (ConvertTo-SecureString 'root@pam!mytoken=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' -AsPlainText -Force) ` -ErrorAction Stop } | Should -Throw } } + Context 'ApiToken accepts a SecureString and warns on a plain string' { + BeforeAll { + # Rejected by the token-format check before any HTTP call, so these cases stay offline. + $script:BadToken = 'not-a-valid-token' + } + + It 'ApiToken should be typed SecureString' { + (Get-Command 'Connect-PveServer').Parameters['ApiToken'].ParameterType | + Should -Be ([System.Security.SecureString]) + } + + It 'Should bind a plain string and warn that it is deprecated' { + $warnings = @() + $err = $null + try { + Connect-PveServer -Server 'pve.example.com' -ApiToken $script:BadToken ` + -ErrorAction Stop -WarningVariable +warnings + } catch { $err = $_ } + $err.FullyQualifiedErrorId | Should -Match 'PveAuthenticationFailed' + ($warnings -join "`n") | Should -Match 'deprecated' + } + + It 'Should not warn about deprecation when given a SecureString' { + $secure = ConvertTo-SecureString $script:BadToken -AsPlainText -Force + $warnings = @() + $err = $null + try { + Connect-PveServer -Server 'pve.example.com' -ApiToken $secure ` + -ErrorAction Stop -WarningVariable +warnings + } catch { $err = $_ } + $err.FullyQualifiedErrorId | Should -Match 'PveAuthenticationFailed' + ($warnings -join "`n") | Should -Not -Match 'deprecated' + } + + It 'Should not warn when a SecureString follows a binding failure that transformed a string' { + $securePass = ConvertTo-SecureString 'hunter2' -AsPlainText -Force + $cred = [System.Management.Automation.PSCredential]::new('root@pam', $securePass) + try { + Connect-PveServer -Server 'pve.example.com' -Credential $cred ` + -ApiToken $script:BadToken -ErrorAction Stop + } catch { } + + $secure = ConvertTo-SecureString $script:BadToken -AsPlainText -Force + $warnings = @() + try { + Connect-PveServer -Server 'pve.example.com' -ApiToken $secure ` + -ErrorAction Stop -WarningVariable +warnings + } catch { } + ($warnings -join "`n") | Should -Not -Match 'deprecated' + } + + It 'Should reject an empty SecureString as an invalid argument' { + $err = $null + try { + Connect-PveServer -Server 'pve.example.com' ` + -ApiToken (New-Object System.Security.SecureString) -ErrorAction Stop + } catch { $err = $_ } + $err.FullyQualifiedErrorId | Should -Match 'ApiTokenEmpty' + } + } + Context 'Parameter metadata' { BeforeAll { $script:Cmd = Get-Command 'Connect-PveServer'