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

--
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

Reply via email to