[Freeipa-devel] [PATCH 0001] Add new parameter --ssh-update to ipa-client-install
Martin Basti
mbasti at redhat.com
Wed Mar 2 16:51:38 UTC 2016
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..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
>>> 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 #
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160302/bed738e1/attachment.htm>
More information about the Freeipa-devel
mailing list