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

Set up DS TLS on replica in CA-less topology #355

Conversation

frasertweedale
Copy link
Contributor

@frasertweedale frasertweedale commented Dec 20, 2016

@HonzaCholasta
Copy link
Contributor

This is basically the same as 89de60c which had to be reverted because it is not the proper fix.

I would rather wait for the proper fix (#41), which has not been merged yet because it is blocked by https://bugzilla.redhat.com/show_bug.cgi?id=1377413.

@tkrizek
Copy link
Contributor

tkrizek commented Dec 20, 2016

89de60c was reveted because while it fixed this particular use case, it broke others. IIRC it broke regular replica promotion with CA.

The proper fix is not yet ready, nor on the IPA side (#41 is a step in the right direction, but it also requires some more code fixes, especially properly closing some ad hoc LDAP connections), nor on the NSS side (ETA unknown).

If this patch works and doesn't break other use cases, I would merge it and keep the ticket open. After the NSS bug is fixed, we can fix this properly.

@frasertweedale
Copy link
Contributor Author

FWIW, this one does not break CA-ful replica promotion.

@tkrizek tkrizek self-assigned this Dec 21, 2016
@tkrizek tkrizek self-requested a review December 21, 2016 09:12
@tkrizek
Copy link
Contributor

tkrizek commented Dec 21, 2016

I've tested the following use cases:

  • CA-less replica promotion domlvl1: ldapssl running; but the following behaviour is present: If ipa-ca-install is executed on replica, it finishes. But next ipa-ca-install, i.e. on master, will fail with CA did not start after 300 seconds. Relevant parts of pki and dirsrv logs:
[21/Dec/2016:12:43:46][localhost-startStop-1]: SSL handshake happened
Could not connect to LDAP server host vm-058-045.abc.idm.lab.eng.brq.redhat.com port 636 Error netscape.ldap.LDAPException: Authentication failed (48)
---
[21/Dec/2016:12:43:46.640540945 +0100] conn=4 fd=66 slot=66 SSL connection from 10.34.58.45 to 10.34.58.45
[21/Dec/2016:12:43:46.653170560 +0100] conn=4 TLS1.2 128-bit AES
[21/Dec/2016:12:43:46.665708312 +0100] conn=4 op=0 BIND dn="" method=sasl version=3 mech=EXTERNAL
[21/Dec/2016:12:43:46.667668986 +0100] conn=4 op=0 RESULT err=48 tag=97 nentries=0 etime=0

The same behavior is present when ipa-ca-install is first installed on master and then on replica. Basically, the second ipa-ca-install will fail. Running ipa-certupdate on the second server fixes the issue. This seems to be a separate issue, so I will file a bug for this.

  • CA-full replica promotion domlvl1: lpadssl running
  • CA-less replica installation domlvl0: ldapssl running
  • CA-full replica installation domlvl0: ldapssl running

The fix seems to properly start the ldapssl both with CA-less and CA-full, therefore I'd accept this as a proper fix for the issue. Please address the minor improvement I suggested inline.

@@ -392,6 +392,8 @@ def create_replica(self, realm_name, master_fqdn, fqdn,
if self.promote:
if self.ca_is_configured:
self.step("retrieving DS Certificate", self.__get_ds_cert)
elif self.pkcs12_info:
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be only two options: either pkcs12 is set (CA-less promotion) or not (->CA has to be configured). Perhaps we could modify the code to make this clearer:

if self.pkcs12_info:
   self.step("configuring ssl for ds instance", self.__enable_ssl)
else:
   self.step("retrieving DS Certificate", self.__get_ds_cert)

That way, if pkcs12_info is not set and CA is not configured, the installation will fail will retrieving the DS cert.

The same code that turns on ldapssl is both in __enable_ssl and __get_ds_cert. Ideally, we would want to avoid the code duplication to make it clearer what is actually going on, but that is out of scope for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomaskrizek this was implemented as requested. Bump for review.

@flo-renaud
Copy link
Contributor

@tomaskrizek FYI, the current documentation states that ipa-certupdate must be run after ipa-ca-install (see https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Linux_Domain_Identity_Authentication_and_Policy_Guide/CA-less-to-CA.html).

@MartinBasti
Copy link
Contributor

@tomaskrizek FYI, the current documentation states that ipa-certupdate must be run after ipa-ca-install (see https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Linux_Domain_Identity_Authentication_and_Policy_Guide/CA-less-to-CA.html).

Bad UX, please open a RFE ticket for ipa-ca-install to execute certupdate automatically when needed

@frasertweedale frasertweedale force-pushed the fix/6226-replica-install-ds-tls branch 2 times, most recently from 34ca89d to 9e2e1fb Compare December 22, 2016 03:38
@HonzaCholasta
Copy link
Contributor

@mbasti-rh, ipa-certupdate has to be run on all systems in the domain after installing a CA. How do you propose we do that from ipa-ca-install? Anyway, the behavior @tomaskrizek is observing happens if you don't run ipa-certupdate before ipa-ca-install on replica and is caused by ipa-ca-install using local files rather than LDAP when looking for CA certificates.

@pvoborni
Copy link
Member

Running ipa-certupdate on all systems after ipa-ca-install is problematic. But we can at least make sure that ipa-ca-install on replica will get whatever is possible automatically at beginning (e.g. run equivalent of ipa-certupdate) and also that it have usable state after finishing ipa-ca-install, i.e., it will run IPA whatever IPA calls it needs and possible. Anyway it is for other ticket. Maybe it already exists.

@MartinBasti
Copy link
Contributor

@jcholast anyway I still see ways how to improve UX

  • print big fat message to user at the end of ipa-ca-install to run ipa-certupdate everywhere when needed (instead of finding solution in the darkest corners in docs)
  • automatically run it at least on the current replica

Still worth ticket IMO

@pvoborni +1

@@ -390,7 +390,9 @@ def create_replica(self, realm_name, master_fqdn, fqdn,

self.step("creating DS keytab", self._request_service_keytab)
if self.promote:
if self.ca_is_configured:
if self.pkcs12_info:
self.step("configuring ssl for ds instance", self.__enable_ssl)
Copy link
Member

Choose a reason for hiding this comment

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

Can you change case for ssl and ds ?

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 are other occurrences of lower-case "ssl" and "ds" within strings in this file. (The string above is a copy-paste of another). I created a new ticket: https://fedorahosted.org/freeipa/ticket/6586. I will add a commit to this PR to address it.

@tkrizek
Copy link
Contributor

tkrizek commented Jan 3, 2017

I re-tested the most recent change in domlvl1. ldapssl is turned on both for CA-less replica install and CA-full replica install.

I also created a ticket for the above mentioned behavior. https://fedorahosted.org/freeipa/ticket/6577

@tkrizek tkrizek added the ack Pull Request approved, can be merged label Jan 3, 2017
@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Jan 5, 2017
@MartinBasti MartinBasti closed this Jan 5, 2017
@MartinBasti
Copy link
Contributor

Please provide PR for ipa-4-4 too

@frasertweedale
Copy link
Contributor Author

ipa-4-4 PR: #371

@frasertweedale frasertweedale deleted the fix/6226-replica-install-ds-tls branch January 5, 2017 23:34
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
7 participants