Add Certificate Authority functionality for AD #209
Conversation
spoore1
left a comment
spoore1
left a comment
There was a problem hiding this comment.
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
force-pushed
the
ad-certificate-management
branch
2 times, most recently
from
eedd166 to
bec5310
Compare
krishnavema
force-pushed
the
ad-certificate-management
branch
2 times, most recently
from
fc5e3ee to
e3824a9
Compare
danlavu
left a comment
danlavu
left a comment
There was a problem hiding this comment.
Overall, this looks great, with a few minor nitpicks and a couple of larger requested changes.
|
@krishnavema I'm sorry, I did review this before I left for PTO but I didn't click submit review. |
krishnavema
force-pushed
the
ad-certificate-management
branch
from
e3824a9 to
cc761a8
Compare
spoore1
left a comment
spoore1
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
left a comment
danlavu
left a comment
There was a problem hiding this comment.
This is great, nitpicks and missing unit tests for the misc functions.
| self.host.conn.run( | ||
| f""" | ||
| $iniPath = "{inf_path}" | ||
| New-PsIniFile -Path $iniPath |
There was a problem hiding this comment.
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
force-pushed
the
ad-certificate-management
branch
from
cc761a8 to
c55f41c
Compare
danlavu
left a comment
danlavu
left a comment
There was a problem hiding this comment.
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.
| """ | ||
| Initialize the AD Certificate Authority helper. | ||
| :param host: Remote AD host. |
There was a problem hiding this comment.
Nitpick, all the hosts are remote, so AD host. is sufficient.
Implement certificate authority for AD