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 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 - authz_data_list[c]) &&
+                strncmp(authz_data_list[c], service_type->data,
+                        service_type->length) == 0) {
+                authz_data_type = sep + 1;
+            }

krb5kdc.log will contain an error:

Mar 05 10:48:50 ipa.linux.ad.test krb5kdc[26703](Error): Ignoring unsupported
authorization data type [nfs:NONE].
Mar 05 10:48:50 ipa.linux.ad.test krb5kdc[26703](info): TGS_REQ (4 etypes {18
17 16 23}) 10.16.78.33: ISSUE: authtime 1362482261, etypes {rep=18 tkt=18
ses=18}, HTTP/ipa.linux.ad.t...@linux.ad.test for
ldap/ipa.linux.ad.t...@linux.ad.test

I think we should just "continue" in this case.

good catch, fixed

Ok, thanks.



Otherwise, this looks and works fine.

Thank you for the review, new version attached.

I have an additional question about processing the service specific
defaults. Using 'service:NONE' is unambiguous because NONE trumps all.
But what do we expect if e.g. the defaults are MS-PAC and ldap:PAD for a
LDAP service ticket. Shall it contain PAC and PAD or only a PAD? I think
the first one where all defaults which apply to a service are added up
is more clear, and this is also the way the current code works. But I
wouldn't mind to change the logic if you think it makes more sense the
other way round, i.e. if there is a service specific default matching
the requested service only the service specific default are accounted.

Hmm, that's a good question. I understand service:PACTYPE as an exclusion to
the general setting, so for me it would make sense to override the global
config - i.e. tje second case.

Thus, if global config is MS-PAC, ldap:PAD, I think that ldap should have just
PAD. If one also want MS-PAC, it should be set like "MS-PAC, ldap:PAD,
ldap:MS-PAC". Otherwise you would not be able to configure that you want MS-PAC
for all services and _only_ PAD for ldap...

Does it make sense? We should also make sure that we update the RFE page to
whatever we decide to do.

+1 to Martin's choice.

Martin, Simo, thank you for the feedback. I will send an new version of
the patches, add a section in the design page explaining the details and
add some tests for this logic.

bye,
Sumit

Design pages is updated and new patches attached.

bye,
Sumit


Thanks, this works great. I just updated the commit messages and added a link to the ticket.

ACK. Pushed to master.

Martin

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

Reply via email to