Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Martin Kosek]

On 03/07/2013 06:32 PM, Sumit Bose wrote:

On Wed, Mar 06, 2013 at 05:33:43PM +0100, Sumit Bose wrote:

On Wed, Mar 06, 2013 at 08:51:47AM -0500, Simo Sorce wrote:

On Wed, 2013-03-06 at 14:49 +0100, Martin Kosek wrote:

On 03/06/2013 10:41 AM, Sumit Bose wrote:

On Tue, Mar 05, 2013 at 05:13:58PM +0100, Martin Kosek wrote:

On 03/04/2013 04:22 PM, Sumit Bose wrote:

On Fri, Mar 01, 2013 at 08:58:34AM -0500, Simo Sorce wrote:

On Fri, 2013-03-01 at 10:08 +0100, Martin Kosek wrote:

On 03/01/2013 09:20 AM, Sumit Bose wrote:

On Fri, Mar 01, 2013 at 08:33:51AM +0100, Martin Kosek wrote:

On 02/28/2013 03:28 PM, Simo Sorce wrote:

On Thu, 2013-02-28 at 13:02 +0100, Martin Kosek wrote:

On 02/28/2013 12:42 PM, Sumit Bose wrote:

On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:

On 02/27/2013 06:48 PM, Sumit Bose wrote:



Hi Sumit,

This looks like a good idea and would prevent the magic default PAC type, yes.
Though I would not add this service-specific setting to global IPA config 
object.

I would rather like to see that in the service tree, for example as a
configuration option of the service root which could be controlled with
serviceconfig-* commands (we already have dnsconfig, trustconfig), e.g:

# ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
# ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
# ipa serviceconfig-show
   Default PAC Map: nfs:NONE, cifs:PAD


Are you thinking of having this in addition to the for-all-services
default values in cn=ipaConfig,cn=etc or shall those be dropped? I don't
like the first case because then three different objects needs to be
consulted to find out which is the right type. This wouldn't be an issue
for the plugin, but I think it is hard for the user/admin to follow.


Hm, you are right.



If the current defaults shall be dropped I think this is a major change
because it will require changes in the current CLI and WebUI which will
be visible to the users. I'm not against this change, I'm just wondering
if it is worth the effort for the next release?

Maybe an argument to keep this is in global default is that the settings
are used for the host/*.* services as well which are in a different
sub-tree of the cn=accounts container. Additionally in future we might
want apply those setting to the user TGTs as well?


Yeah, that was actually my point. That we are mixing service-specific PAC
rules to the global setting. Which may be shared with host/*.* principals and
user principals. This automatic PAC rules may require some designing so that is
is generally usable.


I think putting everything in the general config is more understandable
and discoverable. These per-service defaults are basically exceptions to
the general rule so it make sense to keep everything together.

Simo.



Ok, if these are really just an exceptions to the general rule (and there will
not be too many of them), I think we can leave it in config entry. But if we
expect to have exceptions for other types of entries (hosts, users), I think we
should rather use something like service:nfs:NONE do distinguish this 
exception.

Question is, do we want to implement the interface and processing for that in
current Sumit's patches or do we use that is they are?


I would like to update the patches so that they can handle the
service:TYPE style entry and replace the current update code with just
adding nfs:NONE to the global options. I will update the design page
accordingly, too.


Ok. If the update procedure shrinks just to adding service:nfs:NONE then it'd
be great.


If we need to distinguish between service principals and user principals
I would prefer rather use a special keyword for upns

service: is redundant and I do not want here to be able to say
upn:martin:NONE because per principal options are available on the
principal object.

I actually really do not see the need for changing the default just for
user principals. If we are worried that one day we might want to really
have upn:NONE, then let's use nfs/:NONE, host/:NONE etc... so one day we
might add upn:NONE and the lack of / will tell us this is not a service
named upn/foo.bar.baz but rather it means user principal names.

However I do not see us ever really needing upn:NONE


I would prefer if the enhancements needed for the CLI and WebUI can be
covered by other/new tickets, but I'm happy to add the needed
information to the design page too.

bye,
Sumit


I am OK with adding the interface for this special exception later. In that
case, a ticket + note in the design as you mentioned would be enough.


Ack.

Simo.



Please find attached a new version of the patches. 0095 i(updating) is
renamed and much simpler now. I opened
https://fedorahosted.org/freeipa/ticket/3484 to added the needed change
for 'service:TYPE' to CLI and WebUI. For the time being I've added
patch 0108 which simply allows 'nfs:NONE' as a type to make sure that it
is not deleted accidentally when e.g. using the WebUI. If you do not
like it 

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Sumit Bose]

On Wed, Mar 06, 2013 at 05:33:43PM +0100, Sumit Bose wrote:
 On Wed, Mar 06, 2013 at 08:51:47AM -0500, Simo Sorce wrote:
  On Wed, 2013-03-06 at 14:49 +0100, Martin Kosek wrote:
   On 03/06/2013 10:41 AM, Sumit Bose wrote:
On Tue, Mar 05, 2013 at 05:13:58PM +0100, Martin Kosek wrote:
On 03/04/2013 04:22 PM, Sumit Bose wrote:
On Fri, Mar 01, 2013 at 08:58:34AM -0500, Simo Sorce wrote:
On Fri, 2013-03-01 at 10:08 +0100, Martin Kosek wrote:
On 03/01/2013 09:20 AM, Sumit Bose wrote:
On Fri, Mar 01, 2013 at 08:33:51AM +0100, Martin Kosek wrote:
On 02/28/2013 03:28 PM, Simo Sorce wrote:
On Thu, 2013-02-28 at 13:02 +0100, Martin Kosek wrote:
On 02/28/2013 12:42 PM, Sumit Bose wrote:
On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
On 02/27/2013 06:48 PM, Sumit Bose wrote:
   
Hi Sumit,
   
This looks like a good idea and would prevent the magic 
default PAC type, yes.
Though I would not add this service-specific setting to 
global IPA config object.
   
I would rather like to see that in the service tree, for 
example as a
configuration option of the service root which could be 
controlled with
serviceconfig-* commands (we already have dnsconfig, 
trustconfig), e.g:
   
# ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
# ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
# ipa serviceconfig-show
  Default PAC Map: nfs:NONE, cifs:PAD
   
Are you thinking of having this in addition to the 
for-all-services
default values in cn=ipaConfig,cn=etc or shall those be 
dropped? I don't
like the first case because then three different objects needs 
to be
consulted to find out which is the right type. This wouldn't 
be an issue
for the plugin, but I think it is hard for the user/admin to 
follow.
   
Hm, you are right.
   
   
If the current defaults shall be dropped I think this is a 
major change
because it will require changes in the current CLI and WebUI 
which will
be visible to the users. I'm not against this change, I'm just 
wondering
if it is worth the effort for the next release?
   
Maybe an argument to keep this is in global default is that 
the settings
are used for the host/*.* services as well which are in a 
different
sub-tree of the cn=accounts container. Additionally in future 
we might
want apply those setting to the user TGTs as well?
   
Yeah, that was actually my point. That we are mixing 
service-specific PAC
rules to the global setting. Which may be shared with 
host/*.* principals and
user principals. This automatic PAC rules may require some 
designing so that is
is generally usable.
   
I think putting everything in the general config is more 
understandable
and discoverable. These per-service defaults are basically 
exceptions to
the general rule so it make sense to keep everything together.
   
Simo.
   
   
Ok, if these are really just an exceptions to the general rule 
(and there will
not be too many of them), I think we can leave it in config 
entry. But if we
expect to have exceptions for other types of entries (hosts, 
users), I think we
should rather use something like service:nfs:NONE do 
distinguish this exception.
   
Question is, do we want to implement the interface and processing 
for that in
current Sumit's patches or do we use that is they are?
   
I would like to update the patches so that they can handle the
service:TYPE style entry and replace the current update code with 
just
adding nfs:NONE to the global options. I will update the design 
page
accordingly, too.
   
Ok. If the update procedure shrinks just to adding service:nfs:NONE 
then it'd
be great.
   
If we need to distinguish between service principals and user 
principals
I would prefer rather use a special keyword for upns
   
service: is redundant and I do not want here to be able to say
upn:martin:NONE because per principal options are available on the
principal object.
   
I actually really do not see the need for changing the default just 
for
user principals. If we are worried that one day we might want to 
really
have upn:NONE, then let's use nfs/:NONE, host/:NONE etc... so one 
day we
might add upn:NONE and the lack of / will tell us this is not a 
service
named upn/foo.bar.baz but rather it means user principal names.
   
However I do not see us ever really needing upn:NONE
   
I would prefer if the enhancements needed for the CLI and WebUI 
can be
covered by other/new tickets, but I'm happy to add the needed
information to the design page too.
   
bye,
Sumit
   
I am OK with adding the interface for this special exception later. 
In that
case, a 

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Sumit Bose]

On Tue, Mar 05, 2013 at 05:13:58PM +0100, Martin Kosek wrote:
 On 03/04/2013 04:22 PM, Sumit Bose wrote:
  On Fri, Mar 01, 2013 at 08:58:34AM -0500, Simo Sorce wrote:
  On Fri, 2013-03-01 at 10:08 +0100, Martin Kosek wrote:
  On 03/01/2013 09:20 AM, Sumit Bose wrote:
  On Fri, Mar 01, 2013 at 08:33:51AM +0100, Martin Kosek wrote:
  On 02/28/2013 03:28 PM, Simo Sorce wrote:
  On Thu, 2013-02-28 at 13:02 +0100, Martin Kosek wrote:
  On 02/28/2013 12:42 PM, Sumit Bose wrote:
  On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
  On 02/27/2013 06:48 PM, Sumit Bose wrote:
 
  Hi Sumit,
 
  This looks like a good idea and would prevent the magic default PAC 
  type, yes.
  Though I would not add this service-specific setting to global IPA 
  config object.
 
  I would rather like to see that in the service tree, for example as 
  a
  configuration option of the service root which could be controlled 
  with
  serviceconfig-* commands (we already have dnsconfig, trustconfig), 
  e.g:
 
  # ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
  # ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
  # ipa serviceconfig-show
Default PAC Map: nfs:NONE, cifs:PAD
 
  Are you thinking of having this in addition to the for-all-services
  default values in cn=ipaConfig,cn=etc or shall those be dropped? I 
  don't
  like the first case because then three different objects needs to be
  consulted to find out which is the right type. This wouldn't be an 
  issue
  for the plugin, but I think it is hard for the user/admin to follow.
 
  Hm, you are right.
 
 
  If the current defaults shall be dropped I think this is a major 
  change
  because it will require changes in the current CLI and WebUI which 
  will
  be visible to the users. I'm not against this change, I'm just 
  wondering
  if it is worth the effort for the next release?
 
  Maybe an argument to keep this is in global default is that the 
  settings
  are used for the host/*.* services as well which are in a different
  sub-tree of the cn=accounts container. Additionally in future we 
  might
  want apply those setting to the user TGTs as well?
 
  Yeah, that was actually my point. That we are mixing service-specific 
  PAC
  rules to the global setting. Which may be shared with host/*.* 
  principals and
  user principals. This automatic PAC rules may require some designing 
  so that is
  is generally usable.
 
  I think putting everything in the general config is more understandable
  and discoverable. These per-service defaults are basically exceptions 
  to
  the general rule so it make sense to keep everything together.
 
  Simo.
 
 
  Ok, if these are really just an exceptions to the general rule (and 
  there will
  not be too many of them), I think we can leave it in config entry. But 
  if we
  expect to have exceptions for other types of entries (hosts, users), I 
  think we
  should rather use something like service:nfs:NONE do distinguish this 
  exception.
 
  Question is, do we want to implement the interface and processing for 
  that in
  current Sumit's patches or do we use that is they are?
 
  I would like to update the patches so that they can handle the
  service:TYPE style entry and replace the current update code with just
  adding nfs:NONE to the global options. I will update the design page
  accordingly, too.
 
  Ok. If the update procedure shrinks just to adding service:nfs:NONE then 
  it'd
  be great.
 
  If we need to distinguish between service principals and user principals
  I would prefer rather use a special keyword for upns
 
  service: is redundant and I do not want here to be able to say
  upn:martin:NONE because per principal options are available on the
  principal object.
 
  I actually really do not see the need for changing the default just for
  user principals. If we are worried that one day we might want to really
  have upn:NONE, then let's use nfs/:NONE, host/:NONE etc... so one day we
  might add upn:NONE and the lack of / will tell us this is not a service
  named upn/foo.bar.baz but rather it means user principal names.
 
  However I do not see us ever really needing upn:NONE
 
  I would prefer if the enhancements needed for the CLI and WebUI can be
  covered by other/new tickets, but I'm happy to add the needed
  information to the design page too.
 
  bye,
  Sumit
 
  I am OK with adding the interface for this special exception later. In 
  that
  case, a ticket + note in the design as you mentioned would be enough.
 
  Ack.
 
  Simo.
 
  
  Please find attached a new version of the patches. 0095 i(updating) is
  renamed and much simpler now. I opened
  https://fedorahosted.org/freeipa/ticket/3484 to added the needed change
  for 'service:TYPE' to CLI and WebUI. For the time being I've added
  patch 0108 which simply allows 'nfs:NONE' as a type to make sure that it
  is not deleted accidentally when e.g. using the WebUI. If you do not
  like it it can simply be dropped, 

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Martin Kosek]

On 03/06/2013 10:41 AM, Sumit Bose wrote:
 On Tue, Mar 05, 2013 at 05:13:58PM +0100, Martin Kosek wrote:
 On 03/04/2013 04:22 PM, Sumit Bose wrote:
 On Fri, Mar 01, 2013 at 08:58:34AM -0500, Simo Sorce wrote:
 On Fri, 2013-03-01 at 10:08 +0100, Martin Kosek wrote:
 On 03/01/2013 09:20 AM, Sumit Bose wrote:
 On Fri, Mar 01, 2013 at 08:33:51AM +0100, Martin Kosek wrote:
 On 02/28/2013 03:28 PM, Simo Sorce wrote:
 On Thu, 2013-02-28 at 13:02 +0100, Martin Kosek wrote:
 On 02/28/2013 12:42 PM, Sumit Bose wrote:
 On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
 On 02/27/2013 06:48 PM, Sumit Bose wrote:

 Hi Sumit,

 This looks like a good idea and would prevent the magic default PAC 
 type, yes.
 Though I would not add this service-specific setting to global IPA 
 config object.

 I would rather like to see that in the service tree, for example as 
 a
 configuration option of the service root which could be controlled 
 with
 serviceconfig-* commands (we already have dnsconfig, trustconfig), 
 e.g:

 # ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
 # ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
 # ipa serviceconfig-show
   Default PAC Map: nfs:NONE, cifs:PAD

 Are you thinking of having this in addition to the for-all-services
 default values in cn=ipaConfig,cn=etc or shall those be dropped? I 
 don't
 like the first case because then three different objects needs to be
 consulted to find out which is the right type. This wouldn't be an 
 issue
 for the plugin, but I think it is hard for the user/admin to follow.

 Hm, you are right.


 If the current defaults shall be dropped I think this is a major 
 change
 because it will require changes in the current CLI and WebUI which 
 will
 be visible to the users. I'm not against this change, I'm just 
 wondering
 if it is worth the effort for the next release?

 Maybe an argument to keep this is in global default is that the 
 settings
 are used for the host/*.* services as well which are in a different
 sub-tree of the cn=accounts container. Additionally in future we 
 might
 want apply those setting to the user TGTs as well?

 Yeah, that was actually my point. That we are mixing service-specific 
 PAC
 rules to the global setting. Which may be shared with host/*.* 
 principals and
 user principals. This automatic PAC rules may require some designing 
 so that is
 is generally usable.

 I think putting everything in the general config is more understandable
 and discoverable. These per-service defaults are basically exceptions 
 to
 the general rule so it make sense to keep everything together.

 Simo.


 Ok, if these are really just an exceptions to the general rule (and 
 there will
 not be too many of them), I think we can leave it in config entry. But 
 if we
 expect to have exceptions for other types of entries (hosts, users), I 
 think we
 should rather use something like service:nfs:NONE do distinguish this 
 exception.

 Question is, do we want to implement the interface and processing for 
 that in
 current Sumit's patches or do we use that is they are?

 I would like to update the patches so that they can handle the
 service:TYPE style entry and replace the current update code with just
 adding nfs:NONE to the global options. I will update the design page
 accordingly, too.

 Ok. If the update procedure shrinks just to adding service:nfs:NONE then 
 it'd
 be great.

 If we need to distinguish between service principals and user principals
 I would prefer rather use a special keyword for upns

 service: is redundant and I do not want here to be able to say
 upn:martin:NONE because per principal options are available on the
 principal object.

 I actually really do not see the need for changing the default just for
 user principals. If we are worried that one day we might want to really
 have upn:NONE, then let's use nfs/:NONE, host/:NONE etc... so one day we
 might add upn:NONE and the lack of / will tell us this is not a service
 named upn/foo.bar.baz but rather it means user principal names.

 However I do not see us ever really needing upn:NONE

 I would prefer if the enhancements needed for the CLI and WebUI can be
 covered by other/new tickets, but I'm happy to add the needed
 information to the design page too.

 bye,
 Sumit

 I am OK with adding the interface for this special exception later. In 
 that
 case, a ticket + note in the design as you mentioned would be enough.

 Ack.

 Simo.


 Please find attached a new version of the patches. 0095 i(updating) is
 renamed and much simpler now. I opened
 https://fedorahosted.org/freeipa/ticket/3484 to added the needed change
 for 'service:TYPE' to CLI and WebUI. For the time being I've added
 patch 0108 which simply allows 'nfs:NONE' as a type to make sure that it
 is not deleted accidentally when e.g. using the WebUI. If you do not
 like it it can simply be dropped, everything is working fine without it.

 bye,
 Sumit


 Patch 0098:

 If this part does not match 

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Simo Sorce]

On Wed, 2013-03-06 at 14:49 +0100, Martin Kosek wrote:
 On 03/06/2013 10:41 AM, Sumit Bose wrote:
  On Tue, Mar 05, 2013 at 05:13:58PM +0100, Martin Kosek wrote:
  On 03/04/2013 04:22 PM, Sumit Bose wrote:
  On Fri, Mar 01, 2013 at 08:58:34AM -0500, Simo Sorce wrote:
  On Fri, 2013-03-01 at 10:08 +0100, Martin Kosek wrote:
  On 03/01/2013 09:20 AM, Sumit Bose wrote:
  On Fri, Mar 01, 2013 at 08:33:51AM +0100, Martin Kosek wrote:
  On 02/28/2013 03:28 PM, Simo Sorce wrote:
  On Thu, 2013-02-28 at 13:02 +0100, Martin Kosek wrote:
  On 02/28/2013 12:42 PM, Sumit Bose wrote:
  On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
  On 02/27/2013 06:48 PM, Sumit Bose wrote:
 
  Hi Sumit,
 
  This looks like a good idea and would prevent the magic default 
  PAC type, yes.
  Though I would not add this service-specific setting to global 
  IPA config object.
 
  I would rather like to see that in the service tree, for example 
  as a
  configuration option of the service root which could be 
  controlled with
  serviceconfig-* commands (we already have dnsconfig, 
  trustconfig), e.g:
 
  # ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
  # ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
  # ipa serviceconfig-show
Default PAC Map: nfs:NONE, cifs:PAD
 
  Are you thinking of having this in addition to the for-all-services
  default values in cn=ipaConfig,cn=etc or shall those be dropped? I 
  don't
  like the first case because then three different objects needs to 
  be
  consulted to find out which is the right type. This wouldn't be an 
  issue
  for the plugin, but I think it is hard for the user/admin to 
  follow.
 
  Hm, you are right.
 
 
  If the current defaults shall be dropped I think this is a major 
  change
  because it will require changes in the current CLI and WebUI which 
  will
  be visible to the users. I'm not against this change, I'm just 
  wondering
  if it is worth the effort for the next release?
 
  Maybe an argument to keep this is in global default is that the 
  settings
  are used for the host/*.* services as well which are in a different
  sub-tree of the cn=accounts container. Additionally in future we 
  might
  want apply those setting to the user TGTs as well?
 
  Yeah, that was actually my point. That we are mixing 
  service-specific PAC
  rules to the global setting. Which may be shared with host/*.* 
  principals and
  user principals. This automatic PAC rules may require some 
  designing so that is
  is generally usable.
 
  I think putting everything in the general config is more 
  understandable
  and discoverable. These per-service defaults are basically 
  exceptions to
  the general rule so it make sense to keep everything together.
 
  Simo.
 
 
  Ok, if these are really just an exceptions to the general rule (and 
  there will
  not be too many of them), I think we can leave it in config entry. 
  But if we
  expect to have exceptions for other types of entries (hosts, users), 
  I think we
  should rather use something like service:nfs:NONE do distinguish 
  this exception.
 
  Question is, do we want to implement the interface and processing for 
  that in
  current Sumit's patches or do we use that is they are?
 
  I would like to update the patches so that they can handle the
  service:TYPE style entry and replace the current update code with just
  adding nfs:NONE to the global options. I will update the design page
  accordingly, too.
 
  Ok. If the update procedure shrinks just to adding service:nfs:NONE 
  then it'd
  be great.
 
  If we need to distinguish between service principals and user principals
  I would prefer rather use a special keyword for upns
 
  service: is redundant and I do not want here to be able to say
  upn:martin:NONE because per principal options are available on the
  principal object.
 
  I actually really do not see the need for changing the default just for
  user principals. If we are worried that one day we might want to really
  have upn:NONE, then let's use nfs/:NONE, host/:NONE etc... so one day we
  might add upn:NONE and the lack of / will tell us this is not a service
  named upn/foo.bar.baz but rather it means user principal names.
 
  However I do not see us ever really needing upn:NONE
 
  I would prefer if the enhancements needed for the CLI and WebUI can be
  covered by other/new tickets, but I'm happy to add the needed
  information to the design page too.
 
  bye,
  Sumit
 
  I am OK with adding the interface for this special exception later. In 
  that
  case, a ticket + note in the design as you mentioned would be enough.
 
  Ack.
 
  Simo.
 
 
  Please find attached a new version of the patches. 0095 i(updating) is
  renamed and much simpler now. I opened
  https://fedorahosted.org/freeipa/ticket/3484 to added the needed change
  for 'service:TYPE' to CLI and WebUI. For the time being I've added
  patch 0108 which simply allows 'nfs:NONE' as a type to make sure that it
  is 

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Sumit Bose]

On Wed, Mar 06, 2013 at 08:51:47AM -0500, Simo Sorce wrote:
 On Wed, 2013-03-06 at 14:49 +0100, Martin Kosek wrote:
  On 03/06/2013 10:41 AM, Sumit Bose wrote:
   On Tue, Mar 05, 2013 at 05:13:58PM +0100, Martin Kosek wrote:
   On 03/04/2013 04:22 PM, Sumit Bose wrote:
   On Fri, Mar 01, 2013 at 08:58:34AM -0500, Simo Sorce wrote:
   On Fri, 2013-03-01 at 10:08 +0100, Martin Kosek wrote:
   On 03/01/2013 09:20 AM, Sumit Bose wrote:
   On Fri, Mar 01, 2013 at 08:33:51AM +0100, Martin Kosek wrote:
   On 02/28/2013 03:28 PM, Simo Sorce wrote:
   On Thu, 2013-02-28 at 13:02 +0100, Martin Kosek wrote:
   On 02/28/2013 12:42 PM, Sumit Bose wrote:
   On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
   On 02/27/2013 06:48 PM, Sumit Bose wrote:
  
   Hi Sumit,
  
   This looks like a good idea and would prevent the magic default 
   PAC type, yes.
   Though I would not add this service-specific setting to global 
   IPA config object.
  
   I would rather like to see that in the service tree, for 
   example as a
   configuration option of the service root which could be 
   controlled with
   serviceconfig-* commands (we already have dnsconfig, 
   trustconfig), e.g:
  
   # ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
   # ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
   # ipa serviceconfig-show
 Default PAC Map: nfs:NONE, cifs:PAD
  
   Are you thinking of having this in addition to the 
   for-all-services
   default values in cn=ipaConfig,cn=etc or shall those be dropped? 
   I don't
   like the first case because then three different objects needs 
   to be
   consulted to find out which is the right type. This wouldn't be 
   an issue
   for the plugin, but I think it is hard for the user/admin to 
   follow.
  
   Hm, you are right.
  
  
   If the current defaults shall be dropped I think this is a major 
   change
   because it will require changes in the current CLI and WebUI 
   which will
   be visible to the users. I'm not against this change, I'm just 
   wondering
   if it is worth the effort for the next release?
  
   Maybe an argument to keep this is in global default is that the 
   settings
   are used for the host/*.* services as well which are in a 
   different
   sub-tree of the cn=accounts container. Additionally in future we 
   might
   want apply those setting to the user TGTs as well?
  
   Yeah, that was actually my point. That we are mixing 
   service-specific PAC
   rules to the global setting. Which may be shared with host/*.* 
   principals and
   user principals. This automatic PAC rules may require some 
   designing so that is
   is generally usable.
  
   I think putting everything in the general config is more 
   understandable
   and discoverable. These per-service defaults are basically 
   exceptions to
   the general rule so it make sense to keep everything together.
  
   Simo.
  
  
   Ok, if these are really just an exceptions to the general rule (and 
   there will
   not be too many of them), I think we can leave it in config entry. 
   But if we
   expect to have exceptions for other types of entries (hosts, 
   users), I think we
   should rather use something like service:nfs:NONE do distinguish 
   this exception.
  
   Question is, do we want to implement the interface and processing 
   for that in
   current Sumit's patches or do we use that is they are?
  
   I would like to update the patches so that they can handle the
   service:TYPE style entry and replace the current update code with 
   just
   adding nfs:NONE to the global options. I will update the design page
   accordingly, too.
  
   Ok. If the update procedure shrinks just to adding service:nfs:NONE 
   then it'd
   be great.
  
   If we need to distinguish between service principals and user 
   principals
   I would prefer rather use a special keyword for upns
  
   service: is redundant and I do not want here to be able to say
   upn:martin:NONE because per principal options are available on the
   principal object.
  
   I actually really do not see the need for changing the default just for
   user principals. If we are worried that one day we might want to really
   have upn:NONE, then let's use nfs/:NONE, host/:NONE etc... so one day 
   we
   might add upn:NONE and the lack of / will tell us this is not a service
   named upn/foo.bar.baz but rather it means user principal names.
  
   However I do not see us ever really needing upn:NONE
  
   I would prefer if the enhancements needed for the CLI and WebUI can 
   be
   covered by other/new tickets, but I'm happy to add the needed
   information to the design page too.
  
   bye,
   Sumit
  
   I am OK with adding the interface for this special exception later. 
   In that
   case, a ticket + note in the design as you mentioned would be enough.
  
   Ack.
  
   Simo.
  
  
   Please find attached a new version of the patches. 0095 i(updating) is
   renamed and much simpler now. I opened
   

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Martin Kosek]

On 03/04/2013 04:22 PM, Sumit Bose wrote:
 On Fri, Mar 01, 2013 at 08:58:34AM -0500, Simo Sorce wrote:
 On Fri, 2013-03-01 at 10:08 +0100, Martin Kosek wrote:
 On 03/01/2013 09:20 AM, Sumit Bose wrote:
 On Fri, Mar 01, 2013 at 08:33:51AM +0100, Martin Kosek wrote:
 On 02/28/2013 03:28 PM, Simo Sorce wrote:
 On Thu, 2013-02-28 at 13:02 +0100, Martin Kosek wrote:
 On 02/28/2013 12:42 PM, Sumit Bose wrote:
 On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
 On 02/27/2013 06:48 PM, Sumit Bose wrote:

 Hi Sumit,

 This looks like a good idea and would prevent the magic default PAC 
 type, yes.
 Though I would not add this service-specific setting to global IPA 
 config object.

 I would rather like to see that in the service tree, for example as a
 configuration option of the service root which could be controlled 
 with
 serviceconfig-* commands (we already have dnsconfig, trustconfig), 
 e.g:

 # ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
 # ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
 # ipa serviceconfig-show
   Default PAC Map: nfs:NONE, cifs:PAD

 Are you thinking of having this in addition to the for-all-services
 default values in cn=ipaConfig,cn=etc or shall those be dropped? I 
 don't
 like the first case because then three different objects needs to be
 consulted to find out which is the right type. This wouldn't be an 
 issue
 for the plugin, but I think it is hard for the user/admin to follow.

 Hm, you are right.


 If the current defaults shall be dropped I think this is a major change
 because it will require changes in the current CLI and WebUI which will
 be visible to the users. I'm not against this change, I'm just 
 wondering
 if it is worth the effort for the next release?

 Maybe an argument to keep this is in global default is that the 
 settings
 are used for the host/*.* services as well which are in a different
 sub-tree of the cn=accounts container. Additionally in future we might
 want apply those setting to the user TGTs as well?

 Yeah, that was actually my point. That we are mixing service-specific 
 PAC
 rules to the global setting. Which may be shared with host/*.* 
 principals and
 user principals. This automatic PAC rules may require some designing so 
 that is
 is generally usable.

 I think putting everything in the general config is more understandable
 and discoverable. These per-service defaults are basically exceptions to
 the general rule so it make sense to keep everything together.

 Simo.


 Ok, if these are really just an exceptions to the general rule (and there 
 will
 not be too many of them), I think we can leave it in config entry. But if 
 we
 expect to have exceptions for other types of entries (hosts, users), I 
 think we
 should rather use something like service:nfs:NONE do distinguish this 
 exception.

 Question is, do we want to implement the interface and processing for 
 that in
 current Sumit's patches or do we use that is they are?

 I would like to update the patches so that they can handle the
 service:TYPE style entry and replace the current update code with just
 adding nfs:NONE to the global options. I will update the design page
 accordingly, too.

 Ok. If the update procedure shrinks just to adding service:nfs:NONE then 
 it'd
 be great.

 If we need to distinguish between service principals and user principals
 I would prefer rather use a special keyword for upns

 service: is redundant and I do not want here to be able to say
 upn:martin:NONE because per principal options are available on the
 principal object.

 I actually really do not see the need for changing the default just for
 user principals. If we are worried that one day we might want to really
 have upn:NONE, then let's use nfs/:NONE, host/:NONE etc... so one day we
 might add upn:NONE and the lack of / will tell us this is not a service
 named upn/foo.bar.baz but rather it means user principal names.

 However I do not see us ever really needing upn:NONE

 I would prefer if the enhancements needed for the CLI and WebUI can be
 covered by other/new tickets, but I'm happy to add the needed
 information to the design page too.

 bye,
 Sumit

 I am OK with adding the interface for this special exception later. In that
 case, a ticket + note in the design as you mentioned would be enough.

 Ack.

 Simo.

 
 Please find attached a new version of the patches. 0095 i(updating) is
 renamed and much simpler now. I opened
 https://fedorahosted.org/freeipa/ticket/3484 to added the needed change
 for 'service:TYPE' to CLI and WebUI. For the time being I've added
 patch 0108 which simply allows 'nfs:NONE' as a type to make sure that it
 is not deleted accidentally when e.g. using the WebUI. If you do not
 like it it can simply be dropped, everything is working fine without it.
 
 bye,
 Sumit
 

Patch 0098:

If this part does not match (and it will not for all non-nfs service 
principals):

+if (service_type-length == (sep - 

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Sumit Bose]

On Fri, Mar 01, 2013 at 08:58:34AM -0500, Simo Sorce wrote:
 On Fri, 2013-03-01 at 10:08 +0100, Martin Kosek wrote:
  On 03/01/2013 09:20 AM, Sumit Bose wrote:
   On Fri, Mar 01, 2013 at 08:33:51AM +0100, Martin Kosek wrote:
   On 02/28/2013 03:28 PM, Simo Sorce wrote:
   On Thu, 2013-02-28 at 13:02 +0100, Martin Kosek wrote:
   On 02/28/2013 12:42 PM, Sumit Bose wrote:
   On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
   On 02/27/2013 06:48 PM, Sumit Bose wrote:
  
   Hi Sumit,
  
   This looks like a good idea and would prevent the magic default PAC 
   type, yes.
   Though I would not add this service-specific setting to global IPA 
   config object.
  
   I would rather like to see that in the service tree, for example as a
   configuration option of the service root which could be controlled 
   with
   serviceconfig-* commands (we already have dnsconfig, trustconfig), 
   e.g:
  
   # ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
   # ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
   # ipa serviceconfig-show
 Default PAC Map: nfs:NONE, cifs:PAD
  
   Are you thinking of having this in addition to the for-all-services
   default values in cn=ipaConfig,cn=etc or shall those be dropped? I 
   don't
   like the first case because then three different objects needs to be
   consulted to find out which is the right type. This wouldn't be an 
   issue
   for the plugin, but I think it is hard for the user/admin to follow.
  
   Hm, you are right.
  
  
   If the current defaults shall be dropped I think this is a major 
   change
   because it will require changes in the current CLI and WebUI which 
   will
   be visible to the users. I'm not against this change, I'm just 
   wondering
   if it is worth the effort for the next release?
  
   Maybe an argument to keep this is in global default is that the 
   settings
   are used for the host/*.* services as well which are in a different
   sub-tree of the cn=accounts container. Additionally in future we might
   want apply those setting to the user TGTs as well?
  
   Yeah, that was actually my point. That we are mixing service-specific 
   PAC
   rules to the global setting. Which may be shared with host/*.* 
   principals and
   user principals. This automatic PAC rules may require some designing 
   so that is
   is generally usable.
  
   I think putting everything in the general config is more understandable
   and discoverable. These per-service defaults are basically exceptions to
   the general rule so it make sense to keep everything together.
  
   Simo.
  
  
   Ok, if these are really just an exceptions to the general rule (and 
   there will
   not be too many of them), I think we can leave it in config entry. But 
   if we
   expect to have exceptions for other types of entries (hosts, users), I 
   think we
   should rather use something like service:nfs:NONE do distinguish this 
   exception.
  
   Question is, do we want to implement the interface and processing for 
   that in
   current Sumit's patches or do we use that is they are?
   
   I would like to update the patches so that they can handle the
   service:TYPE style entry and replace the current update code with just
   adding nfs:NONE to the global options. I will update the design page
   accordingly, too.
  
  Ok. If the update procedure shrinks just to adding service:nfs:NONE then 
  it'd
  be great.
 
 If we need to distinguish between service principals and user principals
 I would prefer rather use a special keyword for upns
 
 service: is redundant and I do not want here to be able to say
 upn:martin:NONE because per principal options are available on the
 principal object.
 
 I actually really do not see the need for changing the default just for
 user principals. If we are worried that one day we might want to really
 have upn:NONE, then let's use nfs/:NONE, host/:NONE etc... so one day we
 might add upn:NONE and the lack of / will tell us this is not a service
 named upn/foo.bar.baz but rather it means user principal names.
 
 However I do not see us ever really needing upn:NONE
 
   I would prefer if the enhancements needed for the CLI and WebUI can be
   covered by other/new tickets, but I'm happy to add the needed
   information to the design page too.
   
   bye,
   Sumit
  
  I am OK with adding the interface for this special exception later. In that
  case, a ticket + note in the design as you mentioned would be enough.
 
 Ack.
 
 Simo.
 

Please find attached a new version of the patches. 0095 i(updating) is
renamed and much simpler now. I opened
https://fedorahosted.org/freeipa/ticket/3484 to added the needed change
for 'service:TYPE' to CLI and WebUI. For the time being I've added
patch 0108 which simply allows 'nfs:NONE' as a type to make sure that it
is not deleted accidentally when e.g. using the WebUI. If you do not
like it it can simply be dropped, everything is working fine without it.

bye,
Sumit
From 

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Martin Kosek]

On 03/01/2013 09:20 AM, Sumit Bose wrote:
 On Fri, Mar 01, 2013 at 08:33:51AM +0100, Martin Kosek wrote:
 On 02/28/2013 03:28 PM, Simo Sorce wrote:
 On Thu, 2013-02-28 at 13:02 +0100, Martin Kosek wrote:
 On 02/28/2013 12:42 PM, Sumit Bose wrote:
 On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
 On 02/27/2013 06:48 PM, Sumit Bose wrote:

 Hi Sumit,

 This looks like a good idea and would prevent the magic default PAC 
 type, yes.
 Though I would not add this service-specific setting to global IPA 
 config object.

 I would rather like to see that in the service tree, for example as a
 configuration option of the service root which could be controlled with
 serviceconfig-* commands (we already have dnsconfig, trustconfig), e.g:

 # ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
 # ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
 # ipa serviceconfig-show
   Default PAC Map: nfs:NONE, cifs:PAD

 Are you thinking of having this in addition to the for-all-services
 default values in cn=ipaConfig,cn=etc or shall those be dropped? I don't
 like the first case because then three different objects needs to be
 consulted to find out which is the right type. This wouldn't be an issue
 for the plugin, but I think it is hard for the user/admin to follow.

 Hm, you are right.


 If the current defaults shall be dropped I think this is a major change
 because it will require changes in the current CLI and WebUI which will
 be visible to the users. I'm not against this change, I'm just wondering
 if it is worth the effort for the next release?

 Maybe an argument to keep this is in global default is that the settings
 are used for the host/*.* services as well which are in a different
 sub-tree of the cn=accounts container. Additionally in future we might
 want apply those setting to the user TGTs as well?

 Yeah, that was actually my point. That we are mixing service-specific PAC
 rules to the global setting. Which may be shared with host/*.* 
 principals and
 user principals. This automatic PAC rules may require some designing so 
 that is
 is generally usable.

 I think putting everything in the general config is more understandable
 and discoverable. These per-service defaults are basically exceptions to
 the general rule so it make sense to keep everything together.

 Simo.


 Ok, if these are really just an exceptions to the general rule (and there 
 will
 not be too many of them), I think we can leave it in config entry. But if we
 expect to have exceptions for other types of entries (hosts, users), I think 
 we
 should rather use something like service:nfs:NONE do distinguish this 
 exception.

 Question is, do we want to implement the interface and processing for that in
 current Sumit's patches or do we use that is they are?
 
 I would like to update the patches so that they can handle the
 service:TYPE style entry and replace the current update code with just
 adding nfs:NONE to the global options. I will update the design page
 accordingly, too.

Ok. If the update procedure shrinks just to adding service:nfs:NONE then it'd
be great.

 
 I would prefer if the enhancements needed for the CLI and WebUI can be
 covered by other/new tickets, but I'm happy to add the needed
 information to the design page too.
 
 bye,
 Sumit

I am OK with adding the interface for this special exception later. In that
case, a ticket + note in the design as you mentioned would be enough.

Thanks,
Martin

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

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Simo Sorce]

On Fri, 2013-03-01 at 10:08 +0100, Martin Kosek wrote:
 On 03/01/2013 09:20 AM, Sumit Bose wrote:
  On Fri, Mar 01, 2013 at 08:33:51AM +0100, Martin Kosek wrote:
  On 02/28/2013 03:28 PM, Simo Sorce wrote:
  On Thu, 2013-02-28 at 13:02 +0100, Martin Kosek wrote:
  On 02/28/2013 12:42 PM, Sumit Bose wrote:
  On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
  On 02/27/2013 06:48 PM, Sumit Bose wrote:
 
  Hi Sumit,
 
  This looks like a good idea and would prevent the magic default PAC 
  type, yes.
  Though I would not add this service-specific setting to global IPA 
  config object.
 
  I would rather like to see that in the service tree, for example as a
  configuration option of the service root which could be controlled with
  serviceconfig-* commands (we already have dnsconfig, trustconfig), e.g:
 
  # ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
  # ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
  # ipa serviceconfig-show
Default PAC Map: nfs:NONE, cifs:PAD
 
  Are you thinking of having this in addition to the for-all-services
  default values in cn=ipaConfig,cn=etc or shall those be dropped? I don't
  like the first case because then three different objects needs to be
  consulted to find out which is the right type. This wouldn't be an issue
  for the plugin, but I think it is hard for the user/admin to follow.
 
  Hm, you are right.
 
 
  If the current defaults shall be dropped I think this is a major change
  because it will require changes in the current CLI and WebUI which will
  be visible to the users. I'm not against this change, I'm just wondering
  if it is worth the effort for the next release?
 
  Maybe an argument to keep this is in global default is that the settings
  are used for the host/*.* services as well which are in a different
  sub-tree of the cn=accounts container. Additionally in future we might
  want apply those setting to the user TGTs as well?
 
  Yeah, that was actually my point. That we are mixing service-specific PAC
  rules to the global setting. Which may be shared with host/*.* 
  principals and
  user principals. This automatic PAC rules may require some designing so 
  that is
  is generally usable.
 
  I think putting everything in the general config is more understandable
  and discoverable. These per-service defaults are basically exceptions to
  the general rule so it make sense to keep everything together.
 
  Simo.
 
 
  Ok, if these are really just an exceptions to the general rule (and there 
  will
  not be too many of them), I think we can leave it in config entry. But if 
  we
  expect to have exceptions for other types of entries (hosts, users), I 
  think we
  should rather use something like service:nfs:NONE do distinguish this 
  exception.
 
  Question is, do we want to implement the interface and processing for that 
  in
  current Sumit's patches or do we use that is they are?
  
  I would like to update the patches so that they can handle the
  service:TYPE style entry and replace the current update code with just
  adding nfs:NONE to the global options. I will update the design page
  accordingly, too.
 
 Ok. If the update procedure shrinks just to adding service:nfs:NONE then it'd
 be great.

If we need to distinguish between service principals and user principals
I would prefer rather use a special keyword for upns

service: is redundant and I do not want here to be able to say
upn:martin:NONE because per principal options are available on the
principal object.

I actually really do not see the need for changing the default just for
user principals. If we are worried that one day we might want to really
have upn:NONE, then let's use nfs/:NONE, host/:NONE etc... so one day we
might add upn:NONE and the lack of / will tell us this is not a service
named upn/foo.bar.baz but rather it means user principal names.

However I do not see us ever really needing upn:NONE

  I would prefer if the enhancements needed for the CLI and WebUI can be
  covered by other/new tickets, but I'm happy to add the needed
  information to the design page too.
  
  bye,
  Sumit
 
 I am OK with adding the interface for this special exception later. In that
 case, a ticket + note in the design as you mentioned would be enough.

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] [PATCHES] 94-99 Read and use per-service PAC type

[Sumit Bose]

On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
 On 02/27/2013 06:48 PM, Sumit Bose wrote:
  On Wed, Feb 27, 2013 at 08:37:18AM -0500, Simo Sorce wrote:
  On Wed, 2013-02-27 at 11:58 +0100, Sumit Bose wrote:
  On Mon, Feb 25, 2013 at 04:35:20PM +0100, Martin Kosek wrote:
  On 02/21/2013 04:24 PM, Sumit Bose wrote:
  Hi,
 
  this series of patches fix https://fedorahosted.org/freeipa/ticket/2960
  The related design page is
  http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
 
  It was agreed while discussing the design page that the currently
  hardcoded value for the NFS service will be dropped completely, hence
  the first patch reverts to patch which added the hardcoded value.
 
  The second patch adds the needed attribute to compensate the now missing
  hardcoded value.
 
  In the following three patches the PAC type is read and used
  accordingly. The last patch adds a unit test for a new function.
 
  bye,
  Sumit
 
  I did only sanity testing to the C part of the RFE so far, but by 
  reading it it
  looks ok. It may be beneficial if Simo or Alexander checked if the 
  ipa-kdb part
  is OK.
 
  Alexander asked in irc to change strcmp() to strcasecmp() so that
  options like 'Ms-Pac' or 'none' are accepted as well.
 
  Is there a good reason to allow insensitive matching ?
 
  in LDAP you can't use true or false either for booleans, you have to use
  TRUE and FALSE, so I am not so thrilled to allow insensitive matching
  for this option as well.
  
  I'm fine with this. Alexander, any comments.
  
 
  I will comment on the Python parts:
 
  1) Your update check testing if there is any NFS service with 
  ipakrbauthzdata
  looks like a good heuristics to prevent admin frustration. Though an 
  ideal
  solution would require
  https://fedorahosted.org/freeipa/ticket/2961
  to prevent multiple updates when admin purposefully removes this 
  attribute. We
  may want to reference the ticket in a comment in the update plugin...
 
  I added a comment in the code and in #2961.
 
 
 
  2) The upgrade plugin may get into infinite loop if ldap.find_entries 
  returns
  truncated result. As you do not update entries directly but only return 
  update
  instructions when you have no truncated results, you will loop.
 
  To simulate this, I just created 2 NFS principals and set size_limit=1 
  in your
  find_entries call.
 
  Thanks for catching this, I removed the truncated logic and added a
  messages if truncated was set.
 
 
 
  3) Ok, we set ipakrbauthzdata to NONE on upgrades. But what about new NFS
  service principals added by service-add? Shouldn't we set 
  ipakrbauthzdata for
  new services too? As a default when no user ipakrhbauthzdata is set...
  Otherwise admins could be surprised why their new NFS services do not 
  work
  while the others do.
 
  Also, I think we should have this NFS culprit documented somewhere (i.e. 
  why we
  set ipakrbauthzdata to NONE by default). At least as a small section in 
  the
  online help for services plugin...
 
  I added comments to the help and the doc string in patch #100. If you
  think it is necessary to implicitly set PAC type to 'NONE' if NFS
  services are added and no type is given explicitly, I would prefer if
  you can open a new tickets so that it can be discussed independently.
 
  Thank you for the review. New versions attached.
 
  I do not think we should set the policy to NONE by default, OTOH I see
  the problem with upgrades. Maybe we should have a way to express
  additional defaults, per service type.
 
  Ie add additional values like:
 
  ipakrbAuthsData: nfs:NONE
 
  This would be a default but only for services that have the form nfs/*@*
 
  This would allow us to avoid magical special casing in the code and
  allow admins to change the overall default for specific services as
  needed.
 
  Maybe we should add a new ticket for this ?
  
  This sounds like a good compromise and will make updating much easier.
  If we agree to do it this way I can add ipakrbAuthsData: nfs:NONE and
  fix the code with this ticket. But we would need tickets for the CLI and
  WebUI to handle this new case.
  
  For the WebUI there already is
  https://fedorahosted.org/freeipa/ticket/3404 , maybe it can be extended
  to handle this new case as well?
  
  As for design documents I think I can added the needed information to
  http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
  
  bye,
  Sumit
 
 Hi Sumit,
 
 This looks like a good idea and would prevent the magic default PAC type, yes.
 Though I would not add this service-specific setting to global IPA config 
 object.
 
 I would rather like to see that in the service tree, for example as a
 configuration option of the service root which could be controlled with
 serviceconfig-* commands (we already have dnsconfig, trustconfig), e.g:
 
 # ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
 # ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
 # ipa 

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Martin Kosek]

On 02/28/2013 12:42 PM, Sumit Bose wrote:
 On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
 On 02/27/2013 06:48 PM, Sumit Bose wrote:
 On Wed, Feb 27, 2013 at 08:37:18AM -0500, Simo Sorce wrote:
 On Wed, 2013-02-27 at 11:58 +0100, Sumit Bose wrote:
 On Mon, Feb 25, 2013 at 04:35:20PM +0100, Martin Kosek wrote:
 On 02/21/2013 04:24 PM, Sumit Bose wrote:
 Hi,

 this series of patches fix https://fedorahosted.org/freeipa/ticket/2960
 The related design page is
 http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .

 It was agreed while discussing the design page that the currently
 hardcoded value for the NFS service will be dropped completely, hence
 the first patch reverts to patch which added the hardcoded value.

 The second patch adds the needed attribute to compensate the now missing
 hardcoded value.

 In the following three patches the PAC type is read and used
 accordingly. The last patch adds a unit test for a new function.

 bye,
 Sumit

 I did only sanity testing to the C part of the RFE so far, but by 
 reading it it
 looks ok. It may be beneficial if Simo or Alexander checked if the 
 ipa-kdb part
 is OK.

 Alexander asked in irc to change strcmp() to strcasecmp() so that
 options like 'Ms-Pac' or 'none' are accepted as well.

 Is there a good reason to allow insensitive matching ?

 in LDAP you can't use true or false either for booleans, you have to use
 TRUE and FALSE, so I am not so thrilled to allow insensitive matching
 for this option as well.

 I'm fine with this. Alexander, any comments.


 I will comment on the Python parts:

 1) Your update check testing if there is any NFS service with 
 ipakrbauthzdata
 looks like a good heuristics to prevent admin frustration. Though an 
 ideal
 solution would require
 https://fedorahosted.org/freeipa/ticket/2961
 to prevent multiple updates when admin purposefully removes this 
 attribute. We
 may want to reference the ticket in a comment in the update plugin...

 I added a comment in the code and in #2961.



 2) The upgrade plugin may get into infinite loop if ldap.find_entries 
 returns
 truncated result. As you do not update entries directly but only return 
 update
 instructions when you have no truncated results, you will loop.

 To simulate this, I just created 2 NFS principals and set size_limit=1 
 in your
 find_entries call.

 Thanks for catching this, I removed the truncated logic and added a
 messages if truncated was set.



 3) Ok, we set ipakrbauthzdata to NONE on upgrades. But what about new NFS
 service principals added by service-add? Shouldn't we set 
 ipakrbauthzdata for
 new services too? As a default when no user ipakrhbauthzdata is set...
 Otherwise admins could be surprised why their new NFS services do not 
 work
 while the others do.

 Also, I think we should have this NFS culprit documented somewhere (i.e. 
 why we
 set ipakrbauthzdata to NONE by default). At least as a small section in 
 the
 online help for services plugin...

 I added comments to the help and the doc string in patch #100. If you
 think it is necessary to implicitly set PAC type to 'NONE' if NFS
 services are added and no type is given explicitly, I would prefer if
 you can open a new tickets so that it can be discussed independently.

 Thank you for the review. New versions attached.

 I do not think we should set the policy to NONE by default, OTOH I see
 the problem with upgrades. Maybe we should have a way to express
 additional defaults, per service type.

 Ie add additional values like:

 ipakrbAuthsData: nfs:NONE

 This would be a default but only for services that have the form nfs/*@*

 This would allow us to avoid magical special casing in the code and
 allow admins to change the overall default for specific services as
 needed.

 Maybe we should add a new ticket for this ?

 This sounds like a good compromise and will make updating much easier.
 If we agree to do it this way I can add ipakrbAuthsData: nfs:NONE and
 fix the code with this ticket. But we would need tickets for the CLI and
 WebUI to handle this new case.

 For the WebUI there already is
 https://fedorahosted.org/freeipa/ticket/3404 , maybe it can be extended
 to handle this new case as well?

 As for design documents I think I can added the needed information to
 http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .

 bye,
 Sumit

 Hi Sumit,

 This looks like a good idea and would prevent the magic default PAC type, 
 yes.
 Though I would not add this service-specific setting to global IPA config 
 object.

 I would rather like to see that in the service tree, for example as a
 configuration option of the service root which could be controlled with
 serviceconfig-* commands (we already have dnsconfig, trustconfig), e.g:

 # ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
 # ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
 # ipa serviceconfig-show
   Default PAC Map: nfs:NONE, cifs:PAD
 
 Are you thinking of having this in 

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Simo Sorce]

On Thu, 2013-02-28 at 13:02 +0100, Martin Kosek wrote:
 On 02/28/2013 12:42 PM, Sumit Bose wrote:
  On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
  On 02/27/2013 06:48 PM, Sumit Bose wrote:

  Hi Sumit,
 
  This looks like a good idea and would prevent the magic default PAC type, 
  yes.
  Though I would not add this service-specific setting to global IPA config 
  object.
 
  I would rather like to see that in the service tree, for example as a
  configuration option of the service root which could be controlled with
  serviceconfig-* commands (we already have dnsconfig, trustconfig), e.g:
 
  # ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
  # ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
  # ipa serviceconfig-show
Default PAC Map: nfs:NONE, cifs:PAD
  
  Are you thinking of having this in addition to the for-all-services
  default values in cn=ipaConfig,cn=etc or shall those be dropped? I don't
  like the first case because then three different objects needs to be
  consulted to find out which is the right type. This wouldn't be an issue
  for the plugin, but I think it is hard for the user/admin to follow.
 
 Hm, you are right.
 
  
  If the current defaults shall be dropped I think this is a major change
  because it will require changes in the current CLI and WebUI which will
  be visible to the users. I'm not against this change, I'm just wondering
  if it is worth the effort for the next release?
  
  Maybe an argument to keep this is in global default is that the settings
  are used for the host/*.* services as well which are in a different
  sub-tree of the cn=accounts container. Additionally in future we might
  want apply those setting to the user TGTs as well?
 
 Yeah, that was actually my point. That we are mixing service-specific PAC
 rules to the global setting. Which may be shared with host/*.* principals 
 and
 user principals. This automatic PAC rules may require some designing so that 
 is
 is generally usable.

I think putting everything in the general config is more understandable
and discoverable. These per-service defaults are basically exceptions to
the general rule so it make sense to keep everything together.

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] [PATCHES] 94-99 Read and use per-service PAC type

[Martin Kosek]

On 02/28/2013 03:28 PM, Simo Sorce wrote:
 On Thu, 2013-02-28 at 13:02 +0100, Martin Kosek wrote:
 On 02/28/2013 12:42 PM, Sumit Bose wrote:
 On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
 On 02/27/2013 06:48 PM, Sumit Bose wrote:
 
 Hi Sumit,

 This looks like a good idea and would prevent the magic default PAC type, 
 yes.
 Though I would not add this service-specific setting to global IPA config 
 object.

 I would rather like to see that in the service tree, for example as a
 configuration option of the service root which could be controlled with
 serviceconfig-* commands (we already have dnsconfig, trustconfig), e.g:

 # ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
 # ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
 # ipa serviceconfig-show
   Default PAC Map: nfs:NONE, cifs:PAD

 Are you thinking of having this in addition to the for-all-services
 default values in cn=ipaConfig,cn=etc or shall those be dropped? I don't
 like the first case because then three different objects needs to be
 consulted to find out which is the right type. This wouldn't be an issue
 for the plugin, but I think it is hard for the user/admin to follow.

 Hm, you are right.


 If the current defaults shall be dropped I think this is a major change
 because it will require changes in the current CLI and WebUI which will
 be visible to the users. I'm not against this change, I'm just wondering
 if it is worth the effort for the next release?

 Maybe an argument to keep this is in global default is that the settings
 are used for the host/*.* services as well which are in a different
 sub-tree of the cn=accounts container. Additionally in future we might
 want apply those setting to the user TGTs as well?

 Yeah, that was actually my point. That we are mixing service-specific PAC
 rules to the global setting. Which may be shared with host/*.* principals 
 and
 user principals. This automatic PAC rules may require some designing so that 
 is
 is generally usable.
 
 I think putting everything in the general config is more understandable
 and discoverable. These per-service defaults are basically exceptions to
 the general rule so it make sense to keep everything together.
 
 Simo.
 

Ok, if these are really just an exceptions to the general rule (and there will
not be too many of them), I think we can leave it in config entry. But if we
expect to have exceptions for other types of entries (hosts, users), I think we
should rather use something like service:nfs:NONE do distinguish this 
exception.

Question is, do we want to implement the interface and processing for that in
current Sumit's patches or do we use that is they are?

Martin

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

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Sumit Bose]

On Mon, Feb 25, 2013 at 04:35:20PM +0100, Martin Kosek wrote:
 On 02/21/2013 04:24 PM, Sumit Bose wrote:
  Hi,
  
  this series of patches fix https://fedorahosted.org/freeipa/ticket/2960
  The related design page is
  http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
  
  It was agreed while discussing the design page that the currently
  hardcoded value for the NFS service will be dropped completely, hence
  the first patch reverts to patch which added the hardcoded value.
  
  The second patch adds the needed attribute to compensate the now missing
  hardcoded value.
  
  In the following three patches the PAC type is read and used
  accordingly. The last patch adds a unit test for a new function.
  
  bye,
  Sumit
 
 I did only sanity testing to the C part of the RFE so far, but by reading it 
 it
 looks ok. It may be beneficial if Simo or Alexander checked if the ipa-kdb 
 part
 is OK.

Alexander asked in irc to change strcmp() to strcasecmp() so that
options like 'Ms-Pac' or 'none' are accepted as well.

 
 I will comment on the Python parts:
 
 1) Your update check testing if there is any NFS service with ipakrbauthzdata
 looks like a good heuristics to prevent admin frustration. Though an ideal
 solution would require
 https://fedorahosted.org/freeipa/ticket/2961
 to prevent multiple updates when admin purposefully removes this attribute. We
 may want to reference the ticket in a comment in the update plugin...

I added a comment in the code and in #2961.

 
 
 2) The upgrade plugin may get into infinite loop if ldap.find_entries returns
 truncated result. As you do not update entries directly but only return update
 instructions when you have no truncated results, you will loop.
 
 To simulate this, I just created 2 NFS principals and set size_limit=1 in your
 find_entries call.

Thanks for catching this, I removed the truncated logic and added a
messages if truncated was set.

 
 
 3) Ok, we set ipakrbauthzdata to NONE on upgrades. But what about new NFS
 service principals added by service-add? Shouldn't we set ipakrbauthzdata for
 new services too? As a default when no user ipakrhbauthzdata is set...
 Otherwise admins could be surprised why their new NFS services do not work
 while the others do.
 
 Also, I think we should have this NFS culprit documented somewhere (i.e. why 
 we
 set ipakrbauthzdata to NONE by default). At least as a small section in the
 online help for services plugin...

I added comments to the help and the doc string in patch #100. If you
think it is necessary to implicitly set PAC type to 'NONE' if NFS
services are added and no type is given explicitly, I would prefer if
you can open a new tickets so that it can be discussed independently.

Thank you for the review. New versions attached.

bye,
Sumit

 
 Martin
From 8eb34dd3190853bd3fa400434be4f7c720bf1354 Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Tue, 12 Feb 2013 09:59:00 +0100
Subject: [PATCH 094/100] Revert MS-PAC: Special case NFS services

This reverts commit 5269458f552380759c86018cd1f30b64761be92e.

With the implementation of https://fedorahosted.org/freeipa/ticket/2960
a special hardcoded handling of NFS service tickets is not needed
anymore.
---
 daemons/ipa-kdb/ipa_kdb_mspac.c | 36 +---
 1 file changed, 1 insertion(+), 35 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index 
95349702038e1d55bd257816f7f61e9557a5..f05c552fc18eb9b15ee3e39b513184589bc8c468
 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -743,24 +743,6 @@ static bool is_cross_realm_krbtgt(krb5_const_principal 
princ)
 return true;
 }
 
-static bool is_service_of_type(krb5_const_principal princ, const char *type)
-{
-size_t len;
-
-if (princ-length  2) {
-return false;
-}
-
-len = strlen(type);
-
-if ((princ-data[0].length == len) ||
-(strncasecmp(princ-data[0].data, type, len) == 0)) {
-return true;
-}
-
-return false;
-}
-
 static char *gen_sid_string(TALLOC_CTX *memctx, struct dom_sid *dom_sid,
 uint32_t rid)
 {
@@ -1555,7 +1537,6 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
 krb5_error_code kerr;
 krb5_pac pac = NULL;
 krb5_data pac_data;
-bool is_nfs = false;
 
 /* When using s4u2proxy client_princ actually refers to the proxied user
  * while client-princ to the proxy service asking for the TGS on behalf
@@ -1566,32 +1547,17 @@ krb5_error_code ipadb_sign_authdata(krb5_context 
context,
 ks_client_princ = client-princ;
 }
 
-/* NFS Server on Linux is limited and will choke on big tickets.
- * So avoid attachnig the PAC to nfs/ tickets for now.
- * FIXME: remove this when we have interface to support disabling
- * PACs on arbitrary services */
-if (is_service_of_type(ks_client_princ, nfs) ||
-is_service_of_type(server-princ, nfs)) 

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Simo Sorce]

On Wed, 2013-02-27 at 11:58 +0100, Sumit Bose wrote:
 On Mon, Feb 25, 2013 at 04:35:20PM +0100, Martin Kosek wrote:
  On 02/21/2013 04:24 PM, Sumit Bose wrote:
   Hi,
   
   this series of patches fix https://fedorahosted.org/freeipa/ticket/2960
   The related design page is
   http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
   
   It was agreed while discussing the design page that the currently
   hardcoded value for the NFS service will be dropped completely, hence
   the first patch reverts to patch which added the hardcoded value.
   
   The second patch adds the needed attribute to compensate the now missing
   hardcoded value.
   
   In the following three patches the PAC type is read and used
   accordingly. The last patch adds a unit test for a new function.
   
   bye,
   Sumit
  
  I did only sanity testing to the C part of the RFE so far, but by reading 
  it it
  looks ok. It may be beneficial if Simo or Alexander checked if the ipa-kdb 
  part
  is OK.
 
 Alexander asked in irc to change strcmp() to strcasecmp() so that
 options like 'Ms-Pac' or 'none' are accepted as well.

Is there a good reason to allow insensitive matching ?

in LDAP you can't use true or false either for booleans, you have to use
TRUE and FALSE, so I am not so thrilled to allow insensitive matching
for this option as well.

  I will comment on the Python parts:
  
  1) Your update check testing if there is any NFS service with 
  ipakrbauthzdata
  looks like a good heuristics to prevent admin frustration. Though an ideal
  solution would require
  https://fedorahosted.org/freeipa/ticket/2961
  to prevent multiple updates when admin purposefully removes this attribute. 
  We
  may want to reference the ticket in a comment in the update plugin...
 
 I added a comment in the code and in #2961.
 
  
  
  2) The upgrade plugin may get into infinite loop if ldap.find_entries 
  returns
  truncated result. As you do not update entries directly but only return 
  update
  instructions when you have no truncated results, you will loop.
  
  To simulate this, I just created 2 NFS principals and set size_limit=1 in 
  your
  find_entries call.
 
 Thanks for catching this, I removed the truncated logic and added a
 messages if truncated was set.
 
  
  
  3) Ok, we set ipakrbauthzdata to NONE on upgrades. But what about new NFS
  service principals added by service-add? Shouldn't we set ipakrbauthzdata 
  for
  new services too? As a default when no user ipakrhbauthzdata is set...
  Otherwise admins could be surprised why their new NFS services do not work
  while the others do.
  
  Also, I think we should have this NFS culprit documented somewhere (i.e. 
  why we
  set ipakrbauthzdata to NONE by default). At least as a small section in the
  online help for services plugin...
 
 I added comments to the help and the doc string in patch #100. If you
 think it is necessary to implicitly set PAC type to 'NONE' if NFS
 services are added and no type is given explicitly, I would prefer if
 you can open a new tickets so that it can be discussed independently.
 
 Thank you for the review. New versions attached.

I do not think we should set the policy to NONE by default, OTOH I see
the problem with upgrades. Maybe we should have a way to express
additional defaults, per service type.

Ie add additional values like:

ipakrbAuthsData: nfs:NONE

This would be a default but only for services that have the form nfs/*@*

This would allow us to avoid magical special casing in the code and
allow admins to change the overall default for specific services as
needed.

Maybe we should add a new ticket for this ?

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] [PATCHES] 94-99 Read and use per-service PAC type

[Sumit Bose]

On Wed, Feb 27, 2013 at 06:48:27PM +0100, Sumit Bose wrote:
 On Wed, Feb 27, 2013 at 08:37:18AM -0500, Simo Sorce wrote:
  On Wed, 2013-02-27 at 11:58 +0100, Sumit Bose wrote:
   On Mon, Feb 25, 2013 at 04:35:20PM +0100, Martin Kosek wrote:
On 02/21/2013 04:24 PM, Sumit Bose wrote:
 Hi,
 
 this series of patches fix 
 https://fedorahosted.org/freeipa/ticket/2960
 The related design page is
 http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
 
 It was agreed while discussing the design page that the currently
 hardcoded value for the NFS service will be dropped completely, hence
 the first patch reverts to patch which added the hardcoded value.
 
 The second patch adds the needed attribute to compensate the now 
 missing
 hardcoded value.
 
 In the following three patches the PAC type is read and used
 accordingly. The last patch adds a unit test for a new function.
 
 bye,
 Sumit

I did only sanity testing to the C part of the RFE so far, but by 
reading it it
looks ok. It may be beneficial if Simo or Alexander checked if the 
ipa-kdb part
is OK.
   
   Alexander asked in irc to change strcmp() to strcasecmp() so that
   options like 'Ms-Pac' or 'none' are accepted as well.
  
  Is there a good reason to allow insensitive matching ?
  
  in LDAP you can't use true or false either for booleans, you have to use
  TRUE and FALSE, so I am not so thrilled to allow insensitive matching
  for this option as well.
 
 I'm fine with this. Alexander, any comments.

Alexander said on irc that he is fine with the strict case-sensitive
handling of the values, as long as the user input is automatically
upper-cased by the framework as it is currently done for booleans. Since
the WebUI does not allow user entered values this will only affect the
CLI.

bye,
Sumit

 
  
I will comment on the Python parts:

1) Your update check testing if there is any NFS service with 
ipakrbauthzdata
looks like a good heuristics to prevent admin frustration. Though an 
ideal
solution would require
https://fedorahosted.org/freeipa/ticket/2961
to prevent multiple updates when admin purposefully removes this 
attribute. We
may want to reference the ticket in a comment in the update plugin...
   
   I added a comment in the code and in #2961.
   


2) The upgrade plugin may get into infinite loop if ldap.find_entries 
returns
truncated result. As you do not update entries directly but only return 
update
instructions when you have no truncated results, you will loop.

To simulate this, I just created 2 NFS principals and set size_limit=1 
in your
find_entries call.
   
   Thanks for catching this, I removed the truncated logic and added a
   messages if truncated was set.
   


3) Ok, we set ipakrbauthzdata to NONE on upgrades. But what about new 
NFS
service principals added by service-add? Shouldn't we set 
ipakrbauthzdata for
new services too? As a default when no user ipakrhbauthzdata is set...
Otherwise admins could be surprised why their new NFS services do not 
work
while the others do.

Also, I think we should have this NFS culprit documented somewhere 
(i.e. why we
set ipakrbauthzdata to NONE by default). At least as a small section in 
the
online help for services plugin...
   
   I added comments to the help and the doc string in patch #100. If you
   think it is necessary to implicitly set PAC type to 'NONE' if NFS
   services are added and no type is given explicitly, I would prefer if
   you can open a new tickets so that it can be discussed independently.
   
   Thank you for the review. New versions attached.
  
  I do not think we should set the policy to NONE by default, OTOH I see
  the problem with upgrades. Maybe we should have a way to express
  additional defaults, per service type.
  
  Ie add additional values like:
  
  ipakrbAuthsData: nfs:NONE
  
  This would be a default but only for services that have the form nfs/*@*
  
  This would allow us to avoid magical special casing in the code and
  allow admins to change the overall default for specific services as
  needed.
  
  Maybe we should add a new ticket for this ?
 
 This sounds like a good compromise and will make updating much easier.
 If we agree to do it this way I can add ipakrbAuthsData: nfs:NONE and
 fix the code with this ticket. But we would need tickets for the CLI and
 WebUI to handle this new case.
 
 For the WebUI there already is
 https://fedorahosted.org/freeipa/ticket/3404 , maybe it can be extended
 to handle this new case as well?
 
 As for design documents I think I can added the needed information to
 http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
 
 bye,
 Sumit
  
  Simo.
  
  -- 
  Simo Sorce * Red Hat, Inc * New York
  
 
 

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Martin Kosek]

On 02/27/2013 06:48 PM, Sumit Bose wrote:
 On Wed, Feb 27, 2013 at 08:37:18AM -0500, Simo Sorce wrote:
 On Wed, 2013-02-27 at 11:58 +0100, Sumit Bose wrote:
 On Mon, Feb 25, 2013 at 04:35:20PM +0100, Martin Kosek wrote:
 On 02/21/2013 04:24 PM, Sumit Bose wrote:
 Hi,

 this series of patches fix https://fedorahosted.org/freeipa/ticket/2960
 The related design page is
 http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .

 It was agreed while discussing the design page that the currently
 hardcoded value for the NFS service will be dropped completely, hence
 the first patch reverts to patch which added the hardcoded value.

 The second patch adds the needed attribute to compensate the now missing
 hardcoded value.

 In the following three patches the PAC type is read and used
 accordingly. The last patch adds a unit test for a new function.

 bye,
 Sumit

 I did only sanity testing to the C part of the RFE so far, but by reading 
 it it
 looks ok. It may be beneficial if Simo or Alexander checked if the ipa-kdb 
 part
 is OK.

 Alexander asked in irc to change strcmp() to strcasecmp() so that
 options like 'Ms-Pac' or 'none' are accepted as well.

 Is there a good reason to allow insensitive matching ?

 in LDAP you can't use true or false either for booleans, you have to use
 TRUE and FALSE, so I am not so thrilled to allow insensitive matching
 for this option as well.
 
 I'm fine with this. Alexander, any comments.
 

 I will comment on the Python parts:

 1) Your update check testing if there is any NFS service with 
 ipakrbauthzdata
 looks like a good heuristics to prevent admin frustration. Though an ideal
 solution would require
 https://fedorahosted.org/freeipa/ticket/2961
 to prevent multiple updates when admin purposefully removes this 
 attribute. We
 may want to reference the ticket in a comment in the update plugin...

 I added a comment in the code and in #2961.



 2) The upgrade plugin may get into infinite loop if ldap.find_entries 
 returns
 truncated result. As you do not update entries directly but only return 
 update
 instructions when you have no truncated results, you will loop.

 To simulate this, I just created 2 NFS principals and set size_limit=1 in 
 your
 find_entries call.

 Thanks for catching this, I removed the truncated logic and added a
 messages if truncated was set.



 3) Ok, we set ipakrbauthzdata to NONE on upgrades. But what about new NFS
 service principals added by service-add? Shouldn't we set ipakrbauthzdata 
 for
 new services too? As a default when no user ipakrhbauthzdata is set...
 Otherwise admins could be surprised why their new NFS services do not work
 while the others do.

 Also, I think we should have this NFS culprit documented somewhere (i.e. 
 why we
 set ipakrbauthzdata to NONE by default). At least as a small section in the
 online help for services plugin...

 I added comments to the help and the doc string in patch #100. If you
 think it is necessary to implicitly set PAC type to 'NONE' if NFS
 services are added and no type is given explicitly, I would prefer if
 you can open a new tickets so that it can be discussed independently.

 Thank you for the review. New versions attached.

 I do not think we should set the policy to NONE by default, OTOH I see
 the problem with upgrades. Maybe we should have a way to express
 additional defaults, per service type.

 Ie add additional values like:

 ipakrbAuthsData: nfs:NONE

 This would be a default but only for services that have the form nfs/*@*

 This would allow us to avoid magical special casing in the code and
 allow admins to change the overall default for specific services as
 needed.

 Maybe we should add a new ticket for this ?
 
 This sounds like a good compromise and will make updating much easier.
 If we agree to do it this way I can add ipakrbAuthsData: nfs:NONE and
 fix the code with this ticket. But we would need tickets for the CLI and
 WebUI to handle this new case.
 
 For the WebUI there already is
 https://fedorahosted.org/freeipa/ticket/3404 , maybe it can be extended
 to handle this new case as well?
 
 As for design documents I think I can added the needed information to
 http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
 
 bye,
 Sumit

Hi Sumit,

This looks like a good idea and would prevent the magic default PAC type, yes.
Though I would not add this service-specific setting to global IPA config 
object.

I would rather like to see that in the service tree, for example as a
configuration option of the service root which could be controlled with
serviceconfig-* commands (we already have dnsconfig, trustconfig), e.g:

# ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
# ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
# ipa serviceconfig-show
  Default PAC Map: nfs:NONE, cifs:PAD

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Martin Kosek]

On 02/21/2013 04:24 PM, Sumit Bose wrote:
 Hi,
 
 this series of patches fix https://fedorahosted.org/freeipa/ticket/2960
 The related design page is
 http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
 
 It was agreed while discussing the design page that the currently
 hardcoded value for the NFS service will be dropped completely, hence
 the first patch reverts to patch which added the hardcoded value.
 
 The second patch adds the needed attribute to compensate the now missing
 hardcoded value.
 
 In the following three patches the PAC type is read and used
 accordingly. The last patch adds a unit test for a new function.
 
 bye,
 Sumit

I did only sanity testing to the C part of the RFE so far, but by reading it it
looks ok. It may be beneficial if Simo or Alexander checked if the ipa-kdb part
is OK.

I will comment on the Python parts:

1) Your update check testing if there is any NFS service with ipakrbauthzdata
looks like a good heuristics to prevent admin frustration. Though an ideal
solution would require
https://fedorahosted.org/freeipa/ticket/2961
to prevent multiple updates when admin purposefully removes this attribute. We
may want to reference the ticket in a comment in the update plugin...


2) The upgrade plugin may get into infinite loop if ldap.find_entries returns
truncated result. As you do not update entries directly but only return update
instructions when you have no truncated results, you will loop.

To simulate this, I just created 2 NFS principals and set size_limit=1 in your
find_entries call.


3) Ok, we set ipakrbauthzdata to NONE on upgrades. But what about new NFS
service principals added by service-add? Shouldn't we set ipakrbauthzdata for
new services too? As a default when no user ipakrhbauthzdata is set...
Otherwise admins could be surprised why their new NFS services do not work
while the others do.

Also, I think we should have this NFS culprit documented somewhere (i.e. why we
set ipakrbauthzdata to NONE by default). At least as a small section in the
online help for services plugin...

Martin

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