ADCS Setup for Smart Card Authentication testing#174
Conversation
spoore1
commented
May 5, 2026
spoore1
commented
- Add new Certificate Template task file in ad role.
- Include task in ad role main.yml
- Adding certificate template ldif export files
- Add new Certificate Template task file in ad role. - Include task in ad role main.yml - Adding certificate template ldif export files
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces certificate template setup for smart card authentication by adding LDIF files and a new task file for ADCS configuration. The review identified several critical issues: the CA configuration tasks are redundant and potentially destructive due to the use of overwrite flags, and there are undefined PowerShell variables in the ACL assignment script. Additionally, it is recommended to convert the LDIF files into Jinja2 templates to avoid hardcoded domain suffixes and improve portability.
| - name: Install ADCS Certificate Authority role | ||
| win_feature: | ||
| name: ADCS-Cert-Authority | ||
| include_management_tools: yes | ||
| state: present | ||
|
|
||
| - name: Configure ADCS as Enterprise Root CA | ||
| win_shell: | | ||
| Install-AdcsCertificationAuthority -CAType EnterpriseRootCA ` | ||
| -CACommonName "ad-RootCA" ` | ||
| -KeyLength 2048 ` | ||
| -HashAlgorithmName SHA256 ` | ||
| -ValidityPeriod Years ` | ||
| -OverwriteExistingCAinDS ` | ||
| -OverwriteExistingDatabase ` | ||
| -ValidityPeriodUnits 10 -Force | ||
| register: adcs_config | ||
| args: | ||
| executable: powershell.exe |
There was a problem hiding this comment.
The installation of the ADCS role and the configuration of the Enterprise Root CA in this block are redundant with the tasks already defined in install.yml (lines 33-51). Moreover, the use of -OverwriteExistingDatabase and -OverwriteExistingCAinDS in the win_shell task is dangerous, as it will force a re-initialization of the CA and wipe the existing database whenever this task is executed. It is recommended to remove this redundant block or add a guard (e.g., using the creates parameter or checking for the CA service) to prevent accidental data loss.
| $uRule = New-Object System.DirectoryServices.ActiveDirectoryAccessRule($uSid, $rights, $type, $enrollGuid, $inherit, $guid) | ||
| $cRule = New-Object System.DirectoryServices.ActiveDirectoryAccessRule($cSid, $rights, $type, $enrollGuid, $inherit, $guid) |
There was a problem hiding this comment.
The PowerShell variables $uSid and $cSid used in these lines are undefined. Based on the variable assignments at lines 90 and 91, they should be $sidUsers and $sidComps. This mismatch will cause the script to fail with a null reference error when attempting to create the ActiveDirectoryAccessRule objects.
$uRule = New-Object System.DirectoryServices.ActiveDirectoryAccessRule($sidUsers, $rights, $type, $enrollGuid, $inherit, $guid)
$cRule = New-Object System.DirectoryServices.ActiveDirectoryAccessRule($sidComps, $rights, $type, $enrollGuid, $inherit, $guid)| @@ -0,0 +1,43 @@ | |||
| dn: CN=EnrollmentAgent2,CN=Certificate Templates,CN=Public Key Services,CN=Services,CN=Configuration,DC=ad,DC=test | |||
There was a problem hiding this comment.
The Distinguished Name (DN) contains a hardcoded domain suffix DC=ad,DC=test. This prevents the role from being used in environments with different domain names. To make this role reusable, consider converting this file into a Jinja2 template (.j2) and using the ad_suffix variable defined in the role's facts.
| @@ -0,0 +1,50 @@ | |||
| dn: CN=SmartcardLogon4,CN=Certificate Templates,CN=Public Key Services,CN=Services,CN=Configuration,DC=ad,DC=test | |||
There was a problem hiding this comment.
The Distinguished Name (DN) is hardcoded to DC=ad,DC=test. For better maintainability and portability, this file should be templated using Ansible's template module so that the domain suffix can be dynamically injected via the ad_suffix variable.
1 participant
