Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the environment's capabilities by integrating Active Directory Certificate Services (ADCS) to support smartcard authentication within the Active Directory domain. It also refines the DNS infrastructure to provide more robust and dynamic name resolution for various services across IPA, Samba, and AD. Additionally, client enrollment processes are made more reliable through explicit hostname configuration before domain joins. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 Active Directory Certificate Services (ADCS) setup for smartcard authentication, including new Ansible roles and tasks for configuring ADCS, certificate templates, and related DNS entries. However, it contains a critical security misconfiguration in the certificate template ACLs that allows any domain user to escalate privileges to Domain Administrator (ESC1/ESC3). Additionally, a deprecated cryptographic algorithm (3DES) is used in one of the certificate templates. Beyond the security concerns, there are also templating errors and incorrect DNS record configurations in etc.dnsmasq.conf.j2, and non-idempotent/non-persistent hostname changes in client enrollment tasks. These issues must be addressed before merging.
| - name: Apply Security ACLs and Publish Templates | ||
| win_shell: | | ||
| $configNC = (Get-ADRootDSE).configurationNamingContext | ||
| $templateDN = "CN={{ item.name }},CN=Certificate Templates,CN=Public Key Services,CN=Services,$configNC" | ||
|
|
||
| $sidUsers = (New-Object System.Security.Principal.NTAccount("Domain Users")).Translate([System.Security.Principal.SecurityIdentifier]) | ||
| $sidComps = (New-Object System.Security.Principal.NTAccount("Domain Computers")).Translate([System.Security.Principal.SecurityIdentifier]) | ||
|
|
||
| $acl = Get-Acl -Path "AD:$templateDN" | ||
|
|
||
| $rights = [System.DirectoryServices.ActiveDirectoryRights]::GenericAll | ||
| $type = [System.Security.AccessControl.AccessControlType]::Allow | ||
| $inherit = [System.Security.AccessControl.InheritanceFlags]::ContainerInherit | ||
| $guid = [Guid]::Empty | ||
|
|
||
| $uRule = New-Object System.DirectoryServices.ActiveDirectoryAccessRule($sidUsers, $rights, $type, $guid, $inherit, $guid) | ||
| $cRule = New-Object System.DirectoryServices.ActiveDirectoryAccessRule($sidComps, $rights, $type, $guid, $inherit, $guid) | ||
|
|
||
| $acl.AddAccessRule($uRule) | ||
| $acl.AddAccessRule($cRule) | ||
|
|
||
| Set-Acl -Path "AD:$templateDN" -AclObject $acl | ||
| loop: "{{ templates }}" |
There was a problem hiding this comment.
Critical Security Misconfiguration in AD CS Certificate Templates (ESC1/ESC3)
The Ansible task "Apply Security ACLs and Publish Templates" grants GenericAll permissions to Domain Users and Domain Computers on the certificate templates (EnrollmentAgent2 and SmartcardLogon4). This is an extremely dangerous misconfiguration that allows any domain user to escalate privileges to Domain Administrator.
Specifically:
- ESC1 (Enrollee Supplies Subject): The
SmartcardLogon4template hasCT_FLAG_ENROLLEE_SUPPLIES_SUBJECTenabled. Combined withGenericAll(or even justEnroll) forDomain Users, this allows any authenticated user to request a certificate for any other user (including Domain Administrators) by specifying their name in the Subject Alternative Name (SAN), leading to immediate domain escalation. - ESC3 (Enrollment Agent): Granting
GenericAlltoDomain Userson theEnrollmentAgent2template allows any user to obtain an Enrollment Agent certificate, which can then be used to enroll for certificates on behalf of other users, also leading to domain escalation. - Template Modification:
GenericAllallows any user to modify the template itself, enabling further attacks.
The principle of least privilege must be applied. The PSPKI module, installed in a preceding task, can be used to set these permissions more securely, typically requiring only Read and Enroll for these principals.
Remediation:
- Restrict ACLs: Do not grant
GenericAllorWriteDaclto broad groups likeDomain UsersorDomain Computers. Grant only minimum necessary permissions (e.g.,Enroll) to specific users or groups. - Secure Template Configuration: For
SmartcardLogon4, disableCT_FLAG_ENROLLEE_SUPPLIES_SUBJECT. The subject name should be automatically populated from Active Directory. - Follow Best Practices: Adhere to the principle of least privilege. Use specialized groups for enrollment and administrative tasks.
- name: Apply Security ACLs to Certificate Templates
win_shell: |
Import-Module PSPKI
Get-AdcsCertificateTemplate -Name "{{ item.name }}" | Add-AdcsAcl -User "Domain Users" -AccessType Allow -AccessMask Read, Enroll
Get-AdcsCertificateTemplate -Name "{{ item.name }}" | Add-AdcsAcl -User "Domain Computers" -AccessType Allow -AccessMask Read, Enroll
loop: "{{ templates }}"
args:
executable: powershell.exeThere was a problem hiding this comment.
I like the brevity of the solution you provided but, Get-AdcsCertificateTemplate does not appear to be a powershell cmdlet that I can find.
Can you suggest a simple change to the existing code to change GenericAll to Read and Enroll?
| {% if 'dc.ad.test' in hostvars %} | ||
| server=/ad.test/{{ hostvars['dc.ad.test']['ansible_facts']['ip_addresses'][0] }} | ||
| server=/{{ hostvars[ad]['ansible_facts']['windows_domain'] }}/{{ hostvars['dc.ad.test']['ansible_facts']['ip_addresses'][0] }} | ||
| {% elif 'ad' in groups and groups['ad'] %} | ||
| {% for ad in groups['ad'] %} | ||
| server=/{{ hostvars[ad]['ansible_facts']['windows_domain'] }}/{{ hostvars[ad]['ansible_facts']['ip_addresses'][0] }} | ||
| {% endfor %} | ||
| {% endif %} | ||
|
|
||
| # Add reverse zones for artificial hosts | ||
| {% if 'master.ipa.test' in hostvars %} | ||
| # Add reverse zones for artificial hosts in IPA domain | ||
| server=/251.255.10.in-addr.arpa/{{ hostvars['master.ipa.test']['ansible_facts']['default_ipv4']['address'] }} | ||
| rev-server=10.255.251.0/24,{{ hostvars['master.ipa.test']['ansible_facts']['default_ipv4']['address'] }} | ||
| {% endif %} | ||
| {% if 'dc.samba.test' in hostvars %} | ||
| rev-server=10.255.252.0/24,{{ hostvars['dc.samba.test']['ansible_facts']['default_ipv4']['address'] }} | ||
| {% endif %} | ||
| {% if 'dc.ad.test' in hostvars %} | ||
| rev-server=10.255.250.0/24,{{ hostvars['dc.ad.test']['ansible_facts']['default_ipv4']['address'] }} | ||
| {% elif 'ad' in groups and groups['ad'] %} | ||
| {% for ad in groups['ad'] %} | ||
| rev-server=10.255.250.0/24,{{ hostvars[ad]['ansible_facts']['ip_addresses'][0] }} | ||
| {% endfor %} | ||
| {% endif %} | ||
|
|
||
| # Add SRV record for LDAP | ||
| # Add SRV record records | ||
| {% if 'master.ldap.test' in hostvars %} | ||
| srv-host=_ldap._tcp.ldap.test,master.ldap.test,389 | ||
| {% endif %} | ||
| {% if 'dc.ad.test' in hostvars %} | ||
| srv-host=_ldap._tcp.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},{{ hostvars[ad]['ansible_facts']['windows_domain'] }},389 | ||
| srv-host=_kerberos._tcp.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},{{ hostvars[ad]['ansible_facts']['windows_domain'] }},88 | ||
| srv-host=_kerberos._udp.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},{{ hostvars[ad]['ansible_facts']['windows_domain'] }},88 | ||
| srv-host=_kpasswd._tcp.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},{{ hostvars[ad]['ansible_facts']['windows_domain'] }},464 | ||
| srv-host=_kpasswd._udp.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},{{ hostvars[ad]['ansible_facts']['windows_domain'] }},464 | ||
| {% elif 'ad' in groups and groups['ad'] %} | ||
| {% for ad in groups['ad'] %} | ||
| srv-host=_ldap._tcp.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},{{ hostvars[ad]['ansible_facts']['windows_domain'] }},389 | ||
| srv-host=_kerberos._tcp.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},dc.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},88 | ||
| srv-host=_kerberos._udp.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},dc.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},88 | ||
| srv-host=_kpasswd._tcp.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},dc.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},464 | ||
| srv-host=_kpasswd._udp.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},dc.{{ hostvars[ad]['ansible_facts']['windows_domain'] }},464 | ||
| {% endfor %} | ||
| {% endif %} | ||
| {% if 'dc.samba.test' in hostvars %} | ||
| srv-host=_ldap._tcp.samba.test,dc.samba.test,389 | ||
| srv-host=_kerberos._tcp.samba.test,dc.samba.test,88 | ||
| srv-host=_kerberos._udp.samba.test,dc.samba.test,88 | ||
| srv-host=_kpasswd._tcp.samba.test,dc.samba.test,464 | ||
| srv-host=_kpasswd._udp.samba.test,dc.samba.test,464 | ||
| {% endif %} |
There was a problem hiding this comment.
There are multiple issues in this template that will cause templating errors and generate incorrect DNS configuration:
- In the
if 'dc.ad.test' in hostvarsblocks (lines 22-23 and 49-55), the variableadis used but it is not defined in that context. It is only defined in theelifblocks. This will cause a fatal templating error. - The target host for several SRV records (lines 50-54 and 57) is
{{ hostvars[ad]['ansible_facts']['windows_domain'] }}, which resolves to the domain name (e.g.,ad.test) instead of the server's FQDN (e.g.,dc.ad.test). SRV records must point to a hostname, not a domain.
The suggested change fixes these issues by using the correct variables and ensuring SRV records point to the correct host FQDNs.
{% if 'dc.ad.test' in hostvars %}
server=/{{ hostvars['dc.ad.test']['ansible_facts']['windows_domain'] }}/{{ hostvars['dc.ad.test']['ansible_facts']['ip_addresses'][0] }}
{% elif 'ad' in groups and groups['ad'] %}
{% for ad_host in groups['ad'] %}
server=/{{ hostvars[ad_host]['ansible_facts']['windows_domain'] }}/{{ hostvars[ad_host]['ansible_facts']['ip_addresses'][0] }}
{% endfor %}
{% endif %}
# Add reverse zones for artificial hosts
{% if 'master.ipa.test' in hostvars %}
rev-server=10.255.251.0/24,{{ hostvars['master.ipa.test']['ansible_facts']['default_ipv4']['address'] }}
{% endif %}
{% if 'dc.samba.test' in hostvars %}
rev-server=10.255.252.0/24,{{ hostvars['dc.samba.test']['ansible_facts']['default_ipv4']['address'] }}
{% endif %}
{% if 'dc.ad.test' in hostvars %}
rev-server=10.255.250.0/24,{{ hostvars['dc.ad.test']['ansible_facts']['default_ipv4']['address'] }}
{% elif 'ad' in groups and groups['ad'] %}
{% for ad_host in groups['ad'] %}
rev-server=10.255.250.0/24,{{ hostvars[ad_host]['ansible_facts']['ip_addresses'][0] }}
{% endfor %}
{% endif %}
# Add SRV record records
{% if 'master.ldap.test' in hostvars %}
srv-host=_ldap._tcp.ldap.test,master.ldap.test,389
{% endif %}
{% if 'dc.ad.test' in hostvars %}
{% set ad_host = 'dc.ad.test' %}
{% set ad_domain = hostvars[ad_host]['ansible_facts']['windows_domain'] %}
srv-host=_ldap._tcp.{{ ad_domain }},{{ ad_host }},389
srv-host=_kerberos._tcp.{{ ad_domain }},{{ ad_host }},88
srv-host=_kerberos._udp.{{ ad_domain }},{{ ad_host }},88
srv-host=_kpasswd._tcp.{{ ad_domain }},{{ ad_host }},464
srv-host=_kpasswd._udp.{{ ad_domain }},{{ ad_host }},464
{% elif 'ad' in groups and groups['ad'] %}
{% for ad_host in groups['ad'] %}
{% set ad_domain = hostvars[ad_host]['ansible_facts']['windows_domain'] %}
srv-host=_ldap._tcp.{{ ad_domain }},{{ ad_host }},389
srv-host=_kerberos._tcp.{{ ad_domain }},{{ ad_host }},88
srv-host=_kerberos._udp.{{ ad_domain }},{{ ad_host }},88
srv-host=_kpasswd._tcp.{{ ad_domain }},{{ ad_host }},464
srv-host=_kpasswd._udp.{{ ad_domain }},{{ ad_host }},464
{% endfor %}
{% endif %}
{% if 'dc.samba.test' in hostvars %}
srv-host=_ldap._tcp.samba.test,dc.samba.test,389
srv-host=_kerberos._tcp.samba.test,dc.samba.test,88
srv-host=_kerberos._udp.samba.test,dc.samba.test,88
srv-host=_kpasswd._tcp.samba.test,dc.samba.test,464
srv-host=_kpasswd._udp.samba.test,dc.samba.test,464
{% endif %}
| msPKI-RA-Application-Policies: | ||
| msPKI-Asymmetric-Algorithm`PZPWSTR`RSA`msPKI-Hash-Algorithm`PZPWSTR`SHA256`msP | ||
| KI-Key-Usage`DWORD`16777215`msPKI-RA-Application-Policies`PZPWSTR`1.3.6.1.4.1. | ||
| 311.20.2.1`msPKI-Symmetric-Algorithm`PZPWSTR`3DES`msPKI-Symmetric-Key-Length`D |
There was a problem hiding this comment.
Use of Weak Cryptographic Algorithm (3DES)
The certificate template definition for SmartcardLogon4 specifies the use of 3DES (Triple DES) as the symmetric algorithm in the msPKI-RA-Application-Policies attribute. 3DES is considered weak and has been deprecated by NIST due to its vulnerability to various attacks (e.g., Sweet32).
Remediation:
Update the certificate template to use a modern and secure symmetric algorithm, such as AES (e.g., AES256).
| Set-Acl -Path "AD:$templateDN" -AclObject $acl | ||
| loop: "{{ templates }}" | ||
|
|
||
| - name: Apply Security ACLs and Publish Templates |
There was a problem hiding this comment.
The name of this task, "Apply Security ACLs and Publish Templates", is a duplicate of a previous task name and is misleading. This task only publishes the templates using Add-CATemplate. A more accurate name would be "Publish Certificate Templates" to avoid confusion.
- name: Publish Certificate Templates| - name: Set hostname to FQDN for AD join | ||
| ansible.builtin.command: "hostname {{ inventory_hostname }}.{{ ad_domain }}" |
There was a problem hiding this comment.
Using ansible.builtin.command to run the hostname command sets the hostname only for the current session and is not persistent across reboots. Furthermore, this task is not idempotent; it will run and report a change on every execution. It's better to use the ansible.builtin.hostname module, which handles setting the hostname persistently and is idempotent.
- name: Set hostname to FQDN for AD join
ansible.builtin.hostname:
name: "{{ inventory_hostname }}.{{ ad_domain }}"| - name: Set hostname to FQDN for IPA join | ||
| ansible.builtin.command: "hostname {{ inventory_hostname }}.{{ ipa_domain }}" |
There was a problem hiding this comment.
Using ansible.builtin.command to run the hostname command sets the hostname only for the current session and is not persistent across reboots. Furthermore, this task is not idempotent; it will run and report a change on every execution. It's better to use the ansible.builtin.hostname module, which handles setting the hostname persistently and is idempotent.
- name: Set hostname to FQDN for IPA join
ansible.builtin.hostname:
name: "{{ inventory_hostname }}.{{ ipa_domain }}"| - name: Set hostname to FQDN for samba join | ||
| ansible.builtin.command: "hostname {{ inventory_hostname }}.{{ samba_domain }}" |
There was a problem hiding this comment.
Using ansible.builtin.command to run the hostname command sets the hostname only for the current session and is not persistent across reboots. Furthermore, this task is not idempotent; it will run and report a change on every execution. It's better to use the ansible.builtin.hostname module, which handles setting the hostname persistently and is idempotent.
- name: Set hostname to FQDN for samba join
ansible.builtin.hostname:
name: "{{ inventory_hostname }}.{{ samba_domain }}"- Add new Certificate Template task file in ad role. - Include task in ad role main.yml - Adding certificate template ldif export files
spoore1
force-pushed
the
adcs_smartcard_setup
branch
from
98ded0e to
a236597
Compare
1 participant
No description provided.