[Freeipa-devel] [PATCH 0001] Add new parameter --ssh-update to ipa-client-install
Martin Štefany
martin at stefany.eu
Wed Mar 2 21:15:39 UTC 2016
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160302/6d9b267d/attachment.sig>
More information about the Freeipa-devel
mailing list