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

How to properly handle SID translation failures #78

Closed
jcwalker opened this issue Jan 3, 2018 · 22 comments · May be fixed by #161
Closed

How to properly handle SID translation failures #78

jcwalker opened this issue Jan 3, 2018 · 22 comments · May be fixed by #161
Labels
stale The issue or pull request was marked as stale because there hasn't been activity from the community.

Comments

@jcwalker
Copy link
Contributor

jcwalker commented Jan 3, 2018

I would like to get feedback from the community on the best way to handle SID translation exceptions. Currently the resource will throw an exception if a SID cannot be translated. There have been requests/suggestions that this behavior should be changed to silently handle the exception and have Test-TargetResource return false. In both cases the configuration will need to be updated. The way the resource currently works makes it obvious a correction needs to be made. If the resource is changed to silently handle the exception and return false the administrator will not know anything is wrong until they validate the configuration was successful. Is there anything else that I'm missing here? What are your thoughts?

@hackjammer
Copy link
Contributor

I would prefer a Write-Warning alongside returning false - as of right now, throwing during Test-TargetResource means that Set doesn't run - which is more problematic. There are many examples of cases where SIDs will not get translated where nothing needs to be explicitly fixed (e.g. temporary accounts) - not applying the configuration is bad, reapplying with a warning is far better.

@jcwalker
Copy link
Contributor Author

jcwalker commented Feb 9, 2018

I would argue that something does need to get explicitly fixed. Even if the we write a warning and return false the node will not be in compliance until the configuration script is updated and the identity that cannot be resolved is removed or corrected.

@danielboth
Copy link
Member

I would be ok with throwing an exception if the SID that could not be translated is in the part of the policy I actually want to configure. The problem is that with the current implementation all SIDs are translated, even in parts that I did not describe in the DSC configuration. That feels just wrong.

Also we might needs some exclusion list? In my case, I'm having failures on this resource since IIS is installed on the box and the " Classic .NET AppPool " identity cannot be resolved. So that's one that probably never will work.

When thinking about it, we are configuring a desired state here so let's take this config:

UserRightsAssignment LogonAsAService {
    Policy = 'Log_on_as_a_service'
    Identity = 'DOMAIN\MyCustomGroup'
    Ensure = 'Present'
}

What do we need to do in this case? We need to resolve the identity for MyCustomGroup and ensure that it is present in the Log_on_as_a_service policy. Nothing more, I don't care about any lingering identities that cannot be resolved, I care about what I've described in my DSC configuration. So if we cannot resolve the identity for MyCustomGroup we should throw an exception, but not on other, unrelated, identities.

@jcwalker, does this help? It would be nice if this issue could be solved in the next release!

@johlju
Copy link
Member

johlju commented May 18, 2018

I agree with @danielboth that it is the configuration that is important. If the SID's cannot be resolved that is not part of the configuration then that is not a big deal, although if a SID cannot be resolved that are in the configuration that means that the Set-TargetResource function will always run.

There are a suggestion here in the #74 (comment). I'm also curious what scenarios this happens in, so this can be regression tested, see #74 (comment).

For reference: PR #65, #72 and #74 is dependent on this discussion

@johlju
Copy link
Member

johlju commented May 18, 2018

@jcwalker do you have suggestion to how we move forward with this issue?

@jcwalker
Copy link
Contributor Author

@johlju I've talked with a lot of folks on the best way to handle this. I think that Test-TargetResource needs to run without error and return false that way someone can run Test-DscConfiguration and it will actually complete. There are a lot of issue where folks report the resource is not propertly resolving SIDs that I can't reproduce. Further investigation on why SID resolution fails is needed and document when credentials are needed for resolution to be successful, such as an identity from a different domain.

@danielboth
Copy link
Member

@jcwalker All the SID resolution issues that I've seen so far have not been caused by what we described in the DSC configuration. All were caused by other not SIDs that could not be resolved. Like non existing accounts or the Classic .NET AppPool I described. However, I don't care about those at all.

Is it possible to just focus the resource at what the user intents to have in desired state and ignore all the other errors?

@jcwalker
Copy link
Contributor Author

@danielboth you are the first one to bring up this scenario and I'm puzzled why it's evening happening because Test-TargetResource and Set-TargetResource only handle the policies being managed. But I think what maybe happening is when the code gets the current configuration it grabs all the policies. I will look into that scenario. I think if we make the SID conversion process not throw it would fix it also.

@pjolsen
Copy link

pjolsen commented May 20, 2018

Hi jcwalker,

I've just raised an issue regarding exactly that issue. I have seen multiple machines erroring out due to unresolved SIDs being present in user rights that are not managed through DSC. The target user rights are clean (all SIDs resolving properly) but it seems that function "Get-UserRightsAssignment" doesn't filter to the specific user right to be targeted and attempts to resolve everything in the secedit.exe /export file.

@pjolsen
Copy link

pjolsen commented May 20, 2018

Sorry - I think the code may actually be required in the Get-SecurityPolicy policy function. I think it should be as simple as having an extra parameter (e.g. $userright) passed into the get-securitypolicy function that holds the specific user right being targeted and then modifying the code with a where statement similiar to the below:

foreach ($key in ($privilegeRights.keys | where {$_ -eq $userright})

@johlju
Copy link
Member

johlju commented May 21, 2018

For reference. With the new feature in the test framework it is possible to run tests in a container, so if there are problem in server core of nano server that might be able to be tested by running integration tests in a container. See https://github.com/PowerShell/DscResource.Tests#run-integration-tests-in-order. If this could be

@rdbartram
Copy link

I agree with the premise that if the state cannot be confirmed i.e an exception is thrown, then it really should halt operation as we cannot tell if we should proceed with dependant configurations or not. My recommendation would be to have a memberstoinclude memberstoremove type property this way, if we cannot be sure that that the required user is/is not in the list then we fail otherwise we silently continue with warning. The property should also support passing SIDs so we can remove/add Unresolvable SIDs

@jcwalker
Copy link
Contributor Author

@rdbartram we have the membersToInclude membersToExclude type functionality with the Force parameter.

I am currenlty working on a solution that adds a Scope parameter to the ConvertTo-LocalFriendlyName function so when Get/Test-TargetResource is calling ConvertTo-LocalFriendlyName it will use Scope = 'Get' and the name conversion will not throw a terminiating error if a SID can't be translated. However, if Set-TargetResource is calling ConvertTo-LocalFriendlyName it will have Scope='SET' and then we have to throw a terminating error.

This will solve the issue of orphaned SIDs on policies that are not in the configuration stopping the configuration process because they can't be resolved. Additionally if an orphaned SID is applied to a policy that is managed by DSC, Test-TargetResource will report it as being in an undesired state.

Thoughts?

@johlju
Copy link
Member

johlju commented May 22, 2018

What happens when the Test-function tests for desired state, and cannot resolve a SID, and that identity is in the configuration? Will it always return $false?

@jcwalker
Copy link
Contributor Author

@johlju if an orphaned SID is assigned to a policy and also in the desired configuration it will return $true. However, for an identity to be applied successfully the SID has to be able to be translated or Test-TargetResource will throw.

jcwalker added a commit that referenced this issue May 22, 2018
@johlju
Copy link
Member

johlju commented May 22, 2018

Sounds like you got a solution. Great work!

@stale
Copy link

stale bot commented Jun 21, 2018

This issue has been automatically marked as stale because it has not had activity from the community in the last 30 days. It will be closed if no further activity occurs within 10 days. If the issue is labelled with any of the work labels (e.g bug, enhancement, documentation, or tests) then the issue will not auto-close.

@stale stale bot added the stale The issue or pull request was marked as stale because there hasn't been activity from the community. label Jun 21, 2018
@mjcarrabine
Copy link

Issue #87 Unresolved SIDs in unmanaged user rights cause a failure was closed as a duplicate of this issue so I am posting this here, but I don't think it is being addressed here completely.

Unresolved SIDs in Managed Policies

I agree with @johlju and @jcwalker above regarding how to handle orphaned SIDs on policies in your desired state configuration.

Unresolved SIDs in Unmanaged Policies

Using version 2.3 on Windows Server 2016, I am applying 30 identical UserRightsAssignment resources to 6 servers. On 5 servers, all resources are compliant. However, on 1 server all 30 resources were failing with:

PowerShell DSC resource MSFT_UserRightsAssignment failed to execute Test-TargetResource functionality with error message: Could not convert Identity: BUILTIN to SID

I finally tracked the root cause down to an invalid SID translation occurring in a policy that isn't even in my desired state configuration. The installer for our ERP software had added BUILTIN to the Log on as a service policy.

Removing this invalid account from the policy resulted in all 30 resources being compliant.

Ideally, I think the resource should only try to resolve issues with SIDs in policies that are in the desired state configuration. But, if not, I think any exceptions translating these SIDs should not result in a failing state.

@stale stale bot removed the stale The issue or pull request was marked as stale because there hasn't been activity from the community. label Jun 25, 2018
@stale
Copy link

stale bot commented Jul 25, 2018

This issue has been automatically marked as stale because it has not had activity from the community in the last 30 days. It will be closed if no further activity occurs within 10 days. If the issue is labelled with any of the work labels (e.g bug, enhancement, documentation, or tests) then the issue will not auto-close.

@stale stale bot added the stale The issue or pull request was marked as stale because there hasn't been activity from the community. label Jul 25, 2018
@stale
Copy link

stale bot commented Sep 3, 2018

This issue has been automatically closed because it is has not had activity from the community in the last 40 days.

@husni-nilar
Copy link

husni-nilar commented Jan 23, 2024

Unresolved SIDs in Unmanaged Policies

Using version 2.3 on Windows Server 2016, I am applying 30 identical UserRightsAssignment resources to 6 servers. On 5 servers, all resources are compliant. However, on 1 server all 30 resources were failing with:

PowerShell DSC resource MSFT_UserRightsAssignment failed to execute Test-TargetResource functionality with error message: Could not convert Identity: BUILTIN to SID

I finally tracked the root cause down to an invalid SID translation occurring in a policy that isn't even in my desired state configuration. The installer for our ERP software had added BUILTIN to the Log on as a service policy.

Removing this invalid account from the policy resulted in all 30 resources being compliant.

Ideally, I think the resource should only try to resolve issues with SIDs in policies that are in the desired state configuration. But, if not, I think any exceptions translating these SIDs should not result in a failing state.

@mjcarrabine I am facing this same issue; can you let me know how and where you removed the invalid account from?

@pjolsen
Copy link

pjolsen commented Jan 23, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale The issue or pull request was marked as stale because there hasn't been activity from the community.
Projects
None yet
8 participants