Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update pkispawn to support ACME #4848

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Update pkispawn to support ACME #4848

merged 1 commit into from
Sep 12, 2024

Conversation

edewata
Copy link
Contributor

@edewata edewata commented Sep 12, 2024

pkispawn has been modified to support installing ACME in a shared PKI server (e.g. with existing CA).

New pkispawn params have been added to specify the ACME database, issuer, and realm. A sample configuration has been provided in acme.cfg.

The pki_ds_setup, pki_security_domain_setup, and pki_registry_enable params in the default.cfg have been moved from [DEFAULT] into each subsystem's section so that ACME can skip DS setup, security domain setup, and registry setup by default.

The config templates for ACME database, issuer, and realm have been modified to no longer contain passwords. The passwords will need to be specified during installation.

Some code in acme.py has been moved into subsystem.py so that it can be reused.

The basic ACME test and the test with PostgreSQL have been modified to install ACME using pkispawn.

Copy link
Member

@fmarco76 fmarco76 left a comment

Choose a reason for hiding this comment

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

I am not familiar with ACME but the code looks good. I have a comment for the configuration properties but feel free to merge and discuss later further changes.

pki_security_domain_setup=False
pki_registry_enable=False

# Database params:
Copy link
Member

Choose a reason for hiding this comment

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

Why these parameters are not included with default empty value?

All parameters start with pki_ while these starts with acme_. Actually I think start the subsystem variable with <subsystem>_ and leave pki_ to the global section could be better but I like consistency more.

pkispawn has been modified to support installing ACME in a
shared PKI server (e.g. with existing CA).

New pkispawn params have been added to specify the ACME
database, issuer, and realm. A sample configuration has been
provided in acme.cfg.

The pki_ds_setup, pki_security_domain_setup, and
pki_registry_enable params in the default.cfg have been moved
from [DEFAULT] into each subsystem's section so that ACME can
skip DS setup, security domain setup, and registry setup by
default.

The templates for ACME database, issuer, and realm configs
have been modified to no longer contain passwords. The
passwords need to be specified during installation.

Some code in acme.py has been moved into subsystem.py so that
it can be reused.

The basic ACME test and the test with PostgreSQL have been
modified to install ACME using pkispawn.
Copy link

sonarcloud bot commented Sep 12, 2024

@edewata
Copy link
Contributor Author

edewata commented Sep 12, 2024

@fmarco76 Thanks! I've fixed some pylint issues. I'll merge but feel free to continue the discussion.

There's no default params for database/issuer/realm since the _type param cannot be blank and other params are only required depending on which type is selected so it's probably better to just explain it in the doc and let the user specify the params they need. The doc will be updated separately later.

I was considering to use pki_ prefix too but it seems kind of long (e.g. pki_acme_database_type) and redundant, but I don't have a strong opinion on this. If you prefer to use the pki_ prefix just let me know, we can add it in a separate PR.

@edewata edewata merged commit a72c3f6 into dogtagpki:master Sep 12, 2024
150 of 151 checks passed
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.

2 participants