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

Added upgrade script to fix missing cert/request data #539

Merged
merged 1 commit into from
Aug 31, 2020

Conversation

edewata
Copy link
Contributor

@edewata edewata commented Aug 28, 2020

An upgrade script has been added to fix the missing sslserver
and subsystem cert/request data by copying it from another
subsystem.

https://bugzilla.redhat.com/show_bug.cgi?id=1869893

An upgrade script has been added to fix the missing sslserver
and subsystem cert/request data by copying it from another
subsystem.

https://bugzilla.redhat.com/show_bug.cgi?id=1869893
Copy link
Contributor

@jmagne jmagne left a comment

Choose a reason for hiding this comment

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

Looks good, approving.

Copy link
Member

@SilleBille SilleBille left a comment

Choose a reason for hiding this comment

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

I believe we can just get our required values by reading the CA's CS.cfg rather than trying to loop through all subsystems

def find_source_subsystem(self, subsystems):

# check each subsystem
for subsystem in subsystems:
Copy link
Member

Choose a reason for hiding this comment

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

Instead of trying to loop through all subsystems, can we just replace it with CA? I believe CA for sure contains the value of these

Example:

ca_subsystem = instance.get_subsystem(ca)
ca_subsystem.config.get('ca.sslserver.cert')
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's actually no guarantee that all PKI servers will have a CA subsystem, and even if it does, there's no guarantee if the CA will have all the cert info. This loop is trying to find the first other subsystem that has all the info, so most likely it will stop after the first or second iteration. It's based on the existing code:
https://github.com/dogtagpki/pki/blob/master/base/server/python/pki/server/deployment/pkihelper.py#L2934-L2942

A better solution would be to remove these params from CS.cfg so we don't need to maintain them anymore, but that can be done separately later.

Copy link
Member

Choose a reason for hiding this comment

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

AH! ok. Now i understand; CA and KRA can be installed on 2 different machines but be in the same Security Domain.

Hmmm. Now, that raises me another question: Let's say machine A has CA and machine B has KRA. Now, if the upgrade script is running in machine B, where will it read these values from?

Copy link
Contributor Author

@edewata edewata Aug 31, 2020

Choose a reason for hiding this comment

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

This particular regression presumably only happened to the subsystems added to the same instance after the first one, so that situation should not happen, or is outside the scope of this upgrade script. As mentioned above, a better solution is to remove it completely from CS.cfg.

Copy link
Member

Choose a reason for hiding this comment

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

OK.

Copy link
Member

@SilleBille SilleBille left a comment

Choose a reason for hiding this comment

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

As discussed in this PR, a better solution is to remove the copy of cert and certreq from CS.cfg, which will be addressed via a separate PR. As for the scope of this PR, this looks good to me! Thanks Endi for the explanation! 👍

@edewata
Copy link
Contributor Author

edewata commented Aug 31, 2020

Thanks! Merging.

@edewata edewata merged commit e117f89 into dogtagpki:master Aug 31, 2020
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