<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 27.02.2016 21:19, Martin Štefany wrote: </div> <blockquote cite="mid:1456604366.2844.13.camel@stefany.eu" type="cite"> <pre wrap="">Hi, I did as Jan suggested, everything is now a new command 'ipa-sshupdate', (so it's based on Jan's 'ipa-certupdate', yeah, a bit of copy-paste), rest is based on ipa-client-install's code. I'm not sure if this is correct, but you might want to change ipa-client-install to just 'import ipaclient.ipa_sshupdate' for ssh update, or not - I'm not sure how this is compatible with 'code deduplication', 're-usage', etc. Another open point from my side is PEP8 compliance, I've ran the new code through pep8 utility with defaults and it's 'OK'. But so is code in my employer's project and they look slightly 'different', mainly for brackets, strings, etc. Please, have a look to that, too, I'm happy for any guidance. Martin On Št, 2016-02-25 at 14:36 +0100, Jan Cholasta wrote: </pre> <blockquote type="cite"> <pre wrap="">Hi, On 25.2.2016 14:23, Martin Basti wrote: </pre> <blockquote type="cite"> <pre wrap=""> On 22.02.2016 22:13, Martin Štefany wrote: </pre> <blockquote type="cite"> <pre wrap=""> Hi, please, review the attached patch which adds --ssh-update to ipa- client- install. Ticket:<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/2655">https://fedorahosted.org/freeipa/ticket/2655</a> </pre> </blockquote> <pre wrap="">Hello, thank you for your patch. Please attach a patch as a file next time. I have doubts that this should be part of ipa-client-install, this needs a broader discussion. </pre> </blockquote> <pre wrap="">+1, I think it should be a separate command (ignore my earlier suggestion from Trac to incorporate this into ipa-client-install, I was young and stupid). See client/ipa-certupdate and ipaclient/ipa_certupdate.py for an example of how such a command should be implemented. </pre> <blockquote type="cite"> <pre wrap=""> Code comments inline: </pre> <blockquote type="cite"> <pre wrap=""> --- Martin </pre> <blockquote type="cite"> <pre wrap=""> >From 4974a57f48a0cd48b83724297ae2af572bc530eb Mon Sep 17 00:00:00 2001 </pre> </blockquote> <pre wrap="">From: Martin Stefany <martin stefany eu> Date: Mon, 22 Feb 2016 20:58:13 +0000 Subject: [PATCH] Add new parameter --ssh-update to ipa-client- install Add a new parameter '--ssh-update' which can be used later after freeipa client is installed to update SSH hostkeys and SSHFP DNS records for host. <a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/2655">https://fedorahosted.org/freeipa/ticket/2655</a> --- ipa-client/ipa-install/ipa-client-install | 102 +++++++++++++++++++++++++++++- 1 file changed, 99 insertions(+), 3 deletions(-) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa- client/ipa- install/ipa-client-install index 789ff591591673744ee3b922e5c0181233ad553c..97adfb6b449fb441bddada89 a3b151 33e398ca50 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -71,6 +71,7 @@ CLIENT_INSTALL_ERROR = 1 CLIENT_NOT_CONFIGURED = 2 CLIENT_ALREADY_CONFIGURED = 3 CLIENT_UNINSTALL_ERROR = 4 # error after restoring files/state +CLIENT_SSHUPDATE_ERROR = 5 # error during update of SSH public keys def parse_options(): def validate_ca_cert_file_option(option, opt, value, parser): @@ -215,6 +216,12 @@ def parse_options(): "be run with -- unattended option") parser.add_option_group(uninstall_group) + sshupdate_group = OptionGroup(parser, "SSH key update options") + sshupdate_group.add_option("--ssh-update", dest="ssh_update", + action="store_true", default=False, + help="update local host's SSH public keys in host entry and DNS.") + parser.add_option_group(sshupdate_group) + options, args = parser.parse_args() safe_opts = parser.get_safe_opts(options) @@ -840,6 +847,92 @@ def uninstall(options, env): return rv +def sshupdate(options, env): + if not is_ipa_client_installed(): + root_logger.error("IPA client is not configured on this system.") + return CLIENT_NOT_CONFIGURED + + api.bootstrap(context='cli_installer', debug=options.debug) + api.finalize() + if 'config_loaded' not in api.env: + root_logger.error("Failed to initialize IPA API.") + return CLIENT_SSHUPDATE_ERROR + + # Now, let's try to connect to the server's RPC interface + connected = False + try: + api.Backend.rpcclient.connect() + connected = True + root_logger.debug("Try RPC connection") + api.Backend.rpcclient.forward('ping') + except errors.KerberosError as e: + if connected: + api.Backend.rpcclient.disconnect() + root_logger.info( + "Cannot connect to the server due to Kerberos error: %s. " + "Trying with delegate=True", e) + try: + api.Backend.rpcclient.connect(delegate=True) + root_logger.debug("Try RPC connection") + api.Backend.rpcclient.forward('ping') + + root_logger.info("Connection with delegate=True successful") + + # The remote server is not capable of Kerberos S4U2Proxy + # delegation. This features is implemented in IPA server + # version 2.2 and higher + root_logger.warning( + "Target IPA server has a lower version than the enrolled " + "client") + root_logger.warning( + "Some capabilities including the ipa command capability " + "may not be available") + except errors.PublicError as e2: + root_logger.warning( + "Second connect with delegate=True also failed: %s", e2) + root_logger.error( + "Cannot connect to the IPA server RPC interface: %s", e2) + return CLIENT_SSHUPDATE_ERROR + except errors.PublicError as e: + root_logger.error( + "Cannot connect to the server due to generic error: %s", e) + return CLIENT_SSHUPDATE_ERROR </pre> </blockquote> <pre wrap="">I think you should be kinited with client keytab, client is allowed to modify its SSHpublic keys in ldap. I'd only require to be root to execute it. kinit -kt /etc/krb5.keytab host/`hostname` ipa host-mod `hostname` --sshpubkey="something" Also this rpcconnection looks to me too much complicated, I think it should be just simple rpcconnect </pre> <blockquote type="cite"> <pre wrap=""> + + # We need to pull IPA server address from default.conf + try: + parser = RawConfigParser() + parser.read(paths.IPA_DEFAULT_CONF) + cli_realm = parser.get('global', 'realm') + hostname = parser.get('global', 'host') + # TODO: consult with review team + # except ConfigParser.NoSectionError as e: + # pass + # except ConfigParser.ParsingError as e: + # pass + finally: + pass </pre> </blockquote> <pre wrap="">You can raise error there. </pre> <blockquote type="cite"> <pre wrap=""> + + host_principal = 'host/%s@%s' % (hostname, cli_realm) + # Obtain the TGT. We do it with the temporary krb5.conf, so that + # only the KDC we're installing under is contacted. + # Other KDCs might not have replicated the principal yet. + # Once we have the TGT, it's usable on any server. </pre> </blockquote> <pre wrap="">I don't think that temporary krb5.conf should be used here </pre> <blockquote type="cite"> <pre wrap=""> + try: + ipautil.kinit_keytab(host_principal, paths.KRB5_KEYTAB, + CCACHE_FILE, + # config=krb_name, + attempts=options.kinit_attempts) + env['KRB5CCNAME'] = os.environ['KRB5CCNAME'] = CCACHE_FILE + except Krb5Error as e: + print_port_conf_info() + root_logger.error("Failed to obtain host TGT: %s" % e) + # failure to get ticket makes it impossible to login and bind + # from sssd to LDAP, abort installation and rollback changes + return CLIENT_INSTALL_ERROR </pre> </blockquote> <pre wrap="">This is not install error. </pre> <blockquote type="cite"> <pre wrap=""> + + # passing server parameter seems unneccessary, thus passing only "" + update_ssh_keys("", hostname, services.knownservices.sshd.get_config_dir(), options.create_sshfp) + def configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server, hostname): ipaconf = ipaclient.ipachangeconf.IPAChangeConf("IPA Installer") ipaconf.setOptionAssignment(" =") @@ -2797,7 +2890,7 @@ def install(options, env, fstore, statestore): connected = True root_logger.debug("Try RPC connection") api.Backend.rpcclient.forward('ping') - except errors.KerberosError, e: + except errors.KerberosError as e: </pre> </blockquote> <pre wrap="">Please don't modify code that already exists and it is not related to this change </pre> <blockquote type="cite"> <pre wrap=""> if connected: api.Backend.rpcclient.disconnect() root_logger.info( @@ -2820,13 +2913,13 @@ def install(options, env, fstore, statestore): root_logger.warning( "Some capabilities including the ipa command capability " "may not be available") - except errors.PublicError, e2: + except errors.PublicError as e2: </pre> </blockquote> <pre wrap="">Remove this from patch too </pre> <blockquote type="cite"> <pre wrap=""> root_logger.warning( "Second connect with delegate=True also failed: %s", e2) root_logger.error( "Cannot connect to the IPA server RPC interface: %s", e2) return CLIENT_INSTALL_ERROR - except errors.PublicError, e: + except errors.PublicError as e: </pre> </blockquote> <pre wrap="">and this too </pre> <blockquote type="cite"> <pre wrap=""> root_logger.error( "Cannot connect to the server due to generic error: %s", e) return CLIENT_INSTALL_ERROR @@ -3088,6 +3181,9 @@ def main(): if options.uninstall: return uninstall(options, env) + if options.ssh_update: + return sshupdate(options, env) + if is_ipa_client_installed(on_master=options.on_master): root_logger.error("IPA client is already configured on this system.") root_logger.info( -- 1.8.3.1 </pre> </blockquote> <pre wrap="">Martin^2 </pre> </blockquote> <pre wrap="">Honza </pre> </blockquote> </blockquote> Thanks, I have a few comments 1) Please use new license format in header of the new files <meta http-equiv="content-type" content="text/html; charset=utf-8"> <pre style="background-color:#ffffff;color:#000000;font-family:'DejaVu Sans Mono';font-size:9.0pt;"># # Copyright (C) 2016 FreeIPA Contributors see COPYING for license #</pre> 2) This is very bad, I sent patch to fix it in client code + except ValueError as UnicodeDecodeError: + continue it should be except (ValueError, UnicodeDecodeError): and maybe debug log? 3) I see many errors, respectively not so nice code there, but I realized that everything is from ipa-client-install. I think it would be better to extract update_ssh_keys, and do_nsupdate to separate module and reuse it in both scripts. I have to find out which (ipaclient, ipalib, ...) module are the best. Then fix issues with these functions. stay tuned :) Martin^2 </body> </html>