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.

Sign up for GitHub

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

Jump to bottom

ADManagedServiceAccount: Proposed Breaking Change #515

Closed
X-Guardian opened this issue Oct 5, 2019 · 2 comments · Fixed by #517
Closed

ADManagedServiceAccount: Proposed Breaking Change #515

X-Guardian opened this issue Oct 5, 2019 · 2 comments · Fixed by #517
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request.
Projects
All issues and PR's

Comments

@X-Guardian
Copy link
Contributor

I am currently working on the ADManagedServiceAccount resource to fix issues #511 and #512. Going through the code, there are other issues with it which I am taking the opportuntity to fix, including the following breaking changes:

Breaking:

  • AccountType parameter ValidateSet changed from ('Group', 'Single') to ('Group', 'Standalone') - Standalone is the correct terminology. ref: https://docs.microsoft.com/en-us/windows/security/identity-protection/access-control/service-accounts.
  • AccountType parameter default of Single removed. - Enforce positive choice of account type.
  • MembershipAttribute parameter ValidateSet member SID changed to ObjectSid to match result property of Get-AdObject. Current code does not work if SID is specified.
  • AccountTypeForce parameter removed - unnecessary complication. With the current code for this in place, if the AccountType is changed without specifying this parameter, the change will not apply, but the DSC will still complete successfully. A warning is written to the log, but unless you are looking for it you won't see it. I don't think this is a good model, and none of the other resources use it.
  • Renamed Members parameter to ManagedPasswordPrincipals - to closer match Get-AdServiceAccount result property PrincipalsAllowedToRetrieveManagedPassword. This is so that a DelegateToAccountPrincipals parameter can be added later.

I also intend to subsequently add the following missing parameters to the resource that are directly supported by the ADServiceAaccount PowerShell cmdlets:

  • AccountExpirationDate
  • AccountNotDelegated
  • CompoundIdentitySupported
  • DelegateToAccountPrincipals (PrincipalsAllowedToDelegateToAccount)
  • HomePage
  • ServicePrincipalNames
  • TrustedForDelegation
  • Enabled

Any comments?
@johlju
Copy link
Member

johlju commented Oct 5, 2019

Just wondering if we remove $AccountTypeForce how does the user opt-in to change a standalone to group or vice verse?
I think the opt-in was meant so that an existing account couldn't be changed by mistake. It would be more proper for the resource to throw if it needs to change the type but is not opt-in. 🤔

@johlju johlju added breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community. labels Oct 5, 2019
@X-Guardian
Copy link
Contributor Author

Throwing in this scenario would be better than the current code, but do we really need to protect against this? Part of the reason that I'm proposing to remove the default on AccountType is so that you are always making a positive choice in the first place.

@X-Guardian X-Guardian mentioned this issue Oct 6, 2019
Merged
10 tasks
@johlju johlju added this to To do in All issues and PR's Oct 9, 2019
@johlju johlju added in progress The issue is being actively worked on by someone. and removed help wanted The issue is up for grabs for anyone in the community. labels Nov 2, 2019
All issues and PR's automation moved this from To do to Done Nov 2, 2019
johlju pushed a commit that referenced this issue Nov 2, 2019
@X-Guardian @johlju
BREAKING CHANGE: ADManagedServiceAccount: Add KerberosEncryptionType …
7df133d 
…Property and Refactor (#517)

- Changes to ADManagedServiceAccount
  - KerberosEncryptionType property added (issue #511).
  - BREAKING CHANGE: AccountType parameter ValidateSet changed from ('Group', 'Single') to ('Group', 'Standalone') - Standalone is the correct terminology (issue #515).
  - BREAKING CHANGE: AccountType parameter default of Single removed. - Enforce positive choice of account type.
  - BREAKING CHANGE: MembershipAttribute parameter ValidateSet member SID changed to ObjectSid to match result property of Get-AdObject. Previous code does not work if SID is specified.
  - BREAKING CHANGE: AccountTypeForce parameter removed - unnecessary complication.
  - BREAKING CHANGE: Members parameter renamed to ManagedPasswordPrincipals - to closer match Get-AdServiceAccount result property PrincipalsAllowedToRetrieveManagedPassword. This is so that a DelegateToAccountPrincipals parameter can be added later.
  - Common Compare-ResourcePropertyState function used to replace function specific Compare-TargetResourceState and code refactored (issue #512).
  - Resource unit tests refactored to use nested contexts and follow the logic of the module.
  - Resource Integration tests added.
@johlju johlju removed the in progress The issue is being actively worked on by someone. label Nov 2, 2019
X-Guardian added a commit that referenced this issue Jan 10, 2020
@X-Guardian
BREAKING CHANGE: ADManagedServiceAccount: Add KerberosEncryptionType …
a1526c2 
…Property and Refactor (#517)

- Changes to ADManagedServiceAccount
  - KerberosEncryptionType property added (issue #511).
  - BREAKING CHANGE: AccountType parameter ValidateSet changed from ('Group', 'Single') to ('Group', 'Standalone') - Standalone is the correct terminology (issue #515).
  - BREAKING CHANGE: AccountType parameter default of Single removed. - Enforce positive choice of account type.
  - BREAKING CHANGE: MembershipAttribute parameter ValidateSet member SID changed to ObjectSid to match result property of Get-AdObject. Previous code does not work if SID is specified.
  - BREAKING CHANGE: AccountTypeForce parameter removed - unnecessary complication.
  - BREAKING CHANGE: Members parameter renamed to ManagedPasswordPrincipals - to closer match Get-AdServiceAccount result property PrincipalsAllowedToRetrieveManagedPassword. This is so that a DelegateToAccountPrincipals parameter can be added later.
  - Common Compare-ResourcePropertyState function used to replace function specific Compare-TargetResourceState and code refactored (issue #512).
  - Resource unit tests refactored to use nested contexts and follow the logic of the module.
  - Resource Integration tests added.
johlju pushed a commit to johlju/ActiveDirectoryDsc that referenced this issue Jan 10, 2020
@X-Guardian @johlju
BREAKING CHANGE: ADManagedServiceAccount: Add KerberosEncryptionType …
4ab6e1b 
…Property and Refactor (dsccommunity#517)

- Changes to ADManagedServiceAccount
  - KerberosEncryptionType property added (issue dsccommunity#511).
  - BREAKING CHANGE: AccountType parameter ValidateSet changed from ('Group', 'Single') to ('Group', 'Standalone') - Standalone is the correct terminology (issue dsccommunity#515).
  - BREAKING CHANGE: AccountType parameter default of Single removed. - Enforce positive choice of account type.
  - BREAKING CHANGE: MembershipAttribute parameter ValidateSet member SID changed to ObjectSid to match result property of Get-AdObject. Previous code does not work if SID is specified.
  - BREAKING CHANGE: AccountTypeForce parameter removed - unnecessary complication.
  - BREAKING CHANGE: Members parameter renamed to ManagedPasswordPrincipals - to closer match Get-AdServiceAccount result property PrincipalsAllowedToRetrieveManagedPassword. This is so that a DelegateToAccountPrincipals parameter can be added later.
  - Common Compare-ResourcePropertyState function used to replace function specific Compare-TargetResourceState and code refactored (issue dsccommunity#512).
  - Resource unit tests refactored to use nested contexts and follow the logic of the module.
  - Resource Integration tests added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request.
Projects
All issues and PR's
  
Done
Milestone
No milestone
2 participants
@johlju @X-Guardian