ansible-collections / community.windows

Windows community collection for Ansible
https://galaxy.ansible.com/community/windows
GNU General Public License v3.0
199 stars 152 forks source link

Add GUID mapping to fix AuditPol localization issue #398

Closed clcaldwell closed 2 years ago

clcaldwell commented 2 years ago
SUMMARY

Added a static GUID - English Name mapping for the outputs from auditpol.exe, to get around an issue caused by auditpol's outputs are localized.

Fixes #14

ISSUE TYPE
COMPONENT NAME

win_audit_policy_system

ADDITIONAL INFORMATION

This is the same method used to fix the same localization problem is the AuditPolicyDSC DSC resource: https://github.com/dsccommunity/AuditPolicyDsc/blob/03bd14a560be01a50222f1dfad739c914043b124/DSCResources/AuditPolicyResourceHelper/AuditPolicyResourceHelper.psm1#L249

jborean93 commented 2 years ago

I'm not sure hardcoding in a list of categories and sub categories is the best idea here. IMO this module should accept 3 forms for the category and subcategory options:

I'm not 100% convinced of the 3rd one but I'm not against it. I can see how it could be convenient to have an Ansible play with the English label that works across any system but I do think we should still accept the localized variant and only have the English value as a final fallback.

As for the localization issue with auditpol it seems the better method would be to map all human readable labels of the category and subcategory to the GUID and call the auditpol commands with the GUID specified rather than the label. This keeps the commands relative simple as we don't need to quote the value and it avoids any localization problems.

To get a list of all categories and subcategories, including their localized names you can use the following code that utilities PInvoke to call the relevant Win32 function.

Click to expand! ```powershell Add-Type -TypeDefinition @' using System; using System.ComponentModel; using System.Runtime.InteropServices; namespace Community.Windows.WinAuditPolicySystem { public class Native { [DllImport("Advapi32.dll")] private static extern void AuditFree(IntPtr Buffer); [DllImport("Advapi32.dll", EntryPoint = "AuditEnumerateCategories", SetLastError = true)] [return: MarshalAs(UnmanagedType.I1)] private static extern bool NativeAuditEnumerateCategories( out IntPtr ppAuditCategoriesArray, out int pdwCountReturned); public static Guid[] AuditEnumerateCategories() { IntPtr catArray = IntPtr.Zero; int catCount = 0; if (!NativeAuditEnumerateCategories(out catArray, out catCount)) { throw new Win32Exception(); } try { IntPtr guidPtr = catArray; Guid[] categories = new Guid[catCount]; for (int i = 0; i < catCount; i++) { categories[i] = (Guid)Marshal.PtrToStructure(guidPtr, typeof(Guid)); guidPtr = IntPtr.Add(guidPtr, Marshal.SizeOf(typeof(Guid))); } return categories; } finally { AuditFree(catArray); } } [DllImport("Advapi32.dll", EntryPoint = "AuditEnumerateSubCategories", SetLastError = true)] [return: MarshalAs(UnmanagedType.I1)] private static extern bool NativeAuditEnumerateSubCategories( ref Guid pAuditCategoryGuid, [MarshalAs(UnmanagedType.I1)] bool bRetrieveAllSubCategories, out IntPtr ppAuditCategoriesArray, out int pdwCountReturned); public static Guid[] AuditEnumerateSubCategories(Guid auditCategory) { IntPtr catArray = IntPtr.Zero; int catCount = 0; if (!NativeAuditEnumerateSubCategories(ref auditCategory, false, out catArray, out catCount)) { throw new Win32Exception(); } try { IntPtr guidPtr = catArray; Guid[] categories = new Guid[catCount]; for (int i = 0; i < catCount; i++) { categories[i] = (Guid)Marshal.PtrToStructure(guidPtr, typeof(Guid)); guidPtr = IntPtr.Add(guidPtr, Marshal.SizeOf(typeof(Guid))); } return categories; } finally { AuditFree(catArray); } } [DllImport("Advapi32.dll", CharSet = CharSet.Unicode, SetLastError = true)] [return: MarshalAs(UnmanagedType.I1)] private static extern bool AuditLookupCategoryNameW( ref Guid pAuditCategoryGuid, out IntPtr ppszCategoryName); public static string AuditLookupCategoryName(Guid category) { IntPtr namePtr = IntPtr.Zero; if (!AuditLookupCategoryNameW(ref category, out namePtr)) { throw new Win32Exception(); } try { return Marshal.PtrToStringUni(namePtr); } finally { AuditFree(namePtr); } } [DllImport("Advapi32.dll", CharSet = CharSet.Unicode, SetLastError = true)] [return: MarshalAs(UnmanagedType.I1)] private static extern bool AuditLookupSubCategoryNameW( ref Guid pAuditCategoryGuid, out IntPtr ppszCategoryName); public static string AuditLookupSubCategoryName(Guid category) { IntPtr namePtr = IntPtr.Zero; if (!AuditLookupSubCategoryNameW(ref category, out namePtr)) { throw new Win32Exception(); } try { return Marshal.PtrToStringUni(namePtr); } finally { AuditFree(namePtr); } } } } '@ [Community.Windows.WinAuditPolicySystem.Native]::AuditEnumerateCategories() | ForEach-Object { $category = $_ $name = [Community.Windows.WinAuditPolicySystem.Native]::AuditLookupCategoryName($category) $subCategories = [Community.Windows.WinAuditPolicySystem.Native]::AuditEnumerateSubCategories($_) | ForEach-Object { $subName = [Community.Windows.WinAuditPolicySystem.Native]::AuditLookupSubCategoryName($_) [PSCustomObject]@{ Name = $subName Id = $_ } } [PSCustomObject]@{ Name = $name Id = $category SubCategories = $subCategories } } ```

We can also still continue to use auditpol /list ... /v to get the info from the executable and still continue to work with the localized names as they can be mapped to the known GUIDs and vice versa.

clcaldwell commented 2 years ago

I don't disagree with this, but I am having a hard time finding out exactly how the Ansible project has been handling localizations; I'm not seeing a whole lot of examples or clear guidance on localization issues.

win_power_plan looks like it handles localization in a similar way to what you are proposing (it has a separate GUID field).

win_timezone is using tzutil.exe, which doesn't support localization (neither does Get-TimeZone), but there are localizations being supplied by the OS for time zones, which we could presumably grab from somewhere, see: image

It seems Ansible doesn't have the best localization support in general (ex. module files all have hardcoded English error messages). I don't mind modifying this PR with your suggestion, but I'm not entirely sure if it aligns with the rest of the Ansible project.

jborean93 commented 2 years ago

I don't disagree with this, but I am having a hard time finding out exactly how the Ansible project has been handling localizations; I'm not seeing a whole lot of examples or clear guidance on localization issues.

There really isn't a clear guidance as it's not something we've really set out to define before. That's why you see so many different things across the various modules.

To me the question becomes, what would you like to input if you were writing a playbook. I see 3 different options

The programmer in me would be doing category: 7d1880db-6d82-4047-8bb4-a9957655dcb4 # Category Name but I can understand the UX isn't great on this. The benefits here is that this definition would work on all systems, regardless of their locale. It also helps to ensure the code itself is using the unique ids internally in it's logic making it less susceptible to localisation problems.

From a non-English user supporting English only isn't a great user experience. It could be just as opaque as a GUID if you only had to use English values for these fields.

Ultimately I've probably strayed too far off what you are trying to do. Let's just focus on getting the internals all working by mapping the input values to a known set of GUIDs and have the internal logic use that for comparisons to fix the localisation problems. If we want to talk about making it more friendly and usable to define categories in different languages we can focus on that in a different PR.

jborean93 commented 2 years ago

One thing I have noticed is that the code is still attempting to use the category names with auditpol in a few places.

https://github.com/ansible-collections/community.windows/pull/398/files#diff-4967b1679882e40bdd42fbd2af57f3afafd797048252bf6d59d2168188f411d5R181-L94

This to me seems like it will continue to have problems with localisation and these commands should be changed so it invokes with the GUID as the identifier making it work regardless of the locale.

briantist commented 2 years ago

I also think it would be best to accept GUID or localized form. I am leaning against the hardcoded English fallback.

One thing to consider is that there is not only one "English" to fall back to. "American" (US) English is used by a minority of the world's English-speaking population, but may be appear to be weighed more heavily in tech, while others (Australian English, British English, Indian English, etc.) are separate locales. Spelling and choice of terms can vary between any of them.

I am not sure how much the terms change for this, but I ran @jborean93 's code to get the localized variants and for example I see Authorization Policy Change (for 0cce9231-69ae-11d9-bed3-505054503030) and I wonder if when he runs it, it's Authorisation Policy Change (a quick google suggests it probably still uses a Z because I can't find any results).


Anyway, point being, while it may work for this use case, hardcoding a list of names can be problematic. I would urge use of options 1 & 2 in Jordan's suggestion and discard option 3. Examples should be updated to show both uses, and show at least one non-English language usage (and imo at least one with non-Latin characters as it stands out more what is going on).

github-actions[bot] commented 2 years ago

This pull request is stale because it has been open for 4 weeks with no activity. Remove stale label or comment or this will be closed in 2 weeks.

guyomog78 commented 1 year ago

Hello, In this case, it's to workaroud Windows localization! Auditpol is localized for each language (it's an very old tool) For ansible usage, I always use English even I'm French ;-) no accents

Dylan-Bs commented 1 year ago

Sometimes, there is no choice, in my company, they don't want to work with a computer in english ahah! @guyomog78 What workaround have you find for it ?

guyomog78 commented 1 year ago

"Unfortunately, I have no workaround. I have suggested the capability of using GUIDs because they are standard in all languages, but it seems to be unavailable at the moment... Still waiting and seeing!"