[Freeipa-devel] [PATCH 0001] Add new parameter --ssh-update to ipa-client-install
Jan Cholasta
jcholast at redhat.com
Mon Mar 7 06:56:16 UTC 2016
On 3.3.2016 08:18, Jan Cholasta wrote:
> 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.
Specifically, ipaclient/install/ssh.py.
>
>>>
>>> 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.
I meant "ipa-cacert-manage install", "ipa-server-certinstall" installs
the certs locally, it does not upload them to IPA. The point is, we use
the verb "install" when something is transmitted from the user to IPA.
--
Jan Cholasta
More information about the Freeipa-devel
mailing list