xService: Adding support for Group Managed Service Account #441
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #441 +/- ##
====================================
+ Coverage 72% 72% +<1%
====================================
Files 27 27
Lines 3995 4011 +16
Branches 4 4
====================================
+ Hits 2898 2910 +12
- Misses 1093 1097 +4
Partials 4 4 |
There was a problem hiding this comment.
@danielboth Awesome work on this one! 😃 Just a few review comments.
Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @danielboth)
a discussion (no related file):
There are merge conflicts since your PR was not based on the latest changes. Could you please rebase against branch dev using git rebase (not by using git pull or git merge, to keep the commit history).
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 64 at r1 (raw file):
} $serviceResource = @{
We should evaluate if it is an gSMA and return the the gSMA in the correct property here.
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 80 at r1 (raw file):
{ Write-Verbose -Message ($script:localizedData.ServiceDoesNotExist -f $Name) $serviceResource = @{
We should always return the properties in the returned hash table. Should we return the new property here with $null?
The other properties are not here, but let's start somewhere.
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 249 at r1 (raw file):
($PSBoundParameters.ContainsKey('BuiltInAccount') -and $PSBoundParameters.ContainsKey('GroupManagedServiceAccount')) -or ($PSBoundParameters.ContainsKey('GroupManagedServiceAccount') -and $PSBoundParameters.ContainsKey('Credential')) -or ($PSBoundParameters.ContainsKey('BuiltInAccount') -and $PSBoundParameters.ContainsKey('GroupManagedServiceAccount') -and $PSBoundParameters.ContainsKey('Credential'))
Not sure we need this? The other three checks should cover this, since any two of these three would already be true above?
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 252 at r1 (raw file):
BuiltInAccountAndCredentialSpecified
Can we change the key name to something more appropriate like CredentialParametersAreMutallyExclusive?
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 473 at r1 (raw file):
($PSBoundParameters.ContainsKey('BuiltInAccount') -and $PSBoundParameters.ContainsKey('GroupManagedServiceAccount')) -or ($PSBoundParameters.ContainsKey('GroupManagedServiceAccount') -and $PSBoundParameters.ContainsKey('Credential')) -or ($PSBoundParameters.ContainsKey('BuiltInAccount') -and $PSBoundParameters.ContainsKey('GroupManagedServiceAccount') -and $PSBoundParameters.ContainsKey('Credential'))
Same as a previous comment.
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 1558 at r1 (raw file):
[Parameter()] [String] $GroupManagedServiceAccount,
Please add this parameter to the comment-based help.
Examples/xService_CreateServiceConfigGroupManagedServiceAccount.ps1, line 2 at r1 (raw file):
<#PSScriptInfo .VERSION 1.0.1
Change version to 1.0.0.
Examples/xService_CreateServiceConfigGroupManagedServiceAccount.ps1, line 3 at r1 (raw file):
22b2e2d1-76cd-4f4c-b952-ec3b574b9751
Please update to an unique GUID (easiest by New-Guid). This is the unique GUID when publishing to PowerShell Gallery.
Examples/xService_CreateServiceConfigGroupManagedServiceAccount.ps1, line 41 at r1 (raw file):
='Service1'
Can we add space after the equal sign?
Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 506 at r1 (raw file):
&
Can we write 'and' instead?
Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 557 at r1 (raw file):
&
Can we write 'and' instead?
Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 608 at r1 (raw file):
&
Can we write 'and' instead?
johlju
added
waiting for code fix
There was a problem hiding this comment.
Reviewable status: 4 of 7 files reviewed, 13 unresolved discussions (waiting on @johlju and @danielboth)
a discussion (no related file):
Previously, johlju (Johan Ljunggren) wrote…
There are merge conflicts since your PR was not based on the latest changes. Could you please rebase against branch dev using
git rebase(not by usinggit pullorgit merge, to keep the commit history).
Done.
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 64 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We should evaluate if it is an gSMA and return the the gSMA in the correct property here.
Both BuiltInAccount and Credential output the user used to run the service to the BuiltinAccount property, so I followed the same logic with the gMSA addition. Evaluating if the account is a gMSA would mean doing a call to active directory to verify, as the property returned by the ciminstance is just a string. In a production scenario that would mean that every xService test will check with AD every 15 minutes if the account is a gMSA. I'm not in favor of doing that, so I would like to keep the current approach of just returning the user in the BuiltInAccount property.
What might be better in the future could be replacing the BuiltinAccount, Credential and GroupManagedServiceAccount parameters with three new parameters: User, UserType and Password. UserType can then be specified by the user on what kind of account is used. That would be a breaking change though.
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 80 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We should always return the properties in the returned hash table. Should we return the new property here with $null?
The other properties are not here, but let's start somewhere.
I don't see the value of returning the GroupManagedServiceAccount with $null, what's the upside here?
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 249 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Not sure we need this? The other three checks should cover this, since any two of these three would already be true above?
You're right, removed.
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 252 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
BuiltInAccountAndCredentialSpecifiedCan we change the key name to something more appropriate like
CredentialParametersAreMutallyExclusive?
Fixed, parameter renamed
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 473 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Same as a previous comment.
Removed
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 1558 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add this parameter to the comment-based help.
Done.
Examples/xService_CreateServiceConfigGroupManagedServiceAccount.ps1, line 2 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Change version to 1.0.0.
Done.
Examples/xService_CreateServiceConfigGroupManagedServiceAccount.ps1, line 3 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
22b2e2d1-76cd-4f4c-b952-ec3b574b9751Please update to an unique GUID (easiest by
New-Guid). This is the unique GUID when publishing to PowerShell Gallery.
Done.
Examples/xService_CreateServiceConfigGroupManagedServiceAccount.ps1, line 41 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
='Service1'Can we add space after the equal sign?
Done.
Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 506 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
&Can we write 'and' instead?
Done.
Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 557 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
&Can we write 'and' instead?
Done.
Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 608 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
&Can we write 'and' instead?
Done.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johlju)
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 64 at r1 (raw file):
Previously, danielboth (Daniël) wrote…
Both BuiltInAccount and Credential output the user used to run the service to the BuiltinAccount property, so I followed the same logic with the gMSA addition. Evaluating if the account is a gMSA would mean doing a call to active directory to verify, as the property returned by the ciminstance is just a string. In a production scenario that would mean that every xService test will check with AD every 15 minutes if the account is a gMSA. I'm not in favor of doing that, so I would like to keep the current approach of just returning the user in the BuiltInAccount property.
What might be better in the future could be replacing the BuiltinAccount, Credential and GroupManagedServiceAccount parameters with three new parameters: User, UserType and Password. UserType can then be specified by the user on what kind of account is used. That would be a breaking change though.
Thank you for the detailed explanation. I understand, that is not desired.
-
I like your suggestion with the new parameters. Could you please submit an issue for that so that could be implemented in another PR?
-
I think we should update the documentation for the resource, mentioning that the credential is always returned in the BuiltInAccount regardless if using Credential or GroupManagedServiceAccount parameter.
-
The first section of your comment, could you add that as `.NOTES to the comment-based help? It would help other contributors to know why its done this way.
Also, as per next (non-blocking) review comment, we should always return the GroupManagedServiceAccount with $null. Although I now understand why the credential property is not returned here either. So, in light of your suggestion of the new parameters, I think we skip that.
Do you agree that this comment (and the next) is resolved when item 1-3 above is resolved?
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 80 at r1 (raw file):
This is non-blocking as per previous comment.
The upside is that the hash table always contains the key, even if it set to $null, so that when using Get-DscConfiguration the property is shown and can be evaluated.
In the Get-TargetResource ... This function must return a hash table that lists all the resource properties as keys and the actual values of these properties as the corresponding values.
https://docs.microsoft.com/en-us/powershell/dsc/authoringresourcemof#writing-the-resource-script
There was a problem hiding this comment.
Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @johlju and @danielboth)
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 64 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Thank you for the detailed explanation. I understand, that is not desired.
I like your suggestion with the new parameters. Could you please submit an issue for that so that could be implemented in another PR?
I think we should update the documentation for the resource, mentioning that the credential is always returned in the BuiltInAccount regardless if using Credential or GroupManagedServiceAccount parameter.
The first section of your comment, could you add that as `.NOTES to the comment-based help? It would help other contributors to know why its done this way.
Also, as per next (non-blocking) review comment, we should always return the GroupManagedServiceAccount with $null. Although I now understand why the credential property is not returned here either. So, in light of your suggestion of the new parameters, I think we skip that.
Do you agree that this comment (and the next) is resolved when item 1-3 above is resolved?
- Done, created a new issue xService: Rename BuiltinAccount, Credential and GroupManagedServiceAccount parameters #442 which describes the need to rename the parameters.
- Documentation updated
- Added to the .NOTES section of Get-TargetResource (as that is the function returning these properties)
DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 80 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
This is non-blocking as per previous comment.
The upside is that the hash table always contains the key, even if it set to $null, so that when using Get-DscConfiguration the property is shown and can be evaluated.
In the Get-TargetResource ... This function must return a hash table that lists all the resource properties as keys and the actual values of these properties as the corresponding values.
https://docs.microsoft.com/en-us/powershell/dsc/authoringresourcemof#writing-the-resource-script
Ok, this can be properly resolved once #442 is fixed.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
|
@danielboth I wanted to run this by you before I merge this PR. Please see this PowerShell/PSDscResources#108 (comment), would it be possible to use the same technique adding a gMSA to the Credential property? I just want to rule it out, so we don't add something that is more overly complicated. Even if it do work, maybe this proposal has more advantages anyway? Appreciate your input on this one, thank you! |
johlju
added
waiting for author response
|
Hi @johlju, it's definitely possible to add the gMSA in the same way in the Credential parameter. It would even be easier to add that way, since then we could just verify in the code that when the password is empty, we don't pass it forward to the CimMethod that set's the service properties. Could easily become: if(-not([string]::IsNullOrEmpty($Credential.GetNetworkCredential().Password))){
$changeServiceArguments['StartPassword'] = $Credential.GetNetworkCredential().Password
}Now this has also one (big if you ask me) downside. That is the fact that DSC, even though we are passing an empty password, requires us to enable plaintext password support by setting PSDscAllowPlainTextPassword in the configuration for the node. Try explaning an auditor that in order to use more secure accounts, we need to enable plaintext password support. I would also like to avoid enabling my configurations to save plaintext passwords. That's why I opted for the separate parameter instead. |
|
@danielboth This sounds like a much better approach then adding a separate property for this. And since I'm in favor of changing this to your suggestion, using the |
|
I stil feel that putting in a Group Managed Service Account in a credential object is not the right way. This because we cannot create a credential object for a gMSA, we can just define the user itself and the user itself is just a simple string, nothing more. So why claim you expect a credential when what you are really asking for is the username? Sounds a bit misleading and not intuitive at all. I also see your point about implementing an extra parameter just for this, which might add a bit too much overhead on the codebase. I agree there. What about renaming the current BuiltInAccount parameter to RunAsUser? That property could then be used both for the builtin users (LocalSystem, NetworkService) and for Group Managed Service Accounts. That way, if it's not LocalSystem or NetworkService we assume that the user provides us with a Group Managed Service Account. We will still support the Credential parameter for any other accounts. Would that be acceptable? |
|
I agree that using credential object can be misleading. Renaming built in account sounds like a good option, but would make use remove the ValidateSet, making users potentially spell those built in account wrong (really minor issue though :), and it would be a breaking change if we rename it. Honestly, In this case, and since there are only two opinions in this PR, I will weight the opinion of the PR author higher than my opinion. @danielboth If we disregard that any option need more work, or breaking change, what option do you like the best? Let's go with that. |
|
Hi @johlju, in that case, I would not mind getting this PR merged in it current state. This way, we don't do any breaking changes, it's very clear for the users of the resource Group Managed Service Accounts are fully supported (it even might help a few people to consider that option) and we don't hide it behind a somewhat related parameter. The cost, a slightly bigger codebase, is worth this if you ask me. |
|
I read the whole thread and if I would have to choose (in order of preference) I'd go with:
I can't wait to start using it for that purpose within DSC, for now we have simple resources that enable us to work around the problem of the missing proper support, so the more, the merrier... |
|
@danielboth sounds good. |
|
The CLA bot is waiting for CLA to be signed, but that should be already signed (as per other PR's). Closing and directly open this PR to kick off the CLA bot again. |
johlju
removed
the
waiting for author response
There was a problem hiding this comment.
Look good to be. We can create integration tests from this change since there are no Active Directory available in the build worker.
Reviewable status:
|
@danielboth This is merged now! Thank you for this, and the great discussion around this! 😃 |
Pull Request (PR) description
Add support for Group Managed Service Accounts to xService. This enables the xService DSC resource to setup the runas account for services using a Group Managed Service Account (GMSA). This pull request adds support and unit tests for the gMSA setup.
This Pull Request (PR) fixes the following issues
No open issues for the missing support of gMSA's on xService
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
Integration tests for the GroupManagedServiceAccount were not added as the appveyor build environment does not run in domain context, so integration testing gMSA's is not possible.
This change is