Add Certificate Authority functionality for AD

[krishnavema]

Implement certificate authority for AD

[spoore1]

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.

[danlavu]

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

[danlavu]

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

[spoore1]

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.

[spoore1]

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?

[danlavu]

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]

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

[danlavu]

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

[danlavu]

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.

[danlavu]

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.

[danlavu]

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

[spoore1]

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.

2. 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.

[spoore1]

Updates from test findings.

[spoore1]

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.

[spoore1]

I think there are some places you might be able to simplify the code a little and make it easier to read like:

• use of raise_on_error=False with result checks instead of relying on raising an exception in the host.conn.run() failures.
• indentation of some of the powershell code. maybe we can move the content into variables indented and use textwrap.dedent() to make it a little easier to read?

I'd like to get feedback from you and @danlavu on some of those points. Overall it looks good. My tests are working with this code as it is now so that's awesome!

[spoore1]

Considering that the default behavior for a host.conn.run() is to throw an exception if it fails, do we need to bother checking result.rc here (and in some other places) and returning from the method? Or can we drop this extra check to shorten and simplify the code some?

This is a question for @danlavu as well. Do we prefer to write out specific checks and error messages or leave the run()'s to raise an exception if it fails? I can definitely see some benefit in some of the explanations and error messages but, I'm not sure if we have a preference here.

[danlavu]

Entirely at your discretion, I would think that creating a CSR isn't likely to fail, but I have not troubleshooted this. I would suggest ruling out whatever is easiest to rule out, as this method is the problem when debugging.

[spoore1]

This PR is looking good. I only have one thing for this time but, I'll be discussing more with @danlavu tomorrow.

[danlavu]

Entirely at your discretion, I would think that creating a CSR isn't likely to fail, but I have not troubleshooted this. I would suggest ruling out whatever is easiest to rule out, as this method is the problem when debugging.

[danlavu]

Looks good, the identical parser functions I'd like to clarify on, would like to start the discussion to determine the differences for ipa.

[danlavu]

I forgot to add this comment yesterday. The challenging part of abstraction. The question is, how can we make the following have the parameters and values?

Don't freak out, we can worry about this later, just posing the question, and the question is, how similar is your test code to this smartcard test?

@pytest.mark.importance("high")
@pytest.mark.topology(KnownTopology.IPA)
@pytest.mark.builtwith(client="virtualsmartcard")
def test_ipa__switch_user_with_smartcard_authentication(client: Client, ipa: IPA):
    """
    :title: Smart card authentication allows nested 'su' as IPA user
    :setup:
        1. Create IPA user with key pair and certificate
        2. Copy certificate to client and initialize virtual smart card
        3. Configure SSSD with smart card support and start SSSD services
    :steps:
        1. Execute 'su - ipacertuser1' as root (no authentication required due to pam_rootok.so)
        2. From within the user session, execute nested 'su - ipacertuser1 -c whoami' as ordinary user
    :expectedresults:
        1. First 'su' succeeds without authentication as it's executed by root
        2. Second 'su' prompts for PIN and successfully authenticates with smart card
            a. PIN prompt appears, indicating smart card authentication is triggered for ordinary user
            b. 'whoami' command returns 'ipacertuser1', confirming successful smart card authentication
    :customerscenario: False
    """
    ipa.user("ipacertuser1").add()
    cert, key, _ = ipa.ca.request("ipacertuser1")
    cert_content = ipa.fs.read(cert)
    key_content = ipa.fs.read(key)

    client.fs.write("/opt/test_ca/ipacertuser1.crt", cert_content)
    client.fs.write("/opt/test_ca/ipacertuser1.key", key_content)
    client.smartcard.initialize_card()
    client.smartcard.add_key("/opt/test_ca/ipacertuser1.key")
    client.smartcard.add_cert("/opt/test_ca/ipacertuser1.crt")

    client.authselect.select("sssd", ["with-smartcard"])
    client.sssd.pam["pam_cert_auth"] = "True"
    client.sssd.start()
    client.svc.restart("virt_cacard.service")
    time.sleep(1)

    result = client.host.conn.run("su - ipacertuser1 -c 'su - ipacertuser1 -c whoami'", input="123456")
    assert "PIN" in result.stderr, f"String 'PIN' was not found in stderr! Stderr content: {result.stderr}"
    assert (
        "ipacertuser1" in result.stdout
    ), f"'ipacertuser1' not found in 'whoami' output! Stdout content: {result.stdout}"

[krishnavema]

Might need some changes and you can check here:test

[danlavu]

Thanks for the big patch, good stuff.

[spoore1]

LGTM. I was able to test the code with some additional setup on AD and with DNS updates in sssd-ci-containers that is in progress now. Also it should be noted that in order to test properly with Kerberos I had to change the crypto policy on the client to allow SHA1 for AD.

Tested with a slightly modified version of test mentioned elsewhere:

tests/test_ad.py::test_ad__certificate_authentication_basic (ad) PASSED

I'm holding off approving until the test issue is resolved. I believe you just need to rebase with master to fix the issue though. After that and letting the tests run again, I believe I'll be able to approve and merge this.