[Freeipa-devel] [PATCH 0001] Add new parameter --ssh-update to ipa-client-install
Jan Cholasta
jcholast at redhat.com
Thu Mar 3 07:18:31 UTC 2016
On 2.3.2016 22:15, Martin Štefany wrote:
> 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.
+1
>>
>> I have to find out which (ipaclient, ipalib, ...) module are the best.
ipaclient, obviously.
>>
>> Then fix issues with these functions.
4) I would prefer if the script was named "ipa-client-sshinstall", as
that's consistent with "ipa-server-certinstall", which does a similar
thing with certificates on server.
--
Jan Cholasta
More information about the Freeipa-devel
mailing list