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

Add est to pkispawn #4844

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Add est to pkispawn #4844

merged 1 commit into from
Sep 18, 2024

Conversation

fmarco76
Copy link
Member

@fmarco76 fmarco76 commented Sep 6, 2024

This initial commit add est configuration for deployment in instance with existing CA.

Actually, several step are just skipped which could be required like creating initial users, reporting the status, etc....

The final installation is compliant with the page: https://github.com/dogtagpki/pki/blob/master/docs/installation/est/Installing_EST.md

@edewata
Copy link
Contributor

edewata commented Sep 9, 2024

I noticed that there are some database changes. I think we should provide a database upgrade procedure for someone who already has a CA from an older PKI version then upgrades to the new PKI version to add EST into their system.

@edewata
Copy link
Contributor

edewata commented Sep 9, 2024

About the /usr/local/libexec/estauthz, I'd suggest to include that file in the RPM so it will be installed when the RPM is installed instead of creating it in pkispawn since there's no guarantee that the user running pkispawn will have a permission to modify /usr/local/libexec. If the user wants to use a different script, they should be able to specify that in pkispawn config.

@fmarco76
Copy link
Member Author

fmarco76 commented Sep 9, 2024

About the /usr/local/libexec/estauthz, I'd suggest to include that file in the RPM so it will be installed when the RPM is installed instead of creating it in pkispawn since there's no guarantee that the user running pkispawn will have a permission to modify /usr/local/libexec. If the user wants to use a different script, they should be able to specify that in pkispawn config.

Actually, this path is not needed, it could be inside /usr/share/pki/est or some other place. However, including the file in rpm avoid problems with fapolicy which could require a rule to access generated files.

I have added a first CI test for this deployment but there are many items not working and requiring some thinking before to move on with this PR.

@fmarco76 fmarco76 force-pushed the EST_pkispawn branch 9 times, most recently from 1e5944c to e01ae7b Compare September 13, 2024 13:12
@fmarco76 fmarco76 force-pushed the EST_pkispawn branch 7 times, most recently from a8ec1a7 to 2af054b Compare September 17, 2024 14:48
@fmarco76 fmarco76 marked this pull request as ready for review September 17, 2024 15:29
@fmarco76
Copy link
Member Author

@edewata after a lot of effort to make it works like other subsystem I gave up and make it more like ACME. There were too many missing features.
Now there is a problem with ACME standalone which I can look at while writing the EST standalone test. Not sure the relation with other problems.
Finally, I think we can merge the realm configuration between ACME and EST. They can use the same classes in server package but can be done later.

Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

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

In general it looks good, but there is a change that broke one of the ACME tests, and I also have some minor comments. Feel free to update & merge.

Just FYI, I'm working on adding support for ACME removal using pkidestroy. I suppose EST removal will be similar.

if: always()
uses: actions/upload-artifact@v4
with:
name: ca-basic
Copy link
Contributor

Choose a reason for hiding this comment

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

The artifacts name should match the test name to prevent conflicts and to make it easier to find, but gathering and uploading the artifacts is actually optional since the previous steps are already displaying the important logs so feel free to remove it if you want.

Comment on lines 39 to 43
acme_realm_bind_password
acme_realm_password
est_realm_bind_password
est_realm_password
est_ca_password
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to keep the acme_* params here since they are still used in configure_acme_realm(). If we want to merge the realm params for ACME & EST we probably should use something like pki_realm_* or maybe just realm_*.

Copy link
Member Author

@fmarco76 fmarco76 Sep 18, 2024

Choose a reason for hiding this comment

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

This is a mistake moving between different implementations. Thanks!

@@ -5376,15 +5376,135 @@ def spawn_acme(self):

self.deploy_acme_webapp(subsystem)

def create_est_subsystem(self):
'''
See also pki-server acme-create.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be pki-server est-create.

@@ -0,0 +1,2 @@
class=org.dogtagpki.est.ExternalProcessRequestAuthorizer
executable=/usr/local/libexec/estauthz
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the path correct?

est_realm_users_dn=ou=people,dc=est,dc=pki,dc=example,dc=com
est_realm_groups_dn=ou=groups,dc=est,dc=pki,dc=example,dc=com
est_realm_statements=/usr/share/pki/est/conf/realm/statements.conf
est_authorizer_exec_path=/usr/libexec/estauthz
Copy link
Contributor

Choose a reason for hiding this comment

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

The path in authorizer.conf is different.

BTW, the estauthz script looks more like a sample script instead of a general purpose tool. Would it be better to install it somewhere under /usr/share/pki/est instead of /usr/libexec?

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the script to /usr/share/pki/est/bin/estauthz and update all the references.


else:
# otherwise, deploy the subsystem and wait until it starts
subsystem.enable(
wait=True,
max_wait=self.startup_timeout,
timeout=self.request_timeout)
Copy link
Contributor

@edewata edewata Sep 17, 2024

Choose a reason for hiding this comment

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

The self.instance.start() in the original code is actually needed by spawn_acme() to start the instance in case ACME is the only subsystem on the server. See the test for ACME on separate instance (it's currently failing). EST probably needs a similar code too later.

@fmarco76 fmarco76 force-pushed the EST_pkispawn branch 2 times, most recently from 2f46def to 3c77764 Compare September 18, 2024 08:52
EST deployment is included in pkispwn. The installation does not perform
all the steps done for CA and other subsystems so there is no security
domain management and user administration. During the installation there
is no DS or other DBs connection which has to be performed by the user
before or after the installation.
Copy link

sonarcloud bot commented Sep 18, 2024

@fmarco76
Copy link
Member Author

@edewata Thanks!
I have update following your comments. It is not clear to me why the rpminspect is failing now. Actually, IIUC we have the byte code version set to 17 for compiling but the jvm required is 21 so I was expecting different behaviour before. We can investigate in separate PR.

@fmarco76 fmarco76 merged commit 9dd4ea2 into dogtagpki:master Sep 18, 2024
151 of 159 checks passed
@edewata
Copy link
Contributor

edewata commented Sep 18, 2024

Yeah, rpminspect is a new but unrelated issue.

@fmarco76 fmarco76 deleted the EST_pkispawn branch September 18, 2024 14:10
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