win_secedit module: adds ability to mod local security policies #22775
Conversation
|
Migrated from ansible/ansible-modules-extras#3214 by rndmh3ro (not original author) |
ansibot
added
affects_2.4
|
The test The test |
ansibot
added
needs_revision
|
The test The test The test The test The test |
|
The test |
bcoca
removed
the
needs_triage
mattclay
added
the
ci_verified
ansibot
added
community_review
|
Hey @rndmh3ro and @defionscode I'll try and get to looking at this sometime soon, looks like a good module to have, I know I've written a once off script to do this before. There are a few things in there from a brief look that would need to be made more standard to our other Windows module and I'll add some comments for a first round look. |
There was a problem hiding this comment.
This is probably enough comments for a first round check. Three major things that would need to be done would be to;
- Add check mode to the module
- Add some tests
- More of a question for us to answer in the next WUG around how do we want to handle pipelining and indenting in powershell modules. I'll raise an agenda item.
Please feel free to ask me questions around any of my comments or if you need guidance around check mode/testing.
| ######## | ||
|
|
||
|
|
||
| Set-StrictMode -Version Latest |
There was a problem hiding this comment.
We usually set StrictMode to 2 when running a module automatically, this should be needed
|
|
||
| function Get-IniFile { | ||
| param ( | ||
| [parameter(mandatory=$true, position=0, valuefrompipelinebypropertyname=$true, valuefrompipeline=$true)][string]$FilePath |
There was a problem hiding this comment.
Are we able to just move this to function Get-IniFile($FilePath) to make it more python like
| { | ||
| "^\[(?<Section>.*)\]" | ||
| { | ||
| $ini.Add($curSectionName, $currentSection) |
There was a problem hiding this comment.
Probably want to remove an indent for these lines so they match up in each case statement
| } | ||
| default | ||
| { | ||
| throw "Unidentified: $_" # should not happen |
There was a problem hiding this comment.
Change to Fail-Json @{} "Error Message here" and change the error to be a bit more descriptive
| throw "Unidentified: $_" # should not happen | ||
| } | ||
| } | ||
| if ($ini.Keys -notcontains $curSectionName) { $ini.Add($curSectionName, $currentSection) } |
There was a problem hiding this comment.
Move the if body to a new line
|
|
||
| $params = Parse-Args $args; | ||
|
|
||
| $result = New-Object psobject @{ |
| changed = $false | ||
| }; | ||
|
|
||
| $category = Get-Attr $params "category" -failifempty $true |
There was a problem hiding this comment.
Have a look at Get-AnsibleParam which can be seen in other modules like win_tempfile. This way you can specify the object type and other options.
| }; | ||
|
|
||
| $category = Get-Attr $params "category" -failifempty $true | ||
| $key = Get-Attr $params "key" -failifempty $true |
There was a problem hiding this comment.
Change to Get-AnsibleParam as above
|
|
||
| $category = Get-Attr $params "category" -failifempty $true | ||
| $key = Get-Attr $params "key" -failifempty $true | ||
| $value = Get-Attr $params "value" -failifempty $true |
There was a problem hiding this comment.
Change to Get-AnsibleParam as above
| $sepath = "$home\sec_edit_dump.inf" | ||
|
|
||
| If ((Get-WmiObject Win32_ComputerSystem).PartOfDomain) { | ||
| Fail-Json $result "This host is joined to a Domain Controller, you'll need to modify GPO directly instead of secedit" |
There was a problem hiding this comment.
I don't think this should be here, in some GPO policies I've seen set definitely allowed me to change certain entries just not others. This should probably be a warning in the python document.
ansibot
added
needs_revision
| - The category you wish to modify a value under. This can things like System Access, Event Audit, etc. | ||
| If you supply an invalid category the module will error out and let you know what the valid categories are for that particular system. | ||
| required: true | ||
| default: null |
There was a problem hiding this comment.
If parameter is required, there is no default.
| For example, under the System Access category there is a key MinimumPasswordAge that could be targeted. | ||
| Just like with category, if an invalid key is specified, the module will error out and show what the valid keys for the given category are. | ||
| required: true | ||
| default: null |
| description: | ||
| - The value to assign to the key. | ||
| required: true | ||
| default: null |
ansibot
added
the
stale_ci
|
@rndmh3ro are you still willing to look into getting this merged in. I'm happy to take it on if you don't have the time as I think this would be good to have in Ansible. |
|
@jborean93, feel free to take this on! |
|
@jborean93 - sorry I couldn't make the WWG meeting. |
|
No worries time zones can be quite annoying, the general consensus was to split it up into separate modules mostly focusing on the user right stuff and one to do what this PR focuses on. This is similar to how win_path and win_environment fit together. |
ansibot
added
needs_repo
|
Given where this has gone, should we close this particular pr? |
ansibot
added
support:core
|
Yes, #26332 superseeds this. |
ansibot
added
feature
ISSUE TYPE
Feature Pull Request
COMPONENT NAME
win_secedit
ANSIBLE VERSION
SUMMARY
Allows users to modify local security policies via the secedit utility for example: