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

Fix Bug 1809273 - CRL generation performs an unindexed search. #377

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jmagne
Copy link
Contributor

@jmagne jmagne commented Apr 15, 2020

Implemented with a modification to a CLI command, also calls this command from pkispawn to take care of new instances:

Example use of standalone command:

Which will work if the indexes in question have not been created already.

pki-server ca-db-upgrade --verbose --action update-vlv-indexes --vlv-file crlcaissuer.ldif --vlv-tasks-file crlcaissuertasks.ldif --issuer-dn "CN=CA Signing Certificate,OU=pki-tomcat,O=localhost.localdomain Security Domain"

@jmagne jmagne requested a review from edewata April 15, 2020 22:24
@edewata edewata requested a review from frasertweedale April 15, 2020 22:35
@edewata
Copy link
Contributor

edewata commented Apr 15, 2020

@frasertweedale You might want to take a look at this as well since this is related to LWCA in PKI 10.9.

@cipherboy cipherboy mentioned this pull request Apr 16, 2020
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.

I think it's good that we can get this working as a standalone CLI (instead of as part of installation). However, I just remembered we already have something similar for KRA and TPS:

So KRA and TPS have several general purpose CLIs that can also be used to upgrade the database:

  • pki-server kra/tps-db-vlv-find for listing existing VLVs
  • pki-server kra/tps-db-vlv-add for creating VLVs defined in a file
  • pki-server kra/tps-db-vlv-reindex for running the VLV reindex task
  • pki-server kra/tps-db-vlv-del for deleting existing VLVs

The problem is they are currently broken since Python OpenLDAP library does not support NSS anymore so they need to be rewritten in Java.

I think it would be better if we break apart the current patch into a set of CA CLIs like above instead of adding everything into pki-server ca-db-upgrade. Later we probably can reuse the code to replace the broken KRA/TPS CLIs above.

print(' --action <action> update-vlv-indexes or:')
print(' fix-missing-issuer-names')
print(' (default: fix-missing-issuer-names)')
print(' --issuer_dn <Issuer DN> CA signing cert issuer dn')
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be --issuer-dn.

@edewata edewata requested a review from cipherboy April 20, 2020 21:29
@cipherboy
Copy link
Member

So... I think we need something more general. I don't fully know what all vlv indices do -- but I think we need something more extensive than just vlv indices.

I think it is in two parts:

  • Check if migration needs to happen
  • Code to do the migration

Not every migration will be cheap (computationally, ...) -- I agree Java would be a good idea. Mine is really easy, we're just adding a new object if it doesn't already exist. But we might in the future want something more advanced like scanning certs in the database and adding them to a new index. Maybe that can be done with VLV indices, idk.

I think we also want to be able to run some as part of server startup (add TPS auditor group should be really cheap) and others we want to be run manually... Using ldapjdk (if possible) I think would be best -- then we can use the existing LDAP credentials when possible.

shrugs

@cipherboy cipherboy removed their request for review February 4, 2022 01:57
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