[Freeipa-devel] [PATCH] 1 Add ipa-adtrust-install utility

[Sumit Bose]

On Fri, Aug 26, 2011 at 02:08:27PM +0300, Alexander Bokovoy wrote:
> Hi,
> 
> On 26.08.2011 12:39, Sumit Bose wrote:
> > Hi,
> > 
> > with this patch an initial samba configuration for the AD trust feature
> > can be created by calling ipa-adtrust-install. Please be aware that you
> > will need a samba/master build to start smbd with the created
> > configuration, because only here all the needed features are available.
> > Günther is working on a spec file so that we can include a samba package
> > in the IPA development repository
> > (https://fedorahosted.org/freeipa/ticket/1610).
> 
> > +def parse_options():
> > +    parser = IPAOptionParser(version=version.VERSION)
> > +    parser.add_option("-p", "--ds-password", dest="dm_password",
> > +                      sensitive=True, help="admin password")
> If this is the only password you need, then make it --password. And it
> is Directory Manager's account password, right? Would be nice to change
> help to be more explicit.

ipa-server-install and ipa-dns-install use the same option for the same
purpose, so I thought it might be a good idea to use the same. But you
are right "admin password" is misleading here. Maybe the help should be
fixed in ipa-server-install and ipa-dns-install, too?

> 
> > +    parser.add_option("--ip-address", dest="ip_address",
> > +                      type="ip", ip_local=True, help="Master Server IP Address")
> 
> > +def main():
> > +    safe_options, options = parse_options()
> > +
> > +    if os.getegid() != 0:
> > +        sys.exit("Must be root to setup AD trusts on server")
> > +
> > +    installutils.check_server_configuration()
> > +
> > +    standard_logging_setup("/var/log/ipaserver-install.log", options.debug, filemode='a')
> > +    print "\nThe log file for this installation can be found in /var/log/ipaserver-install.log"
> > +
> > +    logging.debug('%s was invoked with options: %s' % (sys.argv[0], safe_options))
> > +    logging.debug("missing options might be asked for interactively later\n")
> > +
> > +    global fstore
> > +    fstore = sysrestore.FileStore('/var/lib/ipa/sysrestore')
> > +
> > +    print "=============================================================================="
> > +    print "This program will setup components neede to establish trust to AD domains for"
> Typo: "neede_d_"

fixed

> 
> > +    # Check we have a public IP that is associated with the hostname
> > +    if options.ip_address:
> > +        ip = options.ip_address
> I would also run options.ip_address through ipautil.CheckedIPAddress()
> to make sure it is correct and is one of local addresses.
> 
> > +    else:
> > +        hostaddr = resolve_host(api.env.host)
> > +        try:
> > +            ip = hostaddr and ipautil.CheckedIPAddress(hostaddr, match_local=True)
> > +        except Exception, e:
> > +            print "Error: Invalid IP Address %s: %s" % (ip, e)
> > +            ip = None
> > +
> > +    if not ip:
> > +        if options.unattended:
> > +            sys.exit("Unable to resolve IP address for host name")
> > +        else:
> > +            ip = read_ip_address(api.env.host, fstore)
> > +    ip_address = str(ip)
> > +    logging.debug("will use ip_address: %s\n", ip_address)
> And same here. You don't really want to blindly believe into what's entered.

fixed

> 
> > +    print "\tAdditionally you have to make sure the FreeIPA LDAP server cannot reached"
> > +    print "\tby any domain controller in the Active Directory domain by closing the"
> > +    print "\tfollowing ports for these servers:"
> > +    print "\t\tTCP Ports:"
> > +    print "\t\t  * 389, 636: LDAP/LDAPS"
> > +    print "\t\tUDP Ports:"
> > +    print "\t\t  * 389: (C)LDAP"
> > +    print "\tYou may want to choose to REJECT the packages instead of DROPing them to"
> s/packages/network packets/

fixed

> 
> > diff --git a/ipaserver/install/smbinstance.py b/ipaserver/install/smbinstance.py
> > new file mode 100644
> The code in smbinstance.py assumes Samba has been compiled with
> /etc/ipa/smb.conf as default configuration file location. Is that correct?
> 

no, __write_sysconfig_samba() adds "-s /etc/ipa/smb.conf" to
SMBDOPTIONS in /etc/sysconfig/samba.

Thanks for the review. I will send a new patch when I've fixed the
issues Simo found.

bye,
Sumit

> -- 
> / Alexander Bokovoy
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel