Re: [Freeipa-devel] [PATCH] 1012 validate domain in installer

2012-05-17 Thread Martin Kosek 
On Wed, 2012-05-16 at 14:04 -0400, Rob Crittenden wrote:
 Martin Kosek wrote:
  On Mon, 2012-05-14 at 17:29 -0400, Rob Crittenden wrote:
  Martin Kosek wrote:
  On Fri, 2012-05-11 at 16:03 -0400, Rob Crittenden wrote:
  Use our domain validator to validate the domain name we get in the
  installer.
 
  rob
 
  I found few issues with the patch:
 
  1) The unexpected error is not very user friendly error message:
 
  # ipa-server-install
  ...
  Server host name [vm-109.idm.lab.bos.redhat.com]:
 
  The domain name has been calculated based on the host name.
 
  Please confirm the domain name [idm.lab.bos.redhat.com]: bar-.com
 
  Unexpected error - see ipaserver-install.log for details:
 only letters, numbers, and - are allowed. DNS label may not start or
  end with -
 
 
  I think we should make it consistent with hostname validation error
  message format:
 
  # ipa-server-install
  ...
  Server host name [vm-109.idm.lab.bos.redhat.com]: foo-
 
  Invalid hostname 'foo-', must be fully-qualified.
 
 
  2) The error is also confusing when domain is passed as an option, user
  won't know what actually failed:
 
  # ipa-server-install --domain ..
  ...
  Server host name [vm-109.idm.lab.bos.redhat.com]:
 
  Unexpected error - see ipaserver-install.log for details:
 empty DNS label
 
  As for value passed via option, I think we should quit immediately and
  not in second or third interactive step - like we do for --zonemgr
  validation:
 
  # ipa-server-install --zonemgr=foo
  Usage: ipa-server-install [options]
 
  ipa-server-install: error: invalid zonemgr: missing address domain
 
  This applies both for --hostname and --domain options.
 
  Martin
 
 
  Done.
 
  There is a separate ticket to validate hostname in the parser. It is a
  bit more complicated since it depends on other options so I punted on that.
 
  rob
 
  Nack. Domain name passed via option is not used. I assume this is the
  reason:
 
  +def domain_callback(option, opt_str, value, parser):
  ...
  +
  +parser.values.zonemgr = value
  +
 
  Btw. callback is not necessary for domain validation, it may be done
  right in parse_options() in ipa-server-install with other checks.
 
  Martin
 
 
 Ouch, that was embarrassing. Took your advise and dumped the validator, 
 it isn't like this is going to be shared so yeah, it's overkill.
 
 I went back and forth whether to validate in read_domain_name() or not 
 and decided against it.
 
 rob

Yup, this one is ok :-) ACK, pushed to master.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 1012 validate domain in installer

2012-05-15 Thread Martin Kosek 
On Mon, 2012-05-14 at 17:29 -0400, Rob Crittenden wrote:
 Martin Kosek wrote:
  On Fri, 2012-05-11 at 16:03 -0400, Rob Crittenden wrote:
  Use our domain validator to validate the domain name we get in the
  installer.
 
  rob
 
  I found few issues with the patch:
 
  1) The unexpected error is not very user friendly error message:
 
  # ipa-server-install
  ...
  Server host name [vm-109.idm.lab.bos.redhat.com]:
 
  The domain name has been calculated based on the host name.
 
  Please confirm the domain name [idm.lab.bos.redhat.com]: bar-.com
 
  Unexpected error - see ipaserver-install.log for details:
only letters, numbers, and - are allowed. DNS label may not start or
  end with -
 
 
  I think we should make it consistent with hostname validation error
  message format:
 
  # ipa-server-install
  ...
  Server host name [vm-109.idm.lab.bos.redhat.com]: foo-
 
  Invalid hostname 'foo-', must be fully-qualified.
 
 
  2) The error is also confusing when domain is passed as an option, user
  won't know what actually failed:
 
  # ipa-server-install --domain ..
  ...
  Server host name [vm-109.idm.lab.bos.redhat.com]:
 
  Unexpected error - see ipaserver-install.log for details:
empty DNS label
 
  As for value passed via option, I think we should quit immediately and
  not in second or third interactive step - like we do for --zonemgr
  validation:
 
  # ipa-server-install --zonemgr=foo
  Usage: ipa-server-install [options]
 
  ipa-server-install: error: invalid zonemgr: missing address domain
 
  This applies both for --hostname and --domain options.
 
  Martin
 
 
 Done.
 
 There is a separate ticket to validate hostname in the parser. It is a 
 bit more complicated since it depends on other options so I punted on that.
 
 rob

Nack. Domain name passed via option is not used. I assume this is the
reason:

+def domain_callback(option, opt_str, value, parser):
...
+
+parser.values.zonemgr = value
+

Btw. callback is not necessary for domain validation, it may be done
right in parse_options() in ipa-server-install with other checks.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 1012 validate domain in installer

2012-05-14 Thread Martin Kosek 
On Fri, 2012-05-11 at 16:03 -0400, Rob Crittenden wrote:
 Use our domain validator to validate the domain name we get in the 
 installer.
 
 rob

I found few issues with the patch:

1) The unexpected error is not very user friendly error message:

# ipa-server-install 
...
Server host name [vm-109.idm.lab.bos.redhat.com]: 

The domain name has been calculated based on the host name.

Please confirm the domain name [idm.lab.bos.redhat.com]: bar-.com

Unexpected error - see ipaserver-install.log for details:
 only letters, numbers, and - are allowed. DNS label may not start or
end with -


I think we should make it consistent with hostname validation error
message format:

# ipa-server-install 
...
Server host name [vm-109.idm.lab.bos.redhat.com]: foo-

Invalid hostname 'foo-', must be fully-qualified.


2) The error is also confusing when domain is passed as an option, user
won't know what actually failed:

# ipa-server-install --domain ..
...
Server host name [vm-109.idm.lab.bos.redhat.com]: 

Unexpected error - see ipaserver-install.log for details:
 empty DNS label

As for value passed via option, I think we should quit immediately and
not in second or third interactive step - like we do for --zonemgr
validation:

# ipa-server-install --zonemgr=foo
Usage: ipa-server-install [options]

ipa-server-install: error: invalid zonemgr: missing address domain

This applies both for --hostname and --domain options.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 1012 validate domain in installer

2012-05-14 Thread Rob Crittenden 

Martin Kosek wrote:

On Fri, 2012-05-11 at 16:03 -0400, Rob Crittenden wrote:

Use our domain validator to validate the domain name we get in the
installer.

rob


I found few issues with the patch:

1) The unexpected error is not very user friendly error message:

# ipa-server-install
...
Server host name [vm-109.idm.lab.bos.redhat.com]:

The domain name has been calculated based on the host name.

Please confirm the domain name [idm.lab.bos.redhat.com]: bar-.com

Unexpected error - see ipaserver-install.log for details:
  only letters, numbers, and - are allowed. DNS label may not start or
end with -


I think we should make it consistent with hostname validation error
message format:

# ipa-server-install
...
Server host name [vm-109.idm.lab.bos.redhat.com]: foo-

Invalid hostname 'foo-', must be fully-qualified.


2) The error is also confusing when domain is passed as an option, user
won't know what actually failed:

# ipa-server-install --domain ..
...
Server host name [vm-109.idm.lab.bos.redhat.com]:

Unexpected error - see ipaserver-install.log for details:
  empty DNS label

As for value passed via option, I think we should quit immediately and
not in second or third interactive step - like we do for --zonemgr
validation:

# ipa-server-install --zonemgr=foo
Usage: ipa-server-install [options]

ipa-server-install: error: invalid zonemgr: missing address domain

This applies both for --hostname and --domain options.

Martin



Done.

There is a separate ticket to validate hostname in the parser. It is a 
bit more complicated since it depends on other options so I punted on that.


rob
From 6f306111a727b481838b77fbc4bf6a80b7b0cc89 Mon Sep 17 00:00:00 2001
From: Rob Crittenden rcrit...@redhat.com
Date: Fri, 11 May 2012 15:56:43 -0400
Subject: [PATCH] Call the domain validator on the user-provided domain name.

Wrap printing exceptions in unicode() to do Gettext conversion.

https://fedorahosted.org/freeipa/ticket/2196
---
 install/tools/ipa-server-install |   23 +--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install
index 1dd02ba870a02e902c4c345d9f5802ef09f78a7a..a6a98159bb7e44db8a1c37e2d55920d294d953c4 100755
--- a/install/tools/ipa-server-install
+++ b/install/tools/ipa-server-install
@@ -62,6 +62,7 @@ from ipapython.config import IPAOptionParser
 from ipalib.dn import DN
 from ipalib.x509 import load_certificate_from_file, load_certificate_chain_from_file
 from ipalib.constants import DNS_ZONE_REFRESH
+from ipalib.util import validate_domain_name
 from ipapython import services as ipaservices
 from ipapython.ipa_log_manager import *
 
@@ -96,6 +97,17 @@ def subject_callback(option, opt_str, value, parser):
 raise OptionValueError('Invalid subject base format: %s' % str(e))
 parser.values.subject = str(dn) # may as well normalize it
 
+def domain_callback(option, opt_str, value, parser):
+
+Validate the domain name
+
+try:
+validate_domain_name(value)
+except ValueError, e:
+parser.error(invalid domain:  + unicode(e))
+
+parser.values.zonemgr = value
+
 def validate_dm_password(password):
 if len(password)  8:
 raise ValueError(Password must be at least 8 characters long)
@@ -117,6 +129,9 @@ def parse_options():
 basic_group.add_option(-r, --realm, dest=realm_name,
   help=realm name)
 basic_group.add_option(-n, --domain, dest=domain_name,
+  action=callback,
+  callback=domain_callback,
+  type=string,
   help=domain name)
 basic_group.add_option(-p, --ds-password, dest=dm_password,
   sensitive=True, help=admin password)
@@ -727,6 +742,10 @@ def main():
 domain_name = options.domain_name
 
 domain_name = domain_name.lower()
+try:
+validate_domain_name(domain_name)
+except ValueError, e:
+sys.exit(Invalid domain name: %s % unicode(e))
 
 ip = get_server_ip_address(host_name, fstore, options.unattended, options)
 ip_address = str(ip)
@@ -1108,9 +1127,9 @@ try:
 except Exception, e:
 success = False
 if uninstalling:
-message = Unexpected error - see ipaserver-uninstall.log for details:\n %s % str(e)
+message = Unexpected error - see ipaserver-uninstall.log for details:\n %s % unicode(e)
 else:
-message = Unexpected error - see ipaserver-install.log for details:\n %s % str(e)
+message = Unexpected error - see ipaserver-install.log for details:\n %s % unicode(e)
 print message
 message = str(e)
 for str in traceback.format_tb(sys.exc_info()[2]):
-- 
1.7.10.1

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel