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

CA-less installation fix #650

Closed
wants to merge 2 commits into from
Closed

CA-less installation fix #650

wants to merge 2 commits into from

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Mar 24, 2017

These patches fix the CA-less installation by guessing the names for CA and server-cert nicknames in /etc/httpd/alias. The fix is not very nice since it's guessing but I am not sure if there's anything else we can do at this point.

Also, HTTPInstance.start/stop_tracking_certificates would probably not need the guessing since it's only relevant for CA-full installations where we know the server-cert nickname is Server-Cert so I can replace it there if you think that'd be better.

# We only handle one server cert
nickname = server_certs[0][0]
if nickname == 'ipaCert':
nickname = server_certs[1][0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this is not safe, there are various reasons ipaCert might be present in /etc/httpd/alias even now (previous install of older IPA version, etc.)

@@ -441,7 +446,8 @@ def __import_ca_certs(self):
def __publish_ca_cert(self):
ca_db = certs.CertDB(self.realm, nssdir=paths.HTTPD_ALIAS_DIR,
subject_base=self.subject_base)
ca_db.publish_ca_cert(paths.CA_CRT)
ca_nickname = ca_db.find_root_cert(self.cert_nickname)[-1]
ca_db.export_pem_cert(ca_nickname, paths.CA_CRT)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you add this line at the end of __setup_ssl():

        self.cacert_name = db.cacert_name

You can replace this change and all of the other changes with this single line here, above ca_db.publish_ca_cert():

        ca_db.cacert_name = self.cacert_name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right, I did not realize this will be set to the correct value during the initialization from pkcs12.

@stlaz stlaz force-pushed the caless_fix branch 2 times, most recently from 5b8c0f3 to 1ab199a Compare March 24, 2017 11:12
@stlaz
Copy link
Contributor Author

stlaz commented Mar 24, 2017

Fixed according to the comments, thanks.

@HonzaCholasta
Copy link
Contributor

@stlaz, NSSDatabase.publish_ca_cert() and CertDB.publish_ca_cert() become unused after your changes, could we remove them?

@stlaz
Copy link
Contributor Author

stlaz commented Mar 27, 2017

Huh, I did not notice, thanks. Removing it, I could have removed the big fat comment about why not to use it.

@HonzaCholasta
Copy link
Contributor

@stlaz, please rebase.

During CA-less installation, we initialize the HTTPD alias
database from a pkcs12 file. This means there's going to
be different nicknames to the added certificates. Store
the CA certificate nickname in HTTPInstance__setup_ssl()
to be able to correctly export it later.

https://pagure.io/freeipa/issue/6806
@HonzaCholasta
Copy link
Contributor

I found additional bugs in CA-less (replica) install, but with this PR, publish_ca_cert does not fail anymore.

@HonzaCholasta HonzaCholasta added ack Pull Request approved, can be merged and removed ack Pull Request approved, can be merged labels Apr 3, 2017
@HonzaCholasta
Copy link
Contributor

Actually, there is a pylint failure introduced by this PR:

************* Module ipapython.certdb
ipapython/certdb.py:579: [C0305(trailing-newlines), ] Trailing newlines)

NSSDatabase.publish_ca_cert() is not used anymore, remove it.

https://pagure.io/freeipa/issue/6806
@stlaz
Copy link
Contributor Author

stlaz commented Apr 3, 2017

Sorry, must have screwed up the rebase.

@HonzaCholasta HonzaCholasta added the ack Pull Request approved, can be merged label Apr 3, 2017
@HonzaCholasta
Copy link
Contributor

@stlaz, please also provide a version of this PR rebased on ipa-4-5.

@stlaz
Copy link
Contributor Author

stlaz commented Apr 3, 2017

Done in #685

@HonzaCholasta
Copy link
Contributor

master:

  • 8c87014 Get correct CA cert nickname in CA-less
  • aae9a91 Remove publish_ca_cert() method from NSSDatabase

@HonzaCholasta HonzaCholasta added the pushed Pull Request has already been pushed label Apr 3, 2017
@stlaz stlaz deleted the caless_fix branch September 11, 2017 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
2 participants