microsoft / VSTSAgent.PowerShell

Tools for managing and automating your Azure DevOps Agents.
MIT License
29 stars 23 forks source link

Support gmsa when installing agent #44

Open tristanbarcelon opened 9 months ago

tristanbarcelon commented 9 months ago

@jwittner, Install-VSTSAgent function currently supports installation of agents as a Windows service when LogonCredential parameter is set. We would like to deploy it without needing to provide passwords. GMSAs provide the ability to run Windows services without having to provide/rotate passwords and agent now supports it since this update. I'd like to propose adding this feature to Install-VSTSAgent but before that, would prefer to discuss how to do so with maintainers.

jwittner commented 9 months ago

Is it as straightforward as not passing --windowsLogonPassword when calling with a GMSA? We could check where we append the argument here for e.g. an empty string and not pass the argument. So users would create a credential with the user name and an empty password.

tristanbarcelon commented 9 months ago

@jwittner, I tried creating a PSCredential object with an empty string as password and convertto-securestring disallows it, resulting in a null $LogonCredential parameter.

image

Looking at the existing logic below, a non-null $LogonCredential parameter is utilized for detecting intent to run as service by specifying --windowsLogonAccount and --windowsLogonPassword.

image

This feels really icky, but what if we supply a new optional switch parameter $UseGMSA? This will work in conjunction with $LogonParameter and only use the UserName property of $LogonParameter when UseGMSA is also true or specified. Conversely, --windowsLogonPassword will only be specified if UseGMSA is NOT specified or false. Only this block will be modified. Unfortunately, securestring password of the account will still be required even if it is set to an invalid value like a guid string just to have a valid non-null $LogonCredential.

if ( $LogonCredential ) {
    if (($PSBoundParameters.ContainsKey('UseGMSA') -or $UseGMSA)
    {
        $configArgs += '--windowsLogonPassword', `
        [System.Net.NetworkCredential]::new($null, $LogonCredential.Password).Password
    }
}

An alternative to the introduction of $UseGMSA switch parameter would be a new and an optional $GMSA string parameter. If this is not null or empty, then we ignore the $LogonCredential parameter and set value of --windowsLogonAccount equal to value of $GMSA. It might warrant setting different ParameterSet combinations so specifying GMSA precludes the need to specify LogonParameter to make Install-VSTSAgent intuitive.

jwittner commented 9 months ago

This Stack Overflow has a suggestion that worked for me.

$mycreds = New-Object System.Management.Automation.PSCredential("username", (New-Object System.Security.SecureString))
tristanbarcelon commented 9 months ago

Thanks @jwittner. Doesn't look like we'll need any new parameter to use GMSA but simply a new documented way to create $LogonCredential object to signal intent to use GMSA when calling Install-VSTSAgent. Are you ok with a change like this?

if ($LogonCredential ) {
    $NetworkCredential = [System.Net.NetworkCredential]::new($null, $LogonCredential.Password)
    if (-not [string]::IsNullOrEmpty($NetworkCredential.Password))
    {
        $configArgs += '--windowsLogonPassword', $NetworkCredential.Password
    }
}
jwittner commented 9 months ago

Pretty close, but we don't need to convert to test the length, and there's a handy GetNetworkCredential on PSCredential. Maybe this?

if ($LogonCredential -and $LogonCredential.Password.Length -gt 0) {
    $configArgs += '--windowsLogonPassword', $LogonCredential.GetNetworkCredential().Password
}
tristanbarcelon commented 9 months ago

@jwittner, would you be ok if I combine checks involving $LogonCredential? For example, instead of making change as suggested above:

image

I place them closer together as shown below:

image

I'd like to add Write-Verbose statements so I could tell which way the agent is being configured and consolidating them in this block makes code easier to read.

jwittner commented 9 months ago

That change would lose the print of the logon account, valuable information, in the What-If and -Verbose operations. The split is mainly to add the password after printing.

I like the verbose statements you're adding though - might still test the password where the account is added to configArgs and do that, but leave adding the password to the configArgs as a second test below the call to ShouldProcess, something akin to my suggestion above.