Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-12-06 Thread Rob Crittenden

Jakub Hrozek wrote:

On Mon, Nov 12, 2012 at 05:42:21PM +0100, Jan Cholasta wrote:

On 9.11.2012 17:58, Jakub Hrozek wrote:

On Wed, Nov 07, 2012 at 05:20:30PM +0100, Martin Kosek wrote:

On 11/07/2012 12:53 AM, Jakub Hrozek wrote:

On Wed, Oct 31, 2012 at 01:22:52PM +0100, Martin Kosek wrote:

On 10/31/2012 11:00 AM, Jakub Hrozek wrote:

On Mon, Oct 22, 2012 at 02:14:00PM -0400, Simo Sorce wrote:

On Mon, 2012-10-22 at 17:15 +0200, Martin Kosek wrote:

On 10/08/2012 08:27 PM, Rob Crittenden wrote:

Jakub Hrozek wrote:

On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:



- Original Message -

Hi,

the attached patches add the directory the SSSD writes domain-realm
mappings as includedir to krb5.conf when installing the client.

[PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for
options
ipachangeconf only allows one delimeter between keys and values. This
patch adds the possibility of also specifying delim in the option
dictionary to override the default delimeter.

On a slightly-unrelated note, we really should think about adopting
Augeas. Changing configuration with home-grown scripts is getting
tricky.

[PATCH 2/3] Specify includedir in krb5.conf on new installs
This patch utilizes the new functionality from the previous patch to
add
the includedir on top of the krb5.conf file

[PATCH 3/3] Add the includedir to krb5.conf on upgrades
This patch is completely untested and I'm only posting it to get
opinions. At first I was going to use an upgrade script in %post but
then I thought it would be overengineering when all we want to do is
prepend one line.. Would a simple munging like this be acceptable or
shall I write a full script?


NACK, using a scriptlet is fine, but not the way you did, as it has a huge
race condition where krb5.conf exists and has only one line in it (the
include line).

You should first create the new file: echo include ...  /etc/krb.conf.ipanew
Then cat the contents of the existing file in i:t cat /etc/krb.conf 
/etc/krb.conf.ipanew
And finally atomically rename it: mv /etc/krb.conf.ipanew /etc/krb.conf

This method is also safe wrt something killing the yum process ...

Simo.


I'm attaching a new revision of the patches not even two months after
the original nack.

I also think it might be nice to have a more general way of upgrading
the client config so I filed
https://fedorahosted.org/freeipa/ticket/3149


I don't think grepping for a string is an effective way to determine if the
client has been configured. Someone could have removed that line.

I'd prefer using /var/lib/ipa-client/sysrestore/sysrestore.index. If it exists
and has more than 2 lines in it ([files] + one other file) then it is safe to
say the client is configured, or at least partially configured.

rob



I just found one more issue. What if ipa-client-install is run with --no-sssd
option? In that case I assume we should not include the SSSD's krb5.include.d,
right? The same would also appy for upgrades, we would need to check if client
was actually configured with SSSD before mangling their krb5.conf.


Yeah that's right, we should have all sssd related changes under a
conditional that is true only when sssd is enabled.

Simo.


OK, new patches are attached. In new installs, the includedir is only
added when options.sssd is true. During upgrades, I checked for
sssd.conf's existence, I'm not sure if there's any other way to check if
the client was configured with sssd?


Hello Jakub, thanks for these patches. I think that checking if /etc/sssd.conf
exists as actually not so bad way to test if it was configured. Anyway, I did
few tests on server and client but I still see few issues:

1) SELinux context of krb5.conf is not as expected (but I am not sure what real
issue could that cause):

# restorecon -FvvR /etc/krb5.conf
restorecon reset /etc/krb5.conf context
unconfined_u:object_r:etc_t:s0-system_u:object_r:krb5_conf_t:s0



Fixed. Thanks, I shouldn have noticed that doing mv would just replace
the original context.


2) I can no longer kinit on IPA server after applying your patch
# rpm -q sssd
sssd-1.9.90-0.20121030T1436Zgitf46bf56.fc17.x86_64
# rpm -Uvh --force freeipa-*.rpm
# head -n 5 /etc/krb5.conf
includedir /var/lib/sss/pubconf/krb5.include.d/
[logging]
  default = FILE:/var/log/krb5libs.log
  kdc = FILE:/var/log/krb5kdc.log
  admin_server = FILE:/var/log/kadmind.log
# KRB5_TRACE=/dev/stdout kinit admin
[21059] 1351684052.658548: Getting initial credentials for
ad...@idm.lab.bos.redhat.com
[21059] 1351684052.665269: Sending request (200 bytes) to IDM.LAB.BOS.REDHAT.COM
[21059] 1351684052.665989: Resolving hostname vm-044.idm.lab.bos.redhat.com
[21059] 1351684052.667511: Sending initial UDP request to dgram 10.16.78.44:88
[21059] 1351684052.672514: Received answer from dgram 10.16.78.44:88
[21059] 1351684052.672653: Response was from master KDC
[21059] 1351684052.672751: Received error from KDC: -1765328370/KDC has no
support for encryption type
kinit: KDC has no 

Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-11-13 Thread Jakub Hrozek
On Mon, Nov 12, 2012 at 05:42:21PM +0100, Jan Cholasta wrote:
 On 9.11.2012 17:58, Jakub Hrozek wrote:
 On Wed, Nov 07, 2012 at 05:20:30PM +0100, Martin Kosek wrote:
 On 11/07/2012 12:53 AM, Jakub Hrozek wrote:
 On Wed, Oct 31, 2012 at 01:22:52PM +0100, Martin Kosek wrote:
 On 10/31/2012 11:00 AM, Jakub Hrozek wrote:
 On Mon, Oct 22, 2012 at 02:14:00PM -0400, Simo Sorce wrote:
 On Mon, 2012-10-22 at 17:15 +0200, Martin Kosek wrote:
 On 10/08/2012 08:27 PM, Rob Crittenden wrote:
 Jakub Hrozek wrote:
 On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:
 
 
 - Original Message -
 Hi,
 
 the attached patches add the directory the SSSD writes domain-realm
 mappings as includedir to krb5.conf when installing the client.
 
 [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter 
 for
 options
 ipachangeconf only allows one delimeter between keys and values. 
 This
 patch adds the possibility of also specifying delim in the option
 dictionary to override the default delimeter.
 
 On a slightly-unrelated note, we really should think about adopting
 Augeas. Changing configuration with home-grown scripts is getting
 tricky.
 
 [PATCH 2/3] Specify includedir in krb5.conf on new installs
 This patch utilizes the new functionality from the previous patch 
 to
 add
 the includedir on top of the krb5.conf file
 
 [PATCH 3/3] Add the includedir to krb5.conf on upgrades
 This patch is completely untested and I'm only posting it to get
 opinions. At first I was going to use an upgrade script in %post 
 but
 then I thought it would be overengineering when all we want to do 
 is
 prepend one line.. Would a simple munging like this be acceptable 
 or
 shall I write a full script?
 
 NACK, using a scriptlet is fine, but not the way you did, as it has 
 a huge
 race condition where krb5.conf exists and has only one line in it 
 (the
 include line).
 
 You should first create the new file: echo include ...  
 /etc/krb.conf.ipanew
 Then cat the contents of the existing file in i:t cat /etc/krb.conf 
 
 /etc/krb.conf.ipanew
 And finally atomically rename it: mv /etc/krb.conf.ipanew 
 /etc/krb.conf
 
 This method is also safe wrt something killing the yum process ...
 
 Simo.
 
 I'm attaching a new revision of the patches not even two months after
 the original nack.
 
 I also think it might be nice to have a more general way of upgrading
 the client config so I filed
 https://fedorahosted.org/freeipa/ticket/3149
 
 I don't think grepping for a string is an effective way to determine 
 if the
 client has been configured. Someone could have removed that line.
 
 I'd prefer using /var/lib/ipa-client/sysrestore/sysrestore.index. If 
 it exists
 and has more than 2 lines in it ([files] + one other file) then it is 
 safe to
 say the client is configured, or at least partially configured.
 
 rob
 
 
 I just found one more issue. What if ipa-client-install is run with 
 --no-sssd
 option? In that case I assume we should not include the SSSD's 
 krb5.include.d,
 right? The same would also appy for upgrades, we would need to check 
 if client
 was actually configured with SSSD before mangling their krb5.conf.
 
 Yeah that's right, we should have all sssd related changes under a
 conditional that is true only when sssd is enabled.
 
 Simo.
 
 OK, new patches are attached. In new installs, the includedir is only
 added when options.sssd is true. During upgrades, I checked for
 sssd.conf's existence, I'm not sure if there's any other way to check if
 the client was configured with sssd?
 
 Hello Jakub, thanks for these patches. I think that checking if 
 /etc/sssd.conf
 exists as actually not so bad way to test if it was configured. Anyway, I 
 did
 few tests on server and client but I still see few issues:
 
 1) SELinux context of krb5.conf is not as expected (but I am not sure 
 what real
 issue could that cause):
 
 # restorecon -FvvR /etc/krb5.conf
 restorecon reset /etc/krb5.conf context
 unconfined_u:object_r:etc_t:s0-system_u:object_r:krb5_conf_t:s0
 
 
 Fixed. Thanks, I shouldn have noticed that doing mv would just replace
 the original context.
 
 2) I can no longer kinit on IPA server after applying your patch
 # rpm -q sssd
 sssd-1.9.90-0.20121030T1436Zgitf46bf56.fc17.x86_64
 # rpm -Uvh --force freeipa-*.rpm
 # head -n 5 /etc/krb5.conf
 includedir /var/lib/sss/pubconf/krb5.include.d/
 [logging]
   default = FILE:/var/log/krb5libs.log
   kdc = FILE:/var/log/krb5kdc.log
   admin_server = FILE:/var/log/kadmind.log
 # KRB5_TRACE=/dev/stdout kinit admin
 [21059] 1351684052.658548: Getting initial credentials for
 ad...@idm.lab.bos.redhat.com
 [21059] 1351684052.665269: Sending request (200 bytes) to 
 IDM.LAB.BOS.REDHAT.COM
 [21059] 1351684052.665989: Resolving hostname 
 vm-044.idm.lab.bos.redhat.com
 [21059] 1351684052.667511: Sending initial UDP request to dgram 
 10.16.78.44:88
 [21059] 1351684052.672514: Received answer from dgram 10.16.78.44:88
 [21059] 1351684052.672653: Response was from master KDC
 

Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-11-09 Thread Jakub Hrozek
On Wed, Nov 07, 2012 at 05:20:30PM +0100, Martin Kosek wrote:
 On 11/07/2012 12:53 AM, Jakub Hrozek wrote:
  On Wed, Oct 31, 2012 at 01:22:52PM +0100, Martin Kosek wrote:
  On 10/31/2012 11:00 AM, Jakub Hrozek wrote:
  On Mon, Oct 22, 2012 at 02:14:00PM -0400, Simo Sorce wrote:
  On Mon, 2012-10-22 at 17:15 +0200, Martin Kosek wrote:
  On 10/08/2012 08:27 PM, Rob Crittenden wrote:
  Jakub Hrozek wrote:
  On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:
 
 
  - Original Message -
  Hi,
 
  the attached patches add the directory the SSSD writes domain-realm
  mappings as includedir to krb5.conf when installing the client.
 
  [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter 
  for
  options
  ipachangeconf only allows one delimeter between keys and values. 
  This
  patch adds the possibility of also specifying delim in the option
  dictionary to override the default delimeter.
 
  On a slightly-unrelated note, we really should think about adopting
  Augeas. Changing configuration with home-grown scripts is getting
  tricky.
 
  [PATCH 2/3] Specify includedir in krb5.conf on new installs
  This patch utilizes the new functionality from the previous patch to
  add
  the includedir on top of the krb5.conf file
 
  [PATCH 3/3] Add the includedir to krb5.conf on upgrades
  This patch is completely untested and I'm only posting it to get
  opinions. At first I was going to use an upgrade script in %post but
  then I thought it would be overengineering when all we want to do is
  prepend one line.. Would a simple munging like this be acceptable or
  shall I write a full script?
 
  NACK, using a scriptlet is fine, but not the way you did, as it has 
  a huge
  race condition where krb5.conf exists and has only one line in it 
  (the
  include line).
 
  You should first create the new file: echo include ...  
  /etc/krb.conf.ipanew
  Then cat the contents of the existing file in i:t cat /etc/krb.conf 
  
  /etc/krb.conf.ipanew
  And finally atomically rename it: mv /etc/krb.conf.ipanew 
  /etc/krb.conf
 
  This method is also safe wrt something killing the yum process ...
 
  Simo.
 
  I'm attaching a new revision of the patches not even two months after
  the original nack.
 
  I also think it might be nice to have a more general way of upgrading
  the client config so I filed
  https://fedorahosted.org/freeipa/ticket/3149
 
  I don't think grepping for a string is an effective way to determine 
  if the
  client has been configured. Someone could have removed that line.
 
  I'd prefer using /var/lib/ipa-client/sysrestore/sysrestore.index. If 
  it exists
  and has more than 2 lines in it ([files] + one other file) then it is 
  safe to
  say the client is configured, or at least partially configured.
 
  rob
 
 
  I just found one more issue. What if ipa-client-install is run with 
  --no-sssd
  option? In that case I assume we should not include the SSSD's 
  krb5.include.d,
  right? The same would also appy for upgrades, we would need to check if 
  client
  was actually configured with SSSD before mangling their krb5.conf.
 
  Yeah that's right, we should have all sssd related changes under a
  conditional that is true only when sssd is enabled.
 
  Simo.
 
  OK, new patches are attached. In new installs, the includedir is only
  added when options.sssd is true. During upgrades, I checked for
  sssd.conf's existence, I'm not sure if there's any other way to check if
  the client was configured with sssd?
 
  Hello Jakub, thanks for these patches. I think that checking if 
  /etc/sssd.conf
  exists as actually not so bad way to test if it was configured. Anyway, I 
  did
  few tests on server and client but I still see few issues:
 
  1) SELinux context of krb5.conf is not as expected (but I am not sure what 
  real
  issue could that cause):
 
  # restorecon -FvvR /etc/krb5.conf
  restorecon reset /etc/krb5.conf context
  unconfined_u:object_r:etc_t:s0-system_u:object_r:krb5_conf_t:s0
 
  
  Fixed. Thanks, I shouldn have noticed that doing mv would just replace
  the original context.
  
  2) I can no longer kinit on IPA server after applying your patch
  # rpm -q sssd
  sssd-1.9.90-0.20121030T1436Zgitf46bf56.fc17.x86_64
  # rpm -Uvh --force freeipa-*.rpm
  # head -n 5 /etc/krb5.conf
  includedir /var/lib/sss/pubconf/krb5.include.d/
  [logging]
   default = FILE:/var/log/krb5libs.log
   kdc = FILE:/var/log/krb5kdc.log
   admin_server = FILE:/var/log/kadmind.log
  # KRB5_TRACE=/dev/stdout kinit admin
  [21059] 1351684052.658548: Getting initial credentials for
  ad...@idm.lab.bos.redhat.com
  [21059] 1351684052.665269: Sending request (200 bytes) to 
  IDM.LAB.BOS.REDHAT.COM
  [21059] 1351684052.665989: Resolving hostname vm-044.idm.lab.bos.redhat.com
  [21059] 1351684052.667511: Sending initial UDP request to dgram 
  10.16.78.44:88
  [21059] 1351684052.672514: Received answer from dgram 10.16.78.44:88
  [21059] 1351684052.672653: Response was from master KDC
  [21059] 

Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-11-07 Thread Martin Kosek
On 11/07/2012 12:53 AM, Jakub Hrozek wrote:
 On Wed, Oct 31, 2012 at 01:22:52PM +0100, Martin Kosek wrote:
 On 10/31/2012 11:00 AM, Jakub Hrozek wrote:
 On Mon, Oct 22, 2012 at 02:14:00PM -0400, Simo Sorce wrote:
 On Mon, 2012-10-22 at 17:15 +0200, Martin Kosek wrote:
 On 10/08/2012 08:27 PM, Rob Crittenden wrote:
 Jakub Hrozek wrote:
 On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:


 - Original Message -
 Hi,

 the attached patches add the directory the SSSD writes domain-realm
 mappings as includedir to krb5.conf when installing the client.

 [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for
 options
 ipachangeconf only allows one delimeter between keys and values. This
 patch adds the possibility of also specifying delim in the option
 dictionary to override the default delimeter.

 On a slightly-unrelated note, we really should think about adopting
 Augeas. Changing configuration with home-grown scripts is getting
 tricky.

 [PATCH 2/3] Specify includedir in krb5.conf on new installs
 This patch utilizes the new functionality from the previous patch to
 add
 the includedir on top of the krb5.conf file

 [PATCH 3/3] Add the includedir to krb5.conf on upgrades
 This patch is completely untested and I'm only posting it to get
 opinions. At first I was going to use an upgrade script in %post but
 then I thought it would be overengineering when all we want to do is
 prepend one line.. Would a simple munging like this be acceptable or
 shall I write a full script?

 NACK, using a scriptlet is fine, but not the way you did, as it has a 
 huge
 race condition where krb5.conf exists and has only one line in it (the
 include line).

 You should first create the new file: echo include ...  
 /etc/krb.conf.ipanew
 Then cat the contents of the existing file in i:t cat /etc/krb.conf 
 /etc/krb.conf.ipanew
 And finally atomically rename it: mv /etc/krb.conf.ipanew /etc/krb.conf

 This method is also safe wrt something killing the yum process ...

 Simo.

 I'm attaching a new revision of the patches not even two months after
 the original nack.

 I also think it might be nice to have a more general way of upgrading
 the client config so I filed
 https://fedorahosted.org/freeipa/ticket/3149

 I don't think grepping for a string is an effective way to determine if 
 the
 client has been configured. Someone could have removed that line.

 I'd prefer using /var/lib/ipa-client/sysrestore/sysrestore.index. If it 
 exists
 and has more than 2 lines in it ([files] + one other file) then it is 
 safe to
 say the client is configured, or at least partially configured.

 rob


 I just found one more issue. What if ipa-client-install is run with 
 --no-sssd
 option? In that case I assume we should not include the SSSD's 
 krb5.include.d,
 right? The same would also appy for upgrades, we would need to check if 
 client
 was actually configured with SSSD before mangling their krb5.conf.

 Yeah that's right, we should have all sssd related changes under a
 conditional that is true only when sssd is enabled.

 Simo.

 OK, new patches are attached. In new installs, the includedir is only
 added when options.sssd is true. During upgrades, I checked for
 sssd.conf's existence, I'm not sure if there's any other way to check if
 the client was configured with sssd?

 Hello Jakub, thanks for these patches. I think that checking if 
 /etc/sssd.conf
 exists as actually not so bad way to test if it was configured. Anyway, I did
 few tests on server and client but I still see few issues:

 1) SELinux context of krb5.conf is not as expected (but I am not sure what 
 real
 issue could that cause):

 # restorecon -FvvR /etc/krb5.conf
 restorecon reset /etc/krb5.conf context
 unconfined_u:object_r:etc_t:s0-system_u:object_r:krb5_conf_t:s0

 
 Fixed. Thanks, I shouldn have noticed that doing mv would just replace
 the original context.
 
 2) I can no longer kinit on IPA server after applying your patch
 # rpm -q sssd
 sssd-1.9.90-0.20121030T1436Zgitf46bf56.fc17.x86_64
 # rpm -Uvh --force freeipa-*.rpm
 # head -n 5 /etc/krb5.conf
 includedir /var/lib/sss/pubconf/krb5.include.d/
 [logging]
  default = FILE:/var/log/krb5libs.log
  kdc = FILE:/var/log/krb5kdc.log
  admin_server = FILE:/var/log/kadmind.log
 # KRB5_TRACE=/dev/stdout kinit admin
 [21059] 1351684052.658548: Getting initial credentials for
 ad...@idm.lab.bos.redhat.com
 [21059] 1351684052.665269: Sending request (200 bytes) to 
 IDM.LAB.BOS.REDHAT.COM
 [21059] 1351684052.665989: Resolving hostname vm-044.idm.lab.bos.redhat.com
 [21059] 1351684052.667511: Sending initial UDP request to dgram 
 10.16.78.44:88
 [21059] 1351684052.672514: Received answer from dgram 10.16.78.44:88
 [21059] 1351684052.672653: Response was from master KDC
 [21059] 1351684052.672751: Received error from KDC: -1765328370/KDC has no
 support for encryption type
 kinit: KDC has no support for encryption type while getting initial 
 credentials


 Now when I comment includedir:

Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-11-06 Thread Jakub Hrozek
On Wed, Oct 31, 2012 at 01:22:52PM +0100, Martin Kosek wrote:
 On 10/31/2012 11:00 AM, Jakub Hrozek wrote:
  On Mon, Oct 22, 2012 at 02:14:00PM -0400, Simo Sorce wrote:
  On Mon, 2012-10-22 at 17:15 +0200, Martin Kosek wrote:
  On 10/08/2012 08:27 PM, Rob Crittenden wrote:
  Jakub Hrozek wrote:
  On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:
 
 
  - Original Message -
  Hi,
 
  the attached patches add the directory the SSSD writes domain-realm
  mappings as includedir to krb5.conf when installing the client.
 
  [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for
  options
  ipachangeconf only allows one delimeter between keys and values. This
  patch adds the possibility of also specifying delim in the option
  dictionary to override the default delimeter.
 
  On a slightly-unrelated note, we really should think about adopting
  Augeas. Changing configuration with home-grown scripts is getting
  tricky.
 
  [PATCH 2/3] Specify includedir in krb5.conf on new installs
  This patch utilizes the new functionality from the previous patch to
  add
  the includedir on top of the krb5.conf file
 
  [PATCH 3/3] Add the includedir to krb5.conf on upgrades
  This patch is completely untested and I'm only posting it to get
  opinions. At first I was going to use an upgrade script in %post but
  then I thought it would be overengineering when all we want to do is
  prepend one line.. Would a simple munging like this be acceptable or
  shall I write a full script?
 
  NACK, using a scriptlet is fine, but not the way you did, as it has a 
  huge
  race condition where krb5.conf exists and has only one line in it (the
  include line).
 
  You should first create the new file: echo include ...  
  /etc/krb.conf.ipanew
  Then cat the contents of the existing file in i:t cat /etc/krb.conf 
  /etc/krb.conf.ipanew
  And finally atomically rename it: mv /etc/krb.conf.ipanew /etc/krb.conf
 
  This method is also safe wrt something killing the yum process ...
 
  Simo.
 
  I'm attaching a new revision of the patches not even two months after
  the original nack.
 
  I also think it might be nice to have a more general way of upgrading
  the client config so I filed
  https://fedorahosted.org/freeipa/ticket/3149
 
  I don't think grepping for a string is an effective way to determine if 
  the
  client has been configured. Someone could have removed that line.
 
  I'd prefer using /var/lib/ipa-client/sysrestore/sysrestore.index. If it 
  exists
  and has more than 2 lines in it ([files] + one other file) then it is 
  safe to
  say the client is configured, or at least partially configured.
 
  rob
 
 
  I just found one more issue. What if ipa-client-install is run with 
  --no-sssd
  option? In that case I assume we should not include the SSSD's 
  krb5.include.d,
  right? The same would also appy for upgrades, we would need to check if 
  client
  was actually configured with SSSD before mangling their krb5.conf.
 
  Yeah that's right, we should have all sssd related changes under a
  conditional that is true only when sssd is enabled.
 
  Simo.
  
  OK, new patches are attached. In new installs, the includedir is only
  added when options.sssd is true. During upgrades, I checked for
  sssd.conf's existence, I'm not sure if there's any other way to check if
  the client was configured with sssd?
 
 Hello Jakub, thanks for these patches. I think that checking if /etc/sssd.conf
 exists as actually not so bad way to test if it was configured. Anyway, I did
 few tests on server and client but I still see few issues:
 
 1) SELinux context of krb5.conf is not as expected (but I am not sure what 
 real
 issue could that cause):
 
 # restorecon -FvvR /etc/krb5.conf
 restorecon reset /etc/krb5.conf context
 unconfined_u:object_r:etc_t:s0-system_u:object_r:krb5_conf_t:s0
 

Fixed. Thanks, I shouldn have noticed that doing mv would just replace
the original context.

 2) I can no longer kinit on IPA server after applying your patch
 # rpm -q sssd
 sssd-1.9.90-0.20121030T1436Zgitf46bf56.fc17.x86_64
 # rpm -Uvh --force freeipa-*.rpm
 # head -n 5 /etc/krb5.conf
 includedir /var/lib/sss/pubconf/krb5.include.d/
 [logging]
  default = FILE:/var/log/krb5libs.log
  kdc = FILE:/var/log/krb5kdc.log
  admin_server = FILE:/var/log/kadmind.log
 # KRB5_TRACE=/dev/stdout kinit admin
 [21059] 1351684052.658548: Getting initial credentials for
 ad...@idm.lab.bos.redhat.com
 [21059] 1351684052.665269: Sending request (200 bytes) to 
 IDM.LAB.BOS.REDHAT.COM
 [21059] 1351684052.665989: Resolving hostname vm-044.idm.lab.bos.redhat.com
 [21059] 1351684052.667511: Sending initial UDP request to dgram 10.16.78.44:88
 [21059] 1351684052.672514: Received answer from dgram 10.16.78.44:88
 [21059] 1351684052.672653: Response was from master KDC
 [21059] 1351684052.672751: Received error from KDC: -1765328370/KDC has no
 support for encryption type
 kinit: KDC has no support for encryption type while getting initial 
 

Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-11-05 Thread Jakub Hrozek
On Tue, Nov 06, 2012 at 08:47:04AM +0100, Martin Kosek wrote:
  Once updated we get a slew of AVCs:
 
 Yeah, we will need to have those fixed before inclusion in IPA... Did you file
 any bugzilla or should I do it?

Yes:
https://bugzilla.redhat.com/show_bug.cgi?id=873429

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


Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-10-31 Thread Jakub Hrozek
On Mon, Oct 22, 2012 at 02:14:00PM -0400, Simo Sorce wrote:
 On Mon, 2012-10-22 at 17:15 +0200, Martin Kosek wrote:
  On 10/08/2012 08:27 PM, Rob Crittenden wrote:
   Jakub Hrozek wrote:
   On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:
  
  
   - Original Message -
   Hi,
  
   the attached patches add the directory the SSSD writes domain-realm
   mappings as includedir to krb5.conf when installing the client.
  
   [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for
   options
   ipachangeconf only allows one delimeter between keys and values. This
   patch adds the possibility of also specifying delim in the option
   dictionary to override the default delimeter.
  
   On a slightly-unrelated note, we really should think about adopting
   Augeas. Changing configuration with home-grown scripts is getting
   tricky.
  
   [PATCH 2/3] Specify includedir in krb5.conf on new installs
   This patch utilizes the new functionality from the previous patch to
   add
   the includedir on top of the krb5.conf file
  
   [PATCH 3/3] Add the includedir to krb5.conf on upgrades
   This patch is completely untested and I'm only posting it to get
   opinions. At first I was going to use an upgrade script in %post but
   then I thought it would be overengineering when all we want to do is
   prepend one line.. Would a simple munging like this be acceptable or
   shall I write a full script?
  
   NACK, using a scriptlet is fine, but not the way you did, as it has a 
   huge
   race condition where krb5.conf exists and has only one line in it (the
   include line).
  
   You should first create the new file: echo include ...  
   /etc/krb.conf.ipanew
   Then cat the contents of the existing file in i:t cat /etc/krb.conf 
   /etc/krb.conf.ipanew
   And finally atomically rename it: mv /etc/krb.conf.ipanew /etc/krb.conf
  
   This method is also safe wrt something killing the yum process ...
  
   Simo.
  
   I'm attaching a new revision of the patches not even two months after
   the original nack.
  
   I also think it might be nice to have a more general way of upgrading
   the client config so I filed
   https://fedorahosted.org/freeipa/ticket/3149
   
   I don't think grepping for a string is an effective way to determine if 
   the
   client has been configured. Someone could have removed that line.
   
   I'd prefer using /var/lib/ipa-client/sysrestore/sysrestore.index. If it 
   exists
   and has more than 2 lines in it ([files] + one other file) then it is 
   safe to
   say the client is configured, or at least partially configured.
   
   rob
   
  
  I just found one more issue. What if ipa-client-install is run with 
  --no-sssd
  option? In that case I assume we should not include the SSSD's 
  krb5.include.d,
  right? The same would also appy for upgrades, we would need to check if 
  client
  was actually configured with SSSD before mangling their krb5.conf.
 
 Yeah that's right, we should have all sssd related changes under a
 conditional that is true only when sssd is enabled.
 
 Simo.

OK, new patches are attached. In new installs, the includedir is only
added when options.sssd is true. During upgrades, I checked for
sssd.conf's existence, I'm not sure if there's any other way to check if
the client was configured with sssd?
From 184e44e5209e2cdbe03446e3e00754bfc5b115df Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Fri, 17 Aug 2012 11:19:03 +0200
Subject: [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter
 for options

---
 ipa-client/ipaclient/ipachangeconf.py | 35 +++
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/ipa-client/ipaclient/ipachangeconf.py 
b/ipa-client/ipaclient/ipachangeconf.py
index 
6cf47b807957c245fe03ff4259e35526c49175a9..bdc5579fccd82193e837b5e86cbc694c2619317c
 100644
--- a/ipa-client/ipaclient/ipachangeconf.py
+++ b/ipa-client/ipaclient/ipachangeconf.py
@@ -174,9 +174,12 @@ class IPAChangeConf:
   self.subsectdel[1]))
 continue
 if o['type'] == option:
+delim = o.get('delim', self.dassign)
+if delim not in self.assign:
+raise ValueError('Unknown delim %s must be one of %s' 
% (delim,  .join([d for d in self.assign])))
 output.append(self._dump_line(self.indent[level],
   o['name'],
-  self.dassign,
+  delim,
   o['value']))
 continue
 if o['type'] == comment:
@@ -200,13 +203,21 @@ class IPAChangeConf:
 'type': 'comment',
 'value': value.rstrip()}  # pylint: disable=E1103
 
+o = dict()
 parts = line.split(self.dassign, 1)
 if len(parts)  2:
-

Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-10-31 Thread Martin Kosek
On 10/31/2012 11:00 AM, Jakub Hrozek wrote:
 On Mon, Oct 22, 2012 at 02:14:00PM -0400, Simo Sorce wrote:
 On Mon, 2012-10-22 at 17:15 +0200, Martin Kosek wrote:
 On 10/08/2012 08:27 PM, Rob Crittenden wrote:
 Jakub Hrozek wrote:
 On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:


 - Original Message -
 Hi,

 the attached patches add the directory the SSSD writes domain-realm
 mappings as includedir to krb5.conf when installing the client.

 [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for
 options
 ipachangeconf only allows one delimeter between keys and values. This
 patch adds the possibility of also specifying delim in the option
 dictionary to override the default delimeter.

 On a slightly-unrelated note, we really should think about adopting
 Augeas. Changing configuration with home-grown scripts is getting
 tricky.

 [PATCH 2/3] Specify includedir in krb5.conf on new installs
 This patch utilizes the new functionality from the previous patch to
 add
 the includedir on top of the krb5.conf file

 [PATCH 3/3] Add the includedir to krb5.conf on upgrades
 This patch is completely untested and I'm only posting it to get
 opinions. At first I was going to use an upgrade script in %post but
 then I thought it would be overengineering when all we want to do is
 prepend one line.. Would a simple munging like this be acceptable or
 shall I write a full script?

 NACK, using a scriptlet is fine, but not the way you did, as it has a 
 huge
 race condition where krb5.conf exists and has only one line in it (the
 include line).

 You should first create the new file: echo include ...  
 /etc/krb.conf.ipanew
 Then cat the contents of the existing file in i:t cat /etc/krb.conf 
 /etc/krb.conf.ipanew
 And finally atomically rename it: mv /etc/krb.conf.ipanew /etc/krb.conf

 This method is also safe wrt something killing the yum process ...

 Simo.

 I'm attaching a new revision of the patches not even two months after
 the original nack.

 I also think it might be nice to have a more general way of upgrading
 the client config so I filed
 https://fedorahosted.org/freeipa/ticket/3149

 I don't think grepping for a string is an effective way to determine if the
 client has been configured. Someone could have removed that line.

 I'd prefer using /var/lib/ipa-client/sysrestore/sysrestore.index. If it 
 exists
 and has more than 2 lines in it ([files] + one other file) then it is safe 
 to
 say the client is configured, or at least partially configured.

 rob


 I just found one more issue. What if ipa-client-install is run with 
 --no-sssd
 option? In that case I assume we should not include the SSSD's 
 krb5.include.d,
 right? The same would also appy for upgrades, we would need to check if 
 client
 was actually configured with SSSD before mangling their krb5.conf.

 Yeah that's right, we should have all sssd related changes under a
 conditional that is true only when sssd is enabled.

 Simo.
 
 OK, new patches are attached. In new installs, the includedir is only
 added when options.sssd is true. During upgrades, I checked for
 sssd.conf's existence, I'm not sure if there's any other way to check if
 the client was configured with sssd?

Hello Jakub, thanks for these patches. I think that checking if /etc/sssd.conf
exists as actually not so bad way to test if it was configured. Anyway, I did
few tests on server and client but I still see few issues:

1) SELinux context of krb5.conf is not as expected (but I am not sure what real
issue could that cause):

# restorecon -FvvR /etc/krb5.conf
restorecon reset /etc/krb5.conf context
unconfined_u:object_r:etc_t:s0-system_u:object_r:krb5_conf_t:s0

2) I can no longer kinit on IPA server after applying your patch
# rpm -q sssd
sssd-1.9.90-0.20121030T1436Zgitf46bf56.fc17.x86_64
# rpm -Uvh --force freeipa-*.rpm
# head -n 5 /etc/krb5.conf
includedir /var/lib/sss/pubconf/krb5.include.d/
[logging]
 default = FILE:/var/log/krb5libs.log
 kdc = FILE:/var/log/krb5kdc.log
 admin_server = FILE:/var/log/kadmind.log
# KRB5_TRACE=/dev/stdout kinit admin
[21059] 1351684052.658548: Getting initial credentials for
ad...@idm.lab.bos.redhat.com
[21059] 1351684052.665269: Sending request (200 bytes) to IDM.LAB.BOS.REDHAT.COM
[21059] 1351684052.665989: Resolving hostname vm-044.idm.lab.bos.redhat.com
[21059] 1351684052.667511: Sending initial UDP request to dgram 10.16.78.44:88
[21059] 1351684052.672514: Received answer from dgram 10.16.78.44:88
[21059] 1351684052.672653: Response was from master KDC
[21059] 1351684052.672751: Received error from KDC: -1765328370/KDC has no
support for encryption type
kinit: KDC has no support for encryption type while getting initial credentials


Now when I comment includedir:
# head -n 5 /etc/krb5.conf
# kinit admin
Password for ad...@idm.lab.bos.redhat.com:
# echo $?
0

When I upgraded client machine (without krb5kdc), kinit worked fine. Does that
mean that krb5.conf can only be changed on client machines?

3) We 

Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-10-22 Thread Martin Kosek
On 10/08/2012 08:27 PM, Rob Crittenden wrote:
 Jakub Hrozek wrote:
 On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:


 - Original Message -
 Hi,

 the attached patches add the directory the SSSD writes domain-realm
 mappings as includedir to krb5.conf when installing the client.

 [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for
 options
 ipachangeconf only allows one delimeter between keys and values. This
 patch adds the possibility of also specifying delim in the option
 dictionary to override the default delimeter.

 On a slightly-unrelated note, we really should think about adopting
 Augeas. Changing configuration with home-grown scripts is getting
 tricky.

 [PATCH 2/3] Specify includedir in krb5.conf on new installs
 This patch utilizes the new functionality from the previous patch to
 add
 the includedir on top of the krb5.conf file

 [PATCH 3/3] Add the includedir to krb5.conf on upgrades
 This patch is completely untested and I'm only posting it to get
 opinions. At first I was going to use an upgrade script in %post but
 then I thought it would be overengineering when all we want to do is
 prepend one line.. Would a simple munging like this be acceptable or
 shall I write a full script?

 NACK, using a scriptlet is fine, but not the way you did, as it has a huge
 race condition where krb5.conf exists and has only one line in it (the
 include line).

 You should first create the new file: echo include ...  
 /etc/krb.conf.ipanew
 Then cat the contents of the existing file in i:t cat /etc/krb.conf 
 /etc/krb.conf.ipanew
 And finally atomically rename it: mv /etc/krb.conf.ipanew /etc/krb.conf

 This method is also safe wrt something killing the yum process ...

 Simo.

 I'm attaching a new revision of the patches not even two months after
 the original nack.

 I also think it might be nice to have a more general way of upgrading
 the client config so I filed
 https://fedorahosted.org/freeipa/ticket/3149
 
 I don't think grepping for a string is an effective way to determine if the
 client has been configured. Someone could have removed that line.
 
 I'd prefer using /var/lib/ipa-client/sysrestore/sysrestore.index. If it exists
 and has more than 2 lines in it ([files] + one other file) then it is safe to
 say the client is configured, or at least partially configured.
 
 rob
 

I just found one more issue. What if ipa-client-install is run with --no-sssd
option? In that case I assume we should not include the SSSD's krb5.include.d,
right? The same would also appy for upgrades, we would need to check if client
was actually configured with SSSD before mangling their krb5.conf.

Martin

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


Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-10-22 Thread Simo Sorce
On Mon, 2012-10-22 at 17:15 +0200, Martin Kosek wrote:
 On 10/08/2012 08:27 PM, Rob Crittenden wrote:
  Jakub Hrozek wrote:
  On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:
 
 
  - Original Message -
  Hi,
 
  the attached patches add the directory the SSSD writes domain-realm
  mappings as includedir to krb5.conf when installing the client.
 
  [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for
  options
  ipachangeconf only allows one delimeter between keys and values. This
  patch adds the possibility of also specifying delim in the option
  dictionary to override the default delimeter.
 
  On a slightly-unrelated note, we really should think about adopting
  Augeas. Changing configuration with home-grown scripts is getting
  tricky.
 
  [PATCH 2/3] Specify includedir in krb5.conf on new installs
  This patch utilizes the new functionality from the previous patch to
  add
  the includedir on top of the krb5.conf file
 
  [PATCH 3/3] Add the includedir to krb5.conf on upgrades
  This patch is completely untested and I'm only posting it to get
  opinions. At first I was going to use an upgrade script in %post but
  then I thought it would be overengineering when all we want to do is
  prepend one line.. Would a simple munging like this be acceptable or
  shall I write a full script?
 
  NACK, using a scriptlet is fine, but not the way you did, as it has a huge
  race condition where krb5.conf exists and has only one line in it (the
  include line).
 
  You should first create the new file: echo include ...  
  /etc/krb.conf.ipanew
  Then cat the contents of the existing file in i:t cat /etc/krb.conf 
  /etc/krb.conf.ipanew
  And finally atomically rename it: mv /etc/krb.conf.ipanew /etc/krb.conf
 
  This method is also safe wrt something killing the yum process ...
 
  Simo.
 
  I'm attaching a new revision of the patches not even two months after
  the original nack.
 
  I also think it might be nice to have a more general way of upgrading
  the client config so I filed
  https://fedorahosted.org/freeipa/ticket/3149
  
  I don't think grepping for a string is an effective way to determine if the
  client has been configured. Someone could have removed that line.
  
  I'd prefer using /var/lib/ipa-client/sysrestore/sysrestore.index. If it 
  exists
  and has more than 2 lines in it ([files] + one other file) then it is safe 
  to
  say the client is configured, or at least partially configured.
  
  rob
  
 
 I just found one more issue. What if ipa-client-install is run with --no-sssd
 option? In that case I assume we should not include the SSSD's krb5.include.d,
 right? The same would also appy for upgrades, we would need to check if client
 was actually configured with SSSD before mangling their krb5.conf.

Yeah that's right, we should have all sssd related changes under a
conditional that is true only when sssd is enabled.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

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


Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-10-08 Thread Jakub Hrozek
On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:
 
 
 - Original Message -
  Hi,
  
  the attached patches add the directory the SSSD writes domain-realm
  mappings as includedir to krb5.conf when installing the client.
  
  [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for
  options
  ipachangeconf only allows one delimeter between keys and values. This
  patch adds the possibility of also specifying delim in the option
  dictionary to override the default delimeter.
  
  On a slightly-unrelated note, we really should think about adopting
  Augeas. Changing configuration with home-grown scripts is getting
  tricky.
  
  [PATCH 2/3] Specify includedir in krb5.conf on new installs
  This patch utilizes the new functionality from the previous patch to
  add
  the includedir on top of the krb5.conf file
  
  [PATCH 3/3] Add the includedir to krb5.conf on upgrades
  This patch is completely untested and I'm only posting it to get
  opinions. At first I was going to use an upgrade script in %post but
  then I thought it would be overengineering when all we want to do is
  prepend one line.. Would a simple munging like this be acceptable or
  shall I write a full script?
 
 NACK, using a scriptlet is fine, but not the way you did, as it has a huge 
 race condition where krb5.conf exists and has only one line in it (the 
 include line).
 
 You should first create the new file: echo include ...  
 /etc/krb.conf.ipanew
 Then cat the contents of the existing file in i:t cat /etc/krb.conf  
 /etc/krb.conf.ipanew
 And finally atomically rename it: mv /etc/krb.conf.ipanew /etc/krb.conf
 
 This method is also safe wrt something killing the yum process ...
 
 Simo.

I'm attaching a new revision of the patches not even two months after
the original nack.

I also think it might be nice to have a more general way of upgrading
the client config so I filed
https://fedorahosted.org/freeipa/ticket/3149
From f93e181d4812dd66eda7cbc2cd9fc8ccc603e0c5 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Fri, 17 Aug 2012 11:19:03 +0200
Subject: [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter
 for options

---
 ipa-client/ipaclient/ipachangeconf.py | 37 +++
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/ipa-client/ipaclient/ipachangeconf.py 
b/ipa-client/ipaclient/ipachangeconf.py
index 
6cf47b807957c245fe03ff4259e35526c49175a9..087e096920854bdd822d83182102723a2082af2a
 100644
--- a/ipa-client/ipaclient/ipachangeconf.py
+++ b/ipa-client/ipaclient/ipachangeconf.py
@@ -174,9 +174,13 @@ class IPAChangeConf:
   self.subsectdel[1]))
 continue
 if o['type'] == option:
+delim = o.get('delim', self.dassign)
+if delim not in self.assign:
+raise ValueError('Unknown delim %s must be one of %s' 
% (delim,  .join([d for d in self.assign])))
+output += 
self.indent[level]+o['name']+delim+o['value']+self.deol
 output.append(self._dump_line(self.indent[level],
   o['name'],
-  self.dassign,
+  delim,
   o['value']))
 continue
 if o['type'] == comment:
@@ -200,13 +204,21 @@ class IPAChangeConf:
 'type': 'comment',
 'value': value.rstrip()}  # pylint: disable=E1103
 
+o = dict()
 parts = line.split(self.dassign, 1)
 if len(parts)  2:
-raise SyntaxError('Syntax Error: Unknown line format')
+# The default assign didn't match, try the non-default
+for d in self.assign[1:]:
+parts = line.split(d, 1)
+if len(parts) = 2:
+o['delim'] = d
+break
 
-return {'name': parts[0].strip(),
-'type': 'option',
-'value': parts[1].rstrip()}
+if 'delim' not in o:
+raise SyntaxError, 'Syntax Error: Unknown line format'
+
+o.update({'name':parts[0].strip(), 'type':'option', 
'value':parts[1].rstrip()})
+return o
 
 def findOpts(self, opts, type, name, exclude_sections=False):
 
@@ -256,13 +268,14 @@ class IPAChangeConf:
  'value': val})
 continue
 if o['type'] == 'option':
-val = self._dump_line(self.indent[level],
-  o['name'],
-  self.dassign,
-  o['value'])
-opts.append({'name': 'comment',
- 'type': 'comment',
- 'value': val})
+delim = o.get('delim', self.dassign)
+  

Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-10-08 Thread Simo Sorce
On Mon, 2012-10-08 at 18:17 +0200, Jakub Hrozek wrote:
 On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:
  
  
  - Original Message -
   Hi,
   
   the attached patches add the directory the SSSD writes domain-realm
   mappings as includedir to krb5.conf when installing the client.
   
   [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for
   options
   ipachangeconf only allows one delimeter between keys and values. This
   patch adds the possibility of also specifying delim in the option
   dictionary to override the default delimeter.
   
   On a slightly-unrelated note, we really should think about adopting
   Augeas. Changing configuration with home-grown scripts is getting
   tricky.
   
   [PATCH 2/3] Specify includedir in krb5.conf on new installs
   This patch utilizes the new functionality from the previous patch to
   add
   the includedir on top of the krb5.conf file
   
   [PATCH 3/3] Add the includedir to krb5.conf on upgrades
   This patch is completely untested and I'm only posting it to get
   opinions. At first I was going to use an upgrade script in %post but
   then I thought it would be overengineering when all we want to do is
   prepend one line.. Would a simple munging like this be acceptable or
   shall I write a full script?
  
  NACK, using a scriptlet is fine, but not the way you did, as it has a huge 
  race condition where krb5.conf exists and has only one line in it (the 
  include line).
  
  You should first create the new file: echo include ...  
  /etc/krb.conf.ipanew
  Then cat the contents of the existing file in i:t cat /etc/krb.conf  
  /etc/krb.conf.ipanew
  And finally atomically rename it: mv /etc/krb.conf.ipanew /etc/krb.conf
  
  This method is also safe wrt something killing the yum process ...
  
  Simo.
 
 I'm attaching a new revision of the patches not even two months after
 the original nack.
 
 I also think it might be nice to have a more general way of upgrading
 the client config so I filed
 https://fedorahosted.org/freeipa/ticket/3149

ACK

Simo.


-- 
Simo Sorce * Red Hat, Inc * New York

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


Re: [Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-10-08 Thread Rob Crittenden

Jakub Hrozek wrote:

On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:



- Original Message -

Hi,

the attached patches add the directory the SSSD writes domain-realm
mappings as includedir to krb5.conf when installing the client.

[PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for
options
ipachangeconf only allows one delimeter between keys and values. This
patch adds the possibility of also specifying delim in the option
dictionary to override the default delimeter.

On a slightly-unrelated note, we really should think about adopting
Augeas. Changing configuration with home-grown scripts is getting
tricky.

[PATCH 2/3] Specify includedir in krb5.conf on new installs
This patch utilizes the new functionality from the previous patch to
add
the includedir on top of the krb5.conf file

[PATCH 3/3] Add the includedir to krb5.conf on upgrades
This patch is completely untested and I'm only posting it to get
opinions. At first I was going to use an upgrade script in %post but
then I thought it would be overengineering when all we want to do is
prepend one line.. Would a simple munging like this be acceptable or
shall I write a full script?


NACK, using a scriptlet is fine, but not the way you did, as it has a huge race 
condition where krb5.conf exists and has only one line in it (the include line).

You should first create the new file: echo include ...  /etc/krb.conf.ipanew
Then cat the contents of the existing file in i:t cat /etc/krb.conf  
/etc/krb.conf.ipanew
And finally atomically rename it: mv /etc/krb.conf.ipanew /etc/krb.conf

This method is also safe wrt something killing the yum process ...

Simo.


I'm attaching a new revision of the patches not even two months after
the original nack.

I also think it might be nice to have a more general way of upgrading
the client config so I filed
https://fedorahosted.org/freeipa/ticket/3149


I don't think grepping for a string is an effective way to determine if 
the client has been configured. Someone could have removed that line.


I'd prefer using /var/lib/ipa-client/sysrestore/sysrestore.index. If it 
exists and has more than 2 lines in it ([files] + one other file) then 
it is safe to say the client is configured, or at least partially 
configured.


rob

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


[Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

2012-08-17 Thread Jakub Hrozek
Hi,

the attached patches add the directory the SSSD writes domain-realm
mappings as includedir to krb5.conf when installing the client.

[PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for options
ipachangeconf only allows one delimeter between keys and values. This
patch adds the possibility of also specifying delim in the option
dictionary to override the default delimeter.

On a slightly-unrelated note, we really should think about adopting
Augeas. Changing configuration with home-grown scripts is getting
tricky.

[PATCH 2/3] Specify includedir in krb5.conf on new installs
This patch utilizes the new functionality from the previous patch to add
the includedir on top of the krb5.conf file

[PATCH 3/3] Add the includedir to krb5.conf on upgrades
This patch is completely untested and I'm only posting it to get
opinions. At first I was going to use an upgrade script in %post but
then I thought it would be overengineering when all we want to do is
prepend one line.. Would a simple munging like this be acceptable or
shall I write a full script?
From 507dad241486258348153bedb06011e0f884c88f Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Fri, 17 Aug 2012 11:19:03 +0200
Subject: [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter
 for options

---
 ipa-client/ipaclient/ipachangeconf.py | 23 +++
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/ipa-client/ipaclient/ipachangeconf.py 
b/ipa-client/ipaclient/ipachangeconf.py
index 
f6288062be5c5d1b29341ed90814e1fa1431019c..f5ec227fee6a46eb666a1bfe9a3349e40f80b7e9
 100644
--- a/ipa-client/ipaclient/ipachangeconf.py
+++ b/ipa-client/ipaclient/ipachangeconf.py
@@ -161,7 +161,10 @@ class IPAChangeConf:
 output += self.indent[level]+self.subsectdel[1]+self.deol
 continue
 if o['type'] == option:
-output += 
self.indent[level]+o['name']+self.dassign+o['value']+self.deol
+delim = o.get('delim', self.dassign)
+if delim not in self.assign:
+raise ValueError('Unknown delim %s must be one of %s' 
% (delim,  .join([d for d in self.assign])))
+output += 
self.indent[level]+o['name']+delim+o['value']+self.deol
 continue
 if o['type'] == comment:
 output += self.dcomment+o['value']+self.deol
@@ -182,11 +185,21 @@ class IPAChangeConf:
 if value:
 return {'name':'comment', 'type':'comment', 
'value':value.rstrip()} #pylint: disable=E1103
 
+o = dict()
 parts = line.split(self.dassign, 1)
 if len(parts)  2:
-raise SyntaxError, 'Syntax Error: Unknown line format'
+# The default assign didn't match, try the non-default
+for d in self.assign[1:]:
+parts = line.split(d, 1)
+if len(parts) = 2:
+o['delim'] = d
+break
 
-return {'name':parts[0].strip(), 'type':'option', 
'value':parts[1].rstrip()}
+if 'delim' not in o:
+raise SyntaxError, 'Syntax Error: Unknown line format'
+
+o.update({'name':parts[0].strip(), 'type':'option', 
'value':parts[1].rstrip()})
+return o
 
 def findOpts(self, opts, type, name, exclude_sections=False):
 
@@ -224,7 +237,9 @@ class IPAChangeConf:
 opts.append({'name':'comment', 'type':'comment', 'value':val})
 continue
 if o['type'] == 'option':
-val = self.indent[level]+o['name']+self.dassign+o['value']
+delim = o.get('delim', self.dassign)
+if delim not in self.assign:
+val = self.indent[level]+o['name']+delim+o['value']
 opts.append({'name':'comment', 'type':'comment', 'value':val})
 continue
 if o['type'] == 'comment':
-- 
1.7.11.2

From 74091875ec511b1b271f723eb17918544258a957 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Sun, 5 Aug 2012 20:47:12 +0200
Subject: [PATCH 2/3] Specify includedir in krb5.conf on new installs

---
 ipa-client/ipa-install/ipa-client-install | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ipa-client/ipa-install/ipa-client-install 
b/ipa-client/ipa-install/ipa-client-install
index 
2e65921e8de2dfe68443f5b5875954d71dd48ed2..a4c1b6fbdfefc26f9be8297b6e60ac4d0da42876
 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -642,7 +642,7 @@ def hardcode_ldap_server(cli_server):
 def configure_krb5_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server, 
cli_kdc, dnsok, options, filename, client_domain):
 
 krbconf = ipaclient.ipachangeconf.IPAChangeConf(IPA Installer)
-krbconf.setOptionAssignment( = )
+krbconf.setOptionAssignment(( = ,  ))
 krbconf.setSectionNameDelimiters(([,]))