Hi, On St, 2016-03-02 at 17:51 +0100, Martin Basti wrote: > > > On 27.02.2016 21:19, Martin Štefany wrote: > > 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: > > > Hi, > > > > > > On 25.2.2016 14:23, Martin Basti wrote: > > > > > > > > > > > > On 22.02.2016 22:13, Martin Štefany wrote: > > > > > Hi, > > > > > > > > > > please, review the attached patch which adds --ssh-update to > > > > > ipa- > > > > > client- > > > > > install. > > > > > > > > > > Ticket:https://fedorahosted.org/freeipa/ticket/2655 > > > > 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. > > > +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. > > > > > > > > > > > Code comments inline: > > > > > > > > > > --- > > > > > Martin > > > > > > > > > > > From 4974a57f48a0cd48b83724297ae2af572bc530eb Mon Sep 17 > > > > > > 00:00:00 2001 > > > > > 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. > > > > > > > > > > https://fedorahosted.org/freeipa/ticket/2655 > > > > > --- > > > > > 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..97adfb6b449fb441bdda > > > > > da89 > > > > > 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 > > > > 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 > > > > > > > > > + > > > > > + # 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 > > > > You can raise error there. > > > > > > > > > + > > > > > + 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. > > > > I don't think that temporary krb5.conf should be used here > > > > > + 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 > > > > This is not install error. > > > > > > > > > + > > > > > + # 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: > > > > Please don't modify code that already exists and it is not > > > > related > > > > to > > > > this change > > > > > 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: > > > > Remove this from patch too > > > > > 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: > > > > and this too > > > > > 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 > > > > > > > > > > > > > > Martin^2 > > > > > > > > > > > Honza > > Thanks, > > I have a few comments > > 1) > Please use new license format in header of the new files > > # > # Copyright (C) 2016 FreeIPA Contributors see COPYING for license > # OK > > 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? Something like? :
+ for line in f: + line = line[:-1].lstrip() + if not line or line.startswith('#'): + continue + try: + pubkey = ssh.SSHPublicKey(line) + self.log.info("Adding SSH public key from %s", filename) + pubkeys.append(pubkey) + except (ValueError, UnicodeDecodeError) as e: + self.log.debug( + "Skipping SSH public key from %s due to error: %s", + filename, e + ) I had to move it inside try-except clause since not assigning pubkey and handling exception causes then "exception: UnboundLocalError: local variable 'pubkey' referenced before assignment" :\ Should it be debug or warning? > > 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 > Thank you! Regards, Martin
signature.asc
Description: This is a digitally signed message part
-- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code