Make sure remote hosts have our keys #679

simo5 · 2017-03-31T16:23:54Z

In complex replication setups a replica may try to obtain CA keys from a
host that is not the master we initially create the keys against.
In this case race conditions may happen due to replication. So we need
to make sure the server we are contacting to get the CA keys has our
keys in LDAP. We do this by waiting to positively fetch our encryption
public key (the last one we create) from the target host LDAP server.

Fixes: https://pagure.io/freeipa/issue/6838

Signed-off-by: Simo Sorce simo@redhat.com

simo5 · 2017-03-31T16:30:57Z

I haven't tested this yet ... but what could possibily go wrong? :-)

tiran

ipaserver/install/custodiainstance.py:135: [W0612(unused-variable), CustodiaInstance.__wait_keys] Unused variable 'result')

ipaserver/install/custodiainstance.py:155: [E0602(undefined-variable), CustodiaInstance.__get_keys] Undefined variable 'sel')

ipaserver/install/custodiainstance.py:9: [W0611(unused-import), ] Unused ipaldap imported from ipapython)

ipaserver/install/custodiainstance.py:14: [W0611(unused-import), ] Unused replication imported from ipaserver.install)

All addressed

pvoborni · 2017-04-04T16:19:50Z

Shouldn't the ticket number be: https://pagure.io/freeipa/issue/6838 ?

simo5 · 2017-04-04T16:32:49Z

Seem like both errors are the same problem.
Should we mark 6688 a duplicate of 6838 ?

simo5 · 2017-04-04T16:33:59Z

Nevermind they are not duplicates.
I'll fix the commit message.

tiran

LGTM, but I wouldn't mind more logging.

stlaz · 2017-04-12T14:18:36Z

Fails with

2017-04-12T14:16:14Z DEBUG The ipa-replica-install command failed, exception: ValueError: Incorrect number of results (0) searching forpublic key for host/vm-225.abc.idm.lab.eng.brq.redhat.com@DOM-096.ABC.IDM.LAB.ENG.BRQ.REDHAT.COM

on first replica, every try.

stlaz · 2017-04-13T06:30:27Z

ipaserver/install/custodiainstance.py

+            try:
+                konn.get_key(KEY_USAGE_ENC, principal)
+                return
+            except Exception:


Could you use a more specific exception? I noticed that should not keys be retrieved, we're getting ValueError. We don't want to be waiting here if we're unable to connect to remote master.

What do you suggest ?

note that if we do not wait we fail intallation, not sure we gain anything by not waiting, and we can deal with a transient connection error if we wait.

I don't suggest not waiting, of course. I would have gone for just ValueError but you're probably right that the connection error might appear.
It might be worth to do this, at least:

exc = None while int(time.time()) < deadline: try: konn.get_key(KEY_USAGE_ENC, principal) break except Exception as e: if not isinstance(e, ValueError): root_logger.debug("Failed to get keys: '{err}'".format(err=e)) exc = e time.sleep(1) else: if exc is not None: raise exc else: raise RuntimeError("Unable to obtain keys in expected time.")

Note that this is solely for debugging reasons, I just don't like so broad exceptions.

Ok I handled this (but differently) and rebased on top of current master.
Please check it out @stlaz

pvoborni · 2017-05-02T07:40:58Z

What is this PR waiting for?

stlaz · 2017-05-02T07:49:23Z

I was expecting some action about my previous comment:

Fails with
2017-04-12T14:16:14Z DEBUG The ipa-replica-install command failed, exception: ValueError: Incorrect number of results (0) searching forpublic key for host/vm-225.abc.idm.lab.eng.brq.redhat.com@DOM-096.ABC.IDM.LAB.ENG.BRQ.REDHAT.COM
on first replica, every try.

I did not see any change in code to fix this but I can try again.

stlaz · 2017-05-02T16:08:37Z

Still fails.

simo5 · 2017-05-02T19:20:02Z

Can you please attach more of the logs before the failure ?

simo5 · 2017-05-02T20:19:01Z

@stlaz just FYI, I am sking this info because I cannot reproduce locally with a single replica.

simo5 · 2017-05-02T20:23:20Z

Nevermind I finally reproduced

In complex replication setups a replica may try to obtain CA keys from a host that is not the master we initially create the keys against. In this case race conditions may happen due to replication. So we need to make sure the server we are contacting to get the CA keys has our keys in LDAP. We do this by waiting to positively fetch our encryption public key (the last one we create) from the target host LDAP server. Fixes: https://pagure.io/freeipa/issue/6838 Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 · 2017-05-02T21:30:50Z

Turned out my master had some more relaxed permissions I added when developing the feature.
I now have added a new function to just check for the host keys without asking for data that cannot be read with the identity we have available.
This has been tested and seems to work correctly.
Please check @stlaz

stlaz · 2017-05-03T06:06:56Z

@simo5 will check, sorry for not replying yesterday, I was no more at my machine.

stlaz · 2017-05-03T07:39:17Z

ipaserver/secrets/kem.py

+        if len(r) != 1:
+            raise ValueError("Incorrect number of results (%d) searching for"
+                             "public key for %s" % (len(r), host))
+        return True


The return True seems a bit redundant.

I can remove it.

stlaz · 2017-05-03T07:49:28Z

Seems to work fine against current master, but fails with

Configuring ipa-custodia
  [1/4]: Generating ipa-custodia config file
  [2/4]: Generating ipa-custodia keys
  [3/4]: starting ipa-custodia 
  [4/4]: configuring ipa-custodia to start on boot
Done configuring ipa-custodia.
Your system may be partly configured.
Run /usr/sbin/ipa-server-install --uninstall to clean up.

ipa.ipapython.install.cli.install_tool(CompatServerReplicaInstall): ERROR    503 Server Error: Service Unavailable for url: https://master.domain.com/ipa/keys/ca/caSigningCert%20cert-pki-ca?type=kem&value=eyJhbGciOiJSU0EtT0FFUCIsImVuYyI6IkEyNTZDQkMtSFM1MTIiLCJraWQiOm51bGx9.k6y2jmI8oxRIsieU93_RzG5mZU_u_DPW2XL2jjLukYPZ3oZOkLkufof0fBeH6LAR66aL9m5C9j26GmhlTqNsm2FUQT7Xql975rYR3veooDwLQlPx6k4X1J4CTEeSsf7RVj8KfLE5e4K-nW1hTyepsbm7RDAA_-tbLvWzEqCQ0I3bfpPEDmlML08FA9T_yuPb1FkT0-lSCLV5PHya4tOB3R2q5CHC2b6BpwZQtbVW8eohshEmJMTO2NMAyPlfJscgSHYmhi6oliToV_Dh90Ej1UH_S0UOkHLsvIV5IoW4EGeaGdeHwHo4GsSGHGN3exVxWk9GShhJ_WJ-dlXSGQ_9CA.SfWWO_VrqzKKX3EYSh3E1Q.n4GtjcFZOQSZmAG9MShIQVtfRv_N3jEQMS46rLGUU6xIS-BYBL0Xq1UWP6VFrZW-g96Iqe2PIBhv4m1FsuAzP_gzac1lCr2ghcVuj3rAUg81G5s8vPuYNl_Ur5UVlQ2LtWzGLc26s1z_43MF7qCl8iayvXqnweK8_kj54F1RUJ-Awp0--Z4mnK_FFrPU4BBW2_EjZ1tOR8dV7NnxnN2Gd2tiDFl6Kkbj91rf6Bo2f8telN5RJsX52PsNW2z-l78TOIAKY4qfHhSVz31RO3xgUbyu3yQ79sGIxD66hzmVisB_LnbpNHbIjCP1wKEXXSo-IPrDtXk7ZWZrEITtItzynbzBKddVLjcNMjoqGz-lhLWVNg8R8rdHEdUzhlkdM-kFfW6Fz57wSyOZnt4KvQ-lZxY62TLQB1gqJ7vhzUPUs1g7C9rsy4gTQPjuRxXnLRvqXSb3arQPkrUl_hLqRuAm8FL-ClYY9G38KVns81QTygKvkDC8E5LQBJfyzkg93AyTXNBcrdCxP8AGgaxLBlGyEX-ya0g3mVX5fz_Uj6gyKjtOS_x1AUHOMkAMRmVEzvixrz-krCMWYOQDmJi19OlNeNjb7-NUVDxPRryr7e6Po2OqSbSjP6kUSw_QbMZf8BCrqV4TUFOwndTmZ68n1TOrCqie-UO71TJnherD_3m60_t3-Li1uy6_WWX66BBEMCCtsZBJWP7OYj7c9CzWGuzUEI7g75i4TZwoM1z0SjuyoPE.ZbRawj1B943OeF6AD_W0Z3pfk13fs14rbj_Ab8n-ZXI
ipa.ipapython.install.cli.install_tool(CompatServerReplicaInstall): ERROR    The ipa-replica-install command failed. See /var/log/ipareplica-install.log for more information

against 4.4.4 master.

simo5 · 2017-05-03T11:34:44Z

I've seen this once but thought it was a fluke due to my "unclean" master, as the following times it did not happen.
Can you reproduce the error against 4.4.4 consistently ?

stlaz · 2017-05-03T11:45:52Z

I was able to do it two times in a row with the same master, I can try to reinstall both the master and replica if you want. What do you mean "unclean"? It's a clean 4.4.4 master, no code changes, /etc/httpd/alias and /etc/pki/pki-tomcat/alias NSS databases seem fine, too.

edit: both have "custodia-0.3.1-1.fc25.noarch"

simo5 · 2017-05-03T11:54:01Z

I meant my setup was unclean.
I will try to reproduce here.
Does master w/o this patch work properly against 4.4.4 ?

stlaz · 2017-05-03T11:58:41Z

Not sure, I will try that.

stlaz · 2017-05-03T12:09:56Z

It seems that replica install fails even without this patch so it's OK to go with it?

simo5 · 2017-05-03T12:36:20Z

We need to find why it breaks though, but yeah I think we can go forward with this patch of others agree.
Can you open a separate bug for the failure you got ?

stlaz · 2017-05-03T13:40:29Z

Will do, ACKing this in the meantime.

stlaz · 2017-05-03T13:49:37Z

Removing the ACK to retest on 4.4.4 with Fedora custodia version.

tkrizek · 2017-05-03T14:37:51Z

ipa-4-5:

5f8d111 Make sure remote hosts have our keys

master:

1f9f84a Make sure remote hosts have our keys

simo5 force-pushed the cakeysfix branch 2 times, most recently from cefe3df to f2835bf Compare March 31, 2017 16:30

tiran previously requested changes Apr 3, 2017

View reviewed changes

simo5 force-pushed the cakeysfix branch from f2835bf to 23202d8 Compare April 3, 2017 12:55

simo5 force-pushed the cakeysfix branch from 23202d8 to 5d91032 Compare April 4, 2017 16:36

tiran approved these changes Apr 10, 2017

View reviewed changes

stlaz self-assigned this Apr 12, 2017

stlaz reviewed Apr 13, 2017

View reviewed changes

simo5 force-pushed the cakeysfix branch from 5d91032 to d81c657 Compare April 25, 2017 13:30

simo5 force-pushed the cakeysfix branch from d81c657 to 0e70f02 Compare May 2, 2017 21:28

stlaz reviewed May 3, 2017

View reviewed changes

stlaz added ack Pull Request approved, can be merged and removed ack Pull Request approved, can be merged labels May 3, 2017

stlaz added the ack Pull Request approved, can be merged label May 3, 2017

tkrizek added the pushed Pull Request has already been pushed label May 3, 2017

tkrizek closed this May 3, 2017

Make sure remote hosts have our keys #679

Make sure remote hosts have our keys #679

simo5 commented Mar 31, 2017 •

edited by tkrizek

simo5 commented Mar 31, 2017

tiran left a comment

pvoborni commented Apr 4, 2017

simo5 commented Apr 4, 2017

simo5 commented Apr 4, 2017

tiran left a comment

stlaz commented Apr 12, 2017 •

edited

stlaz Apr 13, 2017

simo5 Apr 13, 2017

simo5 Apr 13, 2017

stlaz Apr 18, 2017 •

edited

simo5 Apr 25, 2017

pvoborni commented May 2, 2017

stlaz commented May 2, 2017

stlaz commented May 2, 2017

simo5 commented May 2, 2017

simo5 commented May 2, 2017

simo5 commented May 2, 2017

simo5 commented May 2, 2017

stlaz commented May 3, 2017

stlaz May 3, 2017

simo5 May 3, 2017

stlaz commented May 3, 2017 •

edited

simo5 commented May 3, 2017

stlaz commented May 3, 2017 •

edited

simo5 commented May 3, 2017

stlaz commented May 3, 2017

stlaz commented May 3, 2017

simo5 commented May 3, 2017

stlaz commented May 3, 2017

stlaz commented May 3, 2017

tkrizek commented May 3, 2017

Make sure remote hosts have our keys #679

Make sure remote hosts have our keys #679

Conversation

simo5 commented Mar 31, 2017 • edited by tkrizek

simo5 commented Mar 31, 2017

tiran left a comment

Choose a reason for hiding this comment

pvoborni commented Apr 4, 2017

simo5 commented Apr 4, 2017

simo5 commented Apr 4, 2017

tiran left a comment

Choose a reason for hiding this comment

stlaz commented Apr 12, 2017 • edited

stlaz Apr 13, 2017

Choose a reason for hiding this comment

simo5 Apr 13, 2017

Choose a reason for hiding this comment

simo5 Apr 13, 2017

Choose a reason for hiding this comment

stlaz Apr 18, 2017 • edited

Choose a reason for hiding this comment

simo5 Apr 25, 2017

Choose a reason for hiding this comment

pvoborni commented May 2, 2017

stlaz commented May 2, 2017

stlaz commented May 2, 2017

simo5 commented May 2, 2017

simo5 commented May 2, 2017

simo5 commented May 2, 2017

simo5 commented May 2, 2017

stlaz commented May 3, 2017

stlaz May 3, 2017

Choose a reason for hiding this comment

simo5 May 3, 2017

Choose a reason for hiding this comment

stlaz commented May 3, 2017 • edited

simo5 commented May 3, 2017

stlaz commented May 3, 2017 • edited

simo5 commented May 3, 2017

stlaz commented May 3, 2017

stlaz commented May 3, 2017

simo5 commented May 3, 2017

stlaz commented May 3, 2017

stlaz commented May 3, 2017

tkrizek commented May 3, 2017

simo5 commented Mar 31, 2017 •

edited by tkrizek

stlaz commented Apr 12, 2017 •

edited

stlaz Apr 18, 2017 •

edited

stlaz commented May 3, 2017 •

edited

stlaz commented May 3, 2017 •

edited