Skip to content

Add Certificate Authority functionality for AD#209

Draft
krishnavema wants to merge 1 commit intomasterfrom
ad-certificate-management
Draft

Add Certificate Authority functionality for AD#209
krishnavema wants to merge 1 commit intomasterfrom
ad-certificate-management

Conversation

@krishnavema
Copy link
Contributor

Implement certificate authority for AD

Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start and as I mentioned earlier my main initial concern is around the request()/request_smartcard() methods.

My main thought here is to make request() align more closely with what you wrote for the IPA one so we can abstract it out to the GenericProvider later. I think the current request() could be made request_enrollment() and request_smartcard() renamed to request with some minor changes.

You might also consider a method to generate the INF file based on some basic input like template, subject, keysize. Then use template to select which set of configs to use for the INF based on that.

@krishnavema krishnavema force-pushed the ad-certificate-management branch 2 times, most recently from eedd166 to bec5310 Compare October 18, 2025 15:08
@krishnavema krishnavema requested a review from danlavu October 18, 2025 15:08
@krishnavema krishnavema force-pushed the ad-certificate-management branch 2 times, most recently from fc5e3ee to e3824a9 Compare November 4, 2025 02:22
@krishnavema krishnavema requested a review from spoore1 November 4, 2025 05:38
Copy link
Contributor

@danlavu danlavu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great, with a few minor nitpicks and a couple of larger requested changes.

@danlavu
Copy link
Contributor

danlavu commented Nov 10, 2025

@krishnavema I'm sorry, I did review this before I left for PTO but I didn't click submit review.

@krishnavema krishnavema force-pushed the ad-certificate-management branch from e3824a9 to cc761a8 Compare November 20, 2025 17:19
@krishnavema krishnavema requested a review from danlavu November 20, 2025 17:28
Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another glance and it's looking very good. Just a question about the PSIni module calls you have in the request methods. I can't seem to find those.

self.host.conn.run(
f"""
$iniPath = "{inf_path}"
New-PsIniFile -Path $iniPath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find these cmdlets on my AD server and they don't seem to be a part of PSIni from what I can tell. Maybe I'm missing something. Is this from a custom or external module?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PsIni is loaded into the image, but it is a third-party module.

sssd-ci-containers/src/ansible/roles/ad/tasks/main.yml

 - name: Install powershell modules
   win_shell: |
     [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12
     Get-PackageProvider NuGet -ForceBootstrap
     Set-PSRepository -Name 'PSGallery' -InstallationPolicy Trusted
     Install-Module PSIni -RequiredVersion 3.1.4 -Confirm:$False

We are using it with the GPO stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the problem with module i guess so i reverted to normal powershell script .

Copy link
Contributor

@danlavu danlavu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, nitpicks and missing unit tests for the misc functions.

self.host.conn.run(
f"""
$iniPath = "{inf_path}"
New-PsIniFile -Path $iniPath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PsIni is loaded into the image, but it is a third-party module.

sssd-ci-containers/src/ansible/roles/ad/tasks/main.yml

 - name: Install powershell modules
   win_shell: |
     [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12
     Get-PackageProvider NuGet -ForceBootstrap
     Set-PSRepository -Name 'PSGallery' -InstallationPolicy Trusted
     Install-Module PSIni -RequiredVersion 3.1.4 -Confirm:$False

We are using it with the GPO stuff.

@krishnavema krishnavema force-pushed the ad-certificate-management branch from cc761a8 to c55f41c Compare November 24, 2025 02:36
Copy link
Contributor

@danlavu danlavu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested it, but besides the single nitpick, I think this looks great. It does need to be tested; tentative approval until Scott or I can test it.

@krishnavema krishnavema force-pushed the ad-certificate-management branch from c55f41c to 01cd51d Compare January 9, 2026 18:10
@krishnavema krishnavema requested a review from danlavu January 9, 2026 18:18
Copy link
Contributor

@danlavu danlavu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tentative approval, will finish the approval after I test it.

Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some notes after trying to test.

  1. I believe the __request_enrollment() method in AD may not be necessary.

Most (if not every) smart card certificate case we'll need to cover will require an Enrollment Agent certificate because we're using Administrator to request certificates for other users. In this case, we could just create a default enrollment agent certificate for Administrator during the AD environment setup (still TBD). If we do this, we can simply have a method to get the hash for it that can be used inside of the request() method instead of passing it in to that method.

If you want to keep the code to generate the enrollment agent certificate, I would suggest making __request_enrollment() called only once during class init() or __setup() since we only need one enrollment certificate to issue other certificates.

  1. I think request() will be used far more than request_basic() so the latter may not be necessary either.

Since request() is what should be in the generic class, and we don't have a request_basic() for IPA, I don't think we'll need this (at least for smart card testing). If you want to keep it around for other potential usage later, I'm ok with that. It should be noted though that currently running requst_basic() even with a user name specified for the subject will return a certificate with subject = CN=Administrator. That's why we have to use the "Enroll On Behalf Of" functionality to get "CN=username" for the issued certificate's subject.

@krishnavema krishnavema force-pushed the ad-certificate-management branch from 01cd51d to 1072f70 Compare January 26, 2026 18:40
Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates from test findings.

@krishnavema krishnavema force-pushed the ad-certificate-management branch from 1072f70 to 1fe7c92 Compare February 4, 2026 14:46
@krishnavema krishnavema requested a review from spoore1 February 4, 2026 15:51
Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, after a lot of digging and working through some test environment issues, I got a good test with the following changes I suggest. I'll send more info directly on some test env setup changes you'll need to make as well.

if (Test-Path "C:\pki") {{
Write-Host "Cleaning up certificate directories in C:\pki"
Remove-Item "C:\pki" -Recurse -Force -ErrorAction SilentlyContinue
}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's going to be necessary now but, we may eventually want to have it also cleanup and remove the old certificates as well. Might need more research in this area though. Something to keep in mind later if we see issues with a lot of tests running against the same AD instance.

:type template: str
:param subject: Certificate subject (e.g., "CN=testuser").
:type subject: str
:return: A tuple of (certificate_path, pfx_path, csr_path).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change return from pfx_path to key_path to closer match IPA version of request().

:return: A tuple of (certificate_path, pfx_path, csr_path).
:rtype: tuple[str, str, str]
:raises RuntimeError: If smartcard certificate request fails.
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First let's get base_dn and user_dn to make sure we have the correct values we'll need later:

        base_dn =  ",".join(f"dc={part}" for part in self.host.domain.split(".") if part)
        user_dn = subject
        if base_dn.lower() not in subject.lower():
            user_dn = f"{subject},CN=Users,{base_dn}"


requester_name = f"{self.host.domain.upper()}\\Administrator"

base = re.sub(r"[^a-zA-Z0-9._-]", "_", subject)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the base we can use split like this to just extract the username to use for the filenames from the DN name:

        base = subject.split(',')[0].split('=')[1]

req_path = os.path.join(self.temp_dir, f"{base}.req")
signed_req_path = os.path.join(self.temp_dir, f"{base}_signed.req")
cert_path = os.path.join(self.temp_dir, f"{base}.cer")
pfx_path = os.path.join(self.temp_dir, f"{base}.pfx")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the above variables, you should set some others that'll be needed later:

        extracted_key_path = os.path.join(self.temp_dir, f"{base}_extracted.key")
        key_path = os.path.join(self.temp_dir, f"{base}.key")

f"""
$infContent = @"
[NewRequest]
Subject = "{subject}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change {subject} to {user_dn}


self.export_pfx(cert_path, pfx_path)

return cert_path, pfx_path, req_path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After exporting the pfx file, you should extract the private key and return it. Following is code that worked for me. In order for it to work though, you must first install openssl on the AD server.

To install openssl on my AD server, I did this:

$url = "https://aka.ms/vs/17/release/vc_redist.x64.exe"
$dest = "$env:TEMP\vc_redist.x64.exe"
Invoke-WebRequest -Uri $url -OutFile $dest
Start-Process -FilePath $dest -ArgumentList "/install", "/quiet", "/norestart" -Wait

$url = "https://slproweb.com/download/Win64OpenSSL-3_6_1.exe"
$dest = "$env:TEMP\OpenSSL.exe"
Invoke-WebRequest -Uri $url -OutFile $dest
Start-Process -FilePath $dest -ArgumentList "/silent", "/verysilent", "/sp-", "/suppressmsgboxes" -Wait

After installing openssl, this worked for me (some of the Powershell was generated with Gemini):

        result = self.host.conn.run(
            f'"Secret123" | & openssl pkcs12 -in {pfx_path} -nocerts -nodes -out {extracted_key_path} -passin stdin',
            raise_on_error=False
        )
        if result.rc != 0:
            raise RuntimeError(f"Unable to extract key from PFX file {pfx_path}: {result.stderr}")

        result = self.host.conn.run(
            f'openssl rsa -in {extracted_key_path} -out {key_path}'
        )
        if result.rc != 0:
            raise RuntimeError(f"Unable to convert key from extracted key file {extracted_key_path}: {result.stderr}")

        # Add cert info to user's altSecurityIdentities attribute in AD
        self.host.conn.run(
            f"""
            $certPath = "{cert_path}"
            $userDN = "{user_dn}"
            $cert = New-Object System.Security.Cryptography.X509Certificates.X509Certificate2($certPath)

            # Get Issuer DN
            $issuerDN = $cert.Issuer

            # Reverse Issuer DN to match order needed
            $elements = $issuerDN -split ',\s*'
            [array]::Reverse($elements)
            $reversedDN = $elements -join ","

            # Get Serial Number, remove spaces, and reverse the byte order
            $rawHex = $cert.SerialNumber
            $hexArray = for ($i = $rawHex.Length - 2; $i -ge 0; $i -= 2) {{ $rawHex.Substring($i, 2) }}
            $reversedHex = $hexArray -join ""

            # Construct the altSecurityIdentities string
            $altID = "X509:<I>$reversedDN<SR>$reversedHex"

            # Add serial number mapping to user entry in AD
            Set-ADUser -Identity $userDN -Add @{{altSecurityIdentities = $altID}}
            """)

        return cert_path, key_path, req_path

Comment on lines +2915 to +2918
result = self.host.conn.run(f"(Get-PfxCertificate -FilePath {cert_path}).Thumbprint", raise_on_error=False)
if result.rc != 0:
raise RuntimeError("Thumbprint not found in certificate output")
return result.stdout.strip()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't working for me in my tests but, I was able to replace it with this which did work:

result = self.host.conn.run(f"(Get-PfxCertificate -FilePath {cert_path}).Thumbprint", raise_on_error=False)

@krishnavema krishnavema force-pushed the ad-certificate-management branch from 1fe7c92 to 2d0bd8c Compare February 16, 2026 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants