Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-20 Thread Alexander Bokovoy

On Wed, 19 Nov 2014, Simo Sorce wrote:

- Original Message -

From: Alexander Bokovoy aboko...@redhat.com

[...]


Regarding the patchset itself:

Patch 0001: fix 'wuld' in the commit message. The rest is fine.


Fixed.


Patch 0002:
 - ticket number is missing in the commit message


Added.


 - perhaps, an instruction how to regenerate asn1 code can be made a
   Makefile target? We don't need to call it ourselves but this would
   simplify things in future


Added make regenerate target to asn1c makefile


 - I'm little uncomfortable how ASN_DEBUG() output goes explicitly to
   stderr but I guess this is something we currently cannot override
   with DS-specific log printing, so no big deal right now


ASN_DEBUG() is currently disabled as EMIT_ASN_DEBUG is undefined, we can
later provide a replacement ASN_DEBUG function to hook debugging, but
given the same code is used in both DS plugins and ipa-getkeytab binary
I did not want to assume anything, and how to wire it up (if we even want
to) should probably be discussed at a later time.


 - any specific need to get asn1/compile committed? We don't commit it
   in the client code (ipa-client/compile).


Added 'compile' to .gitignore in second patch


Patch 0003: OK


Nothing changed here.

I also remembered the patch naming policy :-) so new patch names/numbers
are 514,515,516, third revision.

Thanks. The only complaint I have left is number of whitespace errors that git
says are in the 515th patch.

Otherwise, ACK. I've tested it again and everything works except getting
stronger than asked TGT enctype but this is not an issue with getkeytab
controls.
--
/ Alexander Bokovoy

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


Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-20 Thread Simo Sorce
On Thu, 20 Nov 2014 12:36:45 +0200
Alexander Bokovoy aboko...@redhat.com wrote:

 On Wed, 19 Nov 2014, Simo Sorce wrote:
 - Original Message -
  From: Alexander Bokovoy aboko...@redhat.com
 [...]
 
  Regarding the patchset itself:
 
  Patch 0001: fix 'wuld' in the commit message. The rest is fine.
 
 Fixed.
 
  Patch 0002:
   - ticket number is missing in the commit message
 
 Added.
 
   - perhaps, an instruction how to regenerate asn1 code can be made
  a Makefile target? We don't need to call it ourselves but this
  would simplify things in future
 
 Added make regenerate target to asn1c makefile
 
   - I'm little uncomfortable how ASN_DEBUG() output goes explicitly
  to stderr but I guess this is something we currently cannot
  override with DS-specific log printing, so no big deal right now
 
 ASN_DEBUG() is currently disabled as EMIT_ASN_DEBUG is undefined, we
 can later provide a replacement ASN_DEBUG function to hook
 debugging, but given the same code is used in both DS plugins and
 ipa-getkeytab binary I did not want to assume anything, and how to
 wire it up (if we even want to) should probably be discussed at a
 later time.
 
   - any specific need to get asn1/compile committed? We don't
  commit it in the client code (ipa-client/compile).
 
 Added 'compile' to .gitignore in second patch
 
  Patch 0003: OK
 
 Nothing changed here.
 
 I also remembered the patch naming policy :-) so new patch
 names/numbers are 514,515,516, third revision.
 Thanks. The only complaint I have left is number of whitespace errors
 that git says are in the 515th patch.

Yeah the autogenerated code is not a pretty sight style-wise, do we
want to run an automatic indenter on it ?
I was hesitant to do so, but I wouldn't mind adding that, if we feel
strongly about it.

 Otherwise, ACK. I've tested it again and everything works except
 getting stronger than asked TGT enctype but this is not an issue with
 getkeytab controls.

ok,
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] Fix getkeytab operation

2014-11-20 Thread Nathaniel McCallum
On Thu, 2014-11-20 at 09:12 -0500, Simo Sorce wrote:
 On Thu, 20 Nov 2014 12:36:45 +0200
 Alexander Bokovoy aboko...@redhat.com wrote:
 
  On Wed, 19 Nov 2014, Simo Sorce wrote:
  - Original Message -
   From: Alexander Bokovoy aboko...@redhat.com
  [...]
  
   Regarding the patchset itself:
  
   Patch 0001: fix 'wuld' in the commit message. The rest is fine.
  
  Fixed.
  
   Patch 0002:
- ticket number is missing in the commit message
  
  Added.
  
- perhaps, an instruction how to regenerate asn1 code can be made
   a Makefile target? We don't need to call it ourselves but this
   would simplify things in future
  
  Added make regenerate target to asn1c makefile
  
- I'm little uncomfortable how ASN_DEBUG() output goes explicitly
   to stderr but I guess this is something we currently cannot
   override with DS-specific log printing, so no big deal right now
  
  ASN_DEBUG() is currently disabled as EMIT_ASN_DEBUG is undefined, we
  can later provide a replacement ASN_DEBUG function to hook
  debugging, but given the same code is used in both DS plugins and
  ipa-getkeytab binary I did not want to assume anything, and how to
  wire it up (if we even want to) should probably be discussed at a
  later time.
  
- any specific need to get asn1/compile committed? We don't
   commit it in the client code (ipa-client/compile).
  
  Added 'compile' to .gitignore in second patch
  
   Patch 0003: OK
  
  Nothing changed here.
  
  I also remembered the patch naming policy :-) so new patch
  names/numbers are 514,515,516, third revision.
  Thanks. The only complaint I have left is number of whitespace errors
  that git says are in the 515th patch.
 
 Yeah the autogenerated code is not a pretty sight style-wise, do we
 want to run an automatic indenter on it ?
 I was hesitant to do so, but I wouldn't mind adding that, if we feel
 strongly about it.

Let's please not try to correct autogenerated code.

  Otherwise, ACK. I've tested it again and everything works except
  getting stronger than asked TGT enctype but this is not an issue with
  getkeytab controls.


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


Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-20 Thread Alexander Bokovoy

On Thu, 20 Nov 2014, Nathaniel McCallum wrote:

On Thu, 2014-11-20 at 09:12 -0500, Simo Sorce wrote:

On Thu, 20 Nov 2014 12:36:45 +0200
Alexander Bokovoy aboko...@redhat.com wrote:

 On Wed, 19 Nov 2014, Simo Sorce wrote:
 - Original Message -
  From: Alexander Bokovoy aboko...@redhat.com
 [...]
 
  Regarding the patchset itself:
 
  Patch 0001: fix 'wuld' in the commit message. The rest is fine.
 
 Fixed.
 
  Patch 0002:
   - ticket number is missing in the commit message
 
 Added.
 
   - perhaps, an instruction how to regenerate asn1 code can be made
  a Makefile target? We don't need to call it ourselves but this
  would simplify things in future
 
 Added make regenerate target to asn1c makefile
 
   - I'm little uncomfortable how ASN_DEBUG() output goes explicitly
  to stderr but I guess this is something we currently cannot
  override with DS-specific log printing, so no big deal right now
 
 ASN_DEBUG() is currently disabled as EMIT_ASN_DEBUG is undefined, we
 can later provide a replacement ASN_DEBUG function to hook
 debugging, but given the same code is used in both DS plugins and
 ipa-getkeytab binary I did not want to assume anything, and how to
 wire it up (if we even want to) should probably be discussed at a
 later time.
 
   - any specific need to get asn1/compile committed? We don't
  commit it in the client code (ipa-client/compile).
 
 Added 'compile' to .gitignore in second patch
 
  Patch 0003: OK
 
 Nothing changed here.
 
 I also remembered the patch naming policy :-) so new patch
 names/numbers are 514,515,516, third revision.
 Thanks. The only complaint I have left is number of whitespace errors
 that git says are in the 515th patch.

Yeah the autogenerated code is not a pretty sight style-wise, do we
want to run an automatic indenter on it ?
I was hesitant to do so, but I wouldn't mind adding that, if we feel
strongly about it.


Let's please not try to correct autogenerated code.

I'm not tied to this but Simo now thinks it is better to run indenter in
the generator rule as this will give less problems in actual comparison
noise that git diff would give.

I'll make sure to talk back to asn1c author to see if we can improve its
generators upstream.


--
/ Alexander Bokovoy

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


Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-20 Thread Simo Sorce
On Thu, 20 Nov 2014 16:47:29 +0200
Alexander Bokovoy aboko...@redhat.com wrote:

 On Thu, 20 Nov 2014, Nathaniel McCallum wrote:
 On Thu, 2014-11-20 at 09:12 -0500, Simo Sorce wrote:
  On Thu, 20 Nov 2014 12:36:45 +0200
  Alexander Bokovoy aboko...@redhat.com wrote:
 
   On Wed, 19 Nov 2014, Simo Sorce wrote:
   - Original Message -
From: Alexander Bokovoy aboko...@redhat.com
   [...]
   
Regarding the patchset itself:
   
Patch 0001: fix 'wuld' in the commit message. The rest is
fine.
   
   Fixed.
   
Patch 0002:
 - ticket number is missing in the commit message
   
   Added.
   
 - perhaps, an instruction how to regenerate asn1 code can be
made a Makefile target? We don't need to call it ourselves
but this would simplify things in future
   
   Added make regenerate target to asn1c makefile
   
 - I'm little uncomfortable how ASN_DEBUG() output goes
explicitly to stderr but I guess this is something we
currently cannot override with DS-specific log printing, so
no big deal right now
   
   ASN_DEBUG() is currently disabled as EMIT_ASN_DEBUG is
   undefined, we can later provide a replacement ASN_DEBUG
   function to hook debugging, but given the same code is used in
   both DS plugins and ipa-getkeytab binary I did not want to
   assume anything, and how to wire it up (if we even want to)
   should probably be discussed at a later time.
   
 - any specific need to get asn1/compile committed? We don't
commit it in the client code (ipa-client/compile).
   
   Added 'compile' to .gitignore in second patch
   
Patch 0003: OK
   
   Nothing changed here.
   
   I also remembered the patch naming policy :-) so new patch
   names/numbers are 514,515,516, third revision.
   Thanks. The only complaint I have left is number of whitespace
   errors that git says are in the 515th patch.
 
  Yeah the autogenerated code is not a pretty sight style-wise, do we
  want to run an automatic indenter on it ?
  I was hesitant to do so, but I wouldn't mind adding that, if we
  feel strongly about it.
 
 Let's please not try to correct autogenerated code.
 I'm not tied to this but Simo now thinks it is better to run indenter
 in the generator rule as this will give less problems in actual
 comparison noise that git diff would give.
 
 I'll make sure to talk back to asn1c author to see if we can improve
 its generators upstream.

So given Nathaniel doesn't like to touch autogenerated code I'll leave
it as it is.
I am going to push with the only change being to remove
asn1/config.h.in~ with was added to the second commit by mistake.

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] Fix getkeytab operation

2014-11-20 Thread Nathaniel McCallum
On Thu, 2014-11-20 at 10:47 -0500, Simo Sorce wrote:
 On Thu, 20 Nov 2014 16:47:29 +0200
 Alexander Bokovoy aboko...@redhat.com wrote:
 
  On Thu, 20 Nov 2014, Nathaniel McCallum wrote:
  On Thu, 2014-11-20 at 09:12 -0500, Simo Sorce wrote:
   On Thu, 20 Nov 2014 12:36:45 +0200
   Alexander Bokovoy aboko...@redhat.com wrote:
  
On Wed, 19 Nov 2014, Simo Sorce wrote:
- Original Message -
 From: Alexander Bokovoy aboko...@redhat.com
[...]

 Regarding the patchset itself:

 Patch 0001: fix 'wuld' in the commit message. The rest is
 fine.

Fixed.

 Patch 0002:
  - ticket number is missing in the commit message

Added.

  - perhaps, an instruction how to regenerate asn1 code can be
 made a Makefile target? We don't need to call it ourselves
 but this would simplify things in future

Added make regenerate target to asn1c makefile

  - I'm little uncomfortable how ASN_DEBUG() output goes
 explicitly to stderr but I guess this is something we
 currently cannot override with DS-specific log printing, so
 no big deal right now

ASN_DEBUG() is currently disabled as EMIT_ASN_DEBUG is
undefined, we can later provide a replacement ASN_DEBUG
function to hook debugging, but given the same code is used in
both DS plugins and ipa-getkeytab binary I did not want to
assume anything, and how to wire it up (if we even want to)
should probably be discussed at a later time.

  - any specific need to get asn1/compile committed? We don't
 commit it in the client code (ipa-client/compile).

Added 'compile' to .gitignore in second patch

 Patch 0003: OK

Nothing changed here.

I also remembered the patch naming policy :-) so new patch
names/numbers are 514,515,516, third revision.
Thanks. The only complaint I have left is number of whitespace
errors that git says are in the 515th patch.
  
   Yeah the autogenerated code is not a pretty sight style-wise, do we
   want to run an automatic indenter on it ?
   I was hesitant to do so, but I wouldn't mind adding that, if we
   feel strongly about it.
  
  Let's please not try to correct autogenerated code.
  I'm not tied to this but Simo now thinks it is better to run indenter
  in the generator rule as this will give less problems in actual
  comparison noise that git diff would give.
  
  I'll make sure to talk back to asn1c author to see if we can improve
  its generators upstream.
 
 So given Nathaniel doesn't like to touch autogenerated code I'll leave
 it as it is.
 I am going to push with the only change being to remove
 asn1/config.h.in~ with was added to the second commit by mistake.

LGTM

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


Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-20 Thread Simo Sorce
On Thu, 20 Nov 2014 10:47:35 -0500
Simo Sorce s...@redhat.com wrote:

 On Thu, 20 Nov 2014 16:47:29 +0200
 Alexander Bokovoy aboko...@redhat.com wrote:
 
  On Thu, 20 Nov 2014, Nathaniel McCallum wrote:
  On Thu, 2014-11-20 at 09:12 -0500, Simo Sorce wrote:
   On Thu, 20 Nov 2014 12:36:45 +0200
   Alexander Bokovoy aboko...@redhat.com wrote:
  
On Wed, 19 Nov 2014, Simo Sorce wrote:
- Original Message -
 From: Alexander Bokovoy aboko...@redhat.com
[...]

 Regarding the patchset itself:

 Patch 0001: fix 'wuld' in the commit message. The rest is
 fine.

Fixed.

 Patch 0002:
  - ticket number is missing in the commit message

Added.

  - perhaps, an instruction how to regenerate asn1 code can
 be made a Makefile target? We don't need to call it
 ourselves but this would simplify things in future

Added make regenerate target to asn1c makefile

  - I'm little uncomfortable how ASN_DEBUG() output goes
 explicitly to stderr but I guess this is something we
 currently cannot override with DS-specific log printing, so
 no big deal right now

ASN_DEBUG() is currently disabled as EMIT_ASN_DEBUG is
undefined, we can later provide a replacement ASN_DEBUG
function to hook debugging, but given the same code is used in
both DS plugins and ipa-getkeytab binary I did not want to
assume anything, and how to wire it up (if we even want to)
should probably be discussed at a later time.

  - any specific need to get asn1/compile committed? We don't
 commit it in the client code (ipa-client/compile).

Added 'compile' to .gitignore in second patch

 Patch 0003: OK

Nothing changed here.

I also remembered the patch naming policy :-) so new patch
names/numbers are 514,515,516, third revision.
Thanks. The only complaint I have left is number of whitespace
errors that git says are in the 515th patch.
  
   Yeah the autogenerated code is not a pretty sight style-wise, do
   we want to run an automatic indenter on it ?
   I was hesitant to do so, but I wouldn't mind adding that, if we
   feel strongly about it.
  
  Let's please not try to correct autogenerated code.
  I'm not tied to this but Simo now thinks it is better to run
  indenter in the generator rule as this will give less problems in
  actual comparison noise that git diff would give.
  
  I'll make sure to talk back to asn1c author to see if we can improve
  its generators upstream.
 
 So given Nathaniel doesn't like to touch autogenerated code I'll leave
 it as it is.
 I am going to push with the only change being to remove
 asn1/config.h.in~ with was added to the second commit by mistake.

Pushed to

master:
b170851058d6712442d553ef3d11ecd21b282443
c6afc489a1c9d86fd593bd47c4a8dae6d9a008d2
b1a30bff04fe9763b8b270590ec37084fd19b4e0

ipa-4-1:
f065cec8a58bf4fee0334afdfb63db02f76c1ff7
45ceef14f9ffa5f3abf19088e991f427b7c5bd92
dd3e91639bc3e87b5a95e344b7d190136ad30de0

ipa-4-0:
55578e9cb33924085969102186250ee60c0a9d85
598b54716c6e177a6b5bfdbccf483d28bf40e0b8
aa988311d1b5eefe16eb60c04227900814468e9f

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] Fix getkeytab operation

2014-11-19 Thread Alexander Bokovoy

On Tue, 18 Nov 2014, Simo Sorce wrote:

On Tue, 18 Nov 2014 15:01:15 -0500
Nathaniel McCallum npmccal...@redhat.com wrote:


As I see it, we're setting out a new precedent. All new ASN.1 code
will take this route (which is, indeed, better). So while it is small
now, it won't stay small forever. Being that we are in the business
of routinely handling ASN.1 stuff, this seems to me like a sensible
architecture for the future.


Ok, I think I should have fixed all the issues you brought up.

And my tests still work fine :)

Works fine. However, I'm getting wrong TGT enctype back from the KDC when I
try to obtain TGT with des-cbc-crc key:

[root@master ~]# ipa host-add --force f21test.f21.test
-
Added host f21test.f21.test
-
 Host name: f21test.f21.test
 Principal name: host/f21test.f21.t...@f21.test
 Password: False
 Keytab: False
 Managed by: f21test.f21.test
[root@master ~]# ipa service-add --force afs/f21test

Added service afs/f21t...@f21.test

 Principal: afs/f21t...@f21.test
 Managed by: f21test.f21.test
[root@master ~]# ipa-getkeytab -s `hostname` -p afs/f21test   -k 
/tmp/afs.keytab -e des-cbc-crc:v4 -P
New Principal Password: 
Verify Principal Password: 
Keytab successfully retrieved and stored in: /tmp/afs.keytab

[root@master ~]# klist -kt /tmp/afs.keytab  -K -e
Keytab name: FILE:/tmp/afs.keytab
KVNO Timestamp Principal
 - 
  1 11/19/14 12:13:01 afs/f21t...@f21.test (des-cbc-crc) (0xea1a0b29152cb383)

[root@master ~]# KRB5_TRACE=/dev/stderr KRB5CCNAME=/tmp/afs.ccache kinit -kt 
/tmp/afs.keytab afs/f21test
[28636] 1416392072.862773: Getting initial credentials for afs/f21t...@f21.test
[28636] 1416392072.864408: Looked up etypes in keytab: des-cbc-crc
[28636] 1416392072.864522: Sending request (175 bytes) to F21.TEST
[28636] 1416392072.865127: Sending initial UDP request to dgram 192.168.5.169:88
[28636] 1416392072.866958: Received answer (283 bytes) from dgram 
192.168.5.169:88
[28636] 1416392072.867028: Response was from master KDC
[28636] 1416392072.867088: Received error from KDC: -1765328359/Additional 
pre-authentication required
[28636] 1416392072.867140: Processing preauth types: 136, 19, 2, 133
[28636] 1416392072.867175: Selected etype info: etype des-cbc-crc, salt 
F21.TESTafsf21test, params 
[28636] 1416392072.867193: Received cookie: MIT
[28636] 1416392072.867234: Retrieving afs/f21t...@f21.test from 
FILE:/tmp/afs.keytab (vno 0, enctype des-cbc-crc) with result: 0/Success
[28636] 1416392072.867264: AS key obtained for encrypted timestamp: 
des-cbc-crc/0BE8
[28636] 1416392072.867304: Encrypted timestamp (for 1416392072.867050): plain 
301AA011180F32303134313131393130313433325AA10502030D3AEA, encrypted 
1C567557D395C0639CB417EE90C08CD41E4829D910166D62ACEDCC2168C23BAD8C70DFE4CD533A81
[28636] 1416392072.867331: Preauth module encrypted_timestamp (2) (real) 
returned: 0/Success
[28636] 1416392072.867349: Produced preauth for next request: 133, 2
[28636] 1416392072.867372: Sending request (252 bytes) to F21.TEST
[28636] 1416392072.867416: Sending initial UDP request to dgram 192.168.5.169:88
[28636] 1416392072.946260: Received answer (649 bytes) from dgram 
192.168.5.169:88
[28636] 1416392072.946391: Response was from master KDC
[28636] 1416392072.946485: Processing preauth types: 19
[28636] 1416392072.946542: Selected etype info: etype des-cbc-crc, salt 
F21.TESTafsf21test, params 
[28636] 1416392072.946593: Produced preauth for next request: (empty)
[28636] 1416392072.946626: AS key determined by preauth: des-cbc-crc/0BE8
[28636] 1416392072.946688: Decrypted AS reply; session key is: des-cbc-crc/9B41
[28636] 1416392072.946727: FAST negotiation: available
[28636] 1416392072.946793: Initializing FILE:/tmp/afs.ccache with default princ 
afs/f21t...@f21.test
[28636] 1416392072.947118: Removing afs/f21t...@f21.test - 
krbtgt/f21.t...@f21.test from FILE:/tmp/afs.ccache
[28636] 1416392072.947146: Storing afs/f21t...@f21.test - 
krbtgt/f21.t...@f21.test in FILE:/tmp/afs.ccache
[28636] 1416392072.947187: Storing config in FILE:/tmp/afs.ccache for 
krbtgt/f21.t...@f21.test: fast_avail: yes
[28636] 1416392072.947219: Removing afs/f21t...@f21.test - 
krb5_ccache_conf_data/fast_avail/krbtgt\/F21.TEST\@F21.TEST@X-CACHECONF: from 
FILE:/tmp/afs.ccache
[28636] 1416392072.947240: Storing afs/f21t...@f21.test - 
krb5_ccache_conf_data/fast_avail/krbtgt\/F21.TEST\@F21.TEST@X-CACHECONF: in 
FILE:/tmp/afs.ccache
[28636] 1416392072.947419: Storing config in FILE:/tmp/afs.ccache for 
krbtgt/f21.t...@f21.test: pa_type: 2
[28636] 1416392072.947458: Removing afs/f21t...@f21.test - 
krb5_ccache_conf_data/pa_type/krbtgt\/F21.TEST\@F21.TEST@X-CACHECONF: from 
FILE:/tmp/afs.ccache
[28636] 1416392072.947480: Storing afs/f21t...@f21.test - 
krb5_ccache_conf_data/pa_type/krbtgt\/F21.TEST\@F21.TEST@X-CACHECONF: in 

Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-19 Thread Simo Sorce
On Wed, 19 Nov 2014 12:53:01 +0200
Alexander Bokovoy aboko...@redhat.com wrote:

 On Tue, 18 Nov 2014, Simo Sorce wrote:
 On Tue, 18 Nov 2014 15:01:15 -0500
 Nathaniel McCallum npmccal...@redhat.com wrote:
 
  As I see it, we're setting out a new precedent. All new ASN.1 code
  will take this route (which is, indeed, better). So while it is
  small now, it won't stay small forever. Being that we are in the
  business of routinely handling ASN.1 stuff, this seems to me like
  a sensible architecture for the future.
 
 Ok, I think I should have fixed all the issues you brought up.
 
 And my tests still work fine :)
 Works fine. However, I'm getting wrong TGT enctype back from the KDC
 when I try to obtain TGT with des-cbc-crc key:
 
 [root@master ~]# ipa host-add --force f21test.f21.test
 -
 Added host f21test.f21.test
 -
   Host name: f21test.f21.test
   Principal name: host/f21test.f21.t...@f21.test
   Password: False
   Keytab: False
   Managed by: f21test.f21.test
 [root@master ~]# ipa service-add --force afs/f21test
 
 Added service afs/f21t...@f21.test
 
   Principal: afs/f21t...@f21.test
   Managed by: f21test.f21.test
 [root@master ~]# ipa-getkeytab -s `hostname` -p afs/f21test
 -k /tmp/afs.keytab -e des-cbc-crc:v4 -P New Principal Password: 
 Verify Principal Password: 
 Keytab successfully retrieved and stored in: /tmp/afs.keytab
 [root@master ~]# klist -kt /tmp/afs.keytab  -K -e
 Keytab name: FILE:/tmp/afs.keytab
 KVNO Timestamp Principal
  -
  1 11/19/14
 12:13:01 afs/f21t...@f21.test (des-cbc-crc) (0xea1a0b29152cb383)

The key is des-cbc-crc

 
 [root@master ~]# KRB5_TRACE=/dev/stderr KRB5CCNAME=/tmp/afs.ccache
 kinit -kt /tmp/afs.keytab afs/f21test [28636] 1416392072.862773:
 Getting initial credentials for afs/f21t...@f21.test [28636]
 1416392072.864408: Looked up etypes in keytab: des-cbc-crc [28636]
 1416392072.864522: Sending request (175 bytes) to F21.TEST [28636]
 1416392072.865127: Sending initial UDP request to dgram
 192.168.5.169:88 [28636] 1416392072.866958: Received answer (283
 bytes) from dgram 192.168.5.169:88 [28636] 1416392072.867028:
 Response was from master KDC [28636] 1416392072.867088: Received
 error from KDC: -1765328359/Additional pre-authentication required
 [28636] 1416392072.867140: Processing preauth types: 136, 19, 2, 133
 [28636] 1416392072.867175: Selected etype info: etype des-cbc-crc,
 salt F21.TESTafsf21test, params  [28636] 1416392072.867193:
 Received cookie: MIT [28636] 1416392072.867234: Retrieving
 afs/f21t...@f21.test from FILE:/tmp/afs.keytab (vno 0, enctype
 des-cbc-crc) with result: 0/Success [28636] 1416392072.867264: AS key
 obtained for encrypted timestamp: des-cbc-crc/0BE8 [28636]
 1416392072.867304: Encrypted timestamp (for 1416392072.867050): plain
 301AA011180F32303134313131393130313433325AA10502030D3AEA, encrypted
 1C567557D395C0639CB417EE90C08CD41E4829D910166D62ACEDCC2168C23BAD8C70DFE4CD533A81
 [28636] 1416392072.867331: Preauth module encrypted_timestamp (2)
 (real) returned: 0/Success [28636] 1416392072.867349: Produced
 preauth for next request: 133, 2 [28636] 1416392072.867372: Sending
 request (252 bytes) to F21.TEST [28636] 1416392072.867416: Sending
 initial UDP request to dgram 192.168.5.169:88 [28636]
 1416392072.946260: Received answer (649 bytes) from dgram
 192.168.5.169:88 [28636] 1416392072.946391: Response was from master
 KDC [28636] 1416392072.946485: Processing preauth types: 19 [28636]
 1416392072.946542: Selected etype info: etype des-cbc-crc, salt
 F21.TESTafsf21test, params  [28636] 1416392072.946593: Produced
 preauth for next request: (empty) [28636] 1416392072.946626: AS key
 determined by preauth: des-cbc-crc/0BE8 [28636] 1416392072.946688:
 Decrypted AS reply; session key is: des-cbc-crc/9B41 [28636]
 1416392072.946727: FAST negotiation: available [28636]
 1416392072.946793: Initializing FILE:/tmp/afs.ccache with default
 princ afs/f21t...@f21.test [28636] 1416392072.947118: Removing
 afs/f21t...@f21.test - krbtgt/f21.t...@f21.test from
 FILE:/tmp/afs.ccache [28636] 1416392072.947146: Storing
 afs/f21t...@f21.test - krbtgt/f21.t...@f21.test in
 FILE:/tmp/afs.ccache [28636] 1416392072.947187: Storing config in
 FILE:/tmp/afs.ccache for krbtgt/f21.t...@f21.test: fast_avail: yes
 [28636] 1416392072.947219: Removing afs/f21t...@f21.test -
 krb5_ccache_conf_data/fast_avail/krbtgt\/F21.TEST\@F21.TEST@X-CACHECONF:
 from FILE:/tmp/afs.ccache [28636] 1416392072.947240: Storing
 afs/f21t...@f21.test -
 krb5_ccache_conf_data/fast_avail/krbtgt\/F21.TEST\@F21.TEST@X-CACHECONF:
 in FILE:/tmp/afs.ccache [28636] 1416392072.947419: Storing config in
 FILE:/tmp/afs.ccache for krbtgt/f21.t...@f21.test: pa_type: 2 [28636]
 1416392072.947458: Removing afs/f21t...@f21.test -
 

Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-19 Thread Nathaniel McCallum
On Wed, 2014-11-19 at 13:33 -0500, Simo Sorce wrote:
 - Original Message -
  From: Alexander Bokovoy aboko...@redhat.com
 [...]
 
  Regarding the patchset itself:
  
  Patch 0001: fix 'wuld' in the commit message. The rest is fine.
 
 Fixed.
 
  Patch 0002:
   - ticket number is missing in the commit message
 
 Added.
 
   - perhaps, an instruction how to regenerate asn1 code can be made a
 Makefile target? We don't need to call it ourselves but this would
 simplify things in future
 
 Added make regenerate target to asn1c makefile
 
   - I'm little uncomfortable how ASN_DEBUG() output goes explicitly to
 stderr but I guess this is something we currently cannot override
 with DS-specific log printing, so no big deal right now
 
 ASN_DEBUG() is currently disabled as EMIT_ASN_DEBUG is undefined, we can
 later provide a replacement ASN_DEBUG function to hook debugging, but
 given the same code is used in both DS plugins and ipa-getkeytab binary
 I did not want to assume anything, and how to wire it up (if we even want
 to) should probably be discussed at a later time.
 
   - any specific need to get asn1/compile committed? We don't commit it
 in the client code (ipa-client/compile).
 
 Added 'compile' to .gitignore in second patch
 
  Patch 0003: OK
 
 Nothing changed here.
 
 I also remembered the patch naming policy :-) so new patch names/numbers
 are 514,515,516, third revision.

ACK from me so long as abokovoy has nothing else.

Nathaniel

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


Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-18 Thread Nathaniel McCallum
On Tue, 2014-11-18 at 00:01 -0500, Simo Sorce wrote:
 Hello team,
 
 Recently Alexander opened the following bug:
 https://fedorahosted.org/freeipa/ticket/4718
 
 The problem with this code is that manual ASN.1 encoding is fragile and
 too prone to error, and I found various issues while investigating the
 bug. So I decided to give a shot at replacing the fragile manual code
 with more robust code autogenerated by the asn1c compiler.
 
 While working on replacing the code with the autogenerated one I
 discovered additional encoding issues of which the following ticket
 represent only the most evident:
 https://fedorahosted.org/freeipa/ticket/4728
 
 I found numerous encoding errors which basically made the getkeytab
 control we implemented in both the client and the server not respect
 the encoding we actually defined with ASN.1 notation here:
 http://www.freeipa.org/page/V4/Keytab_Retrieval
 
 While digging and testing replacement functions is also became evident
 that the getkeytab feature was only partially working and that the bug
 in 4718 was not just a minor error, but cannot ever work on existing
 servers as there are compounding bugs that would prevent using the
 getkeytab protocol to ever work if the user specified enctypes via
 ipa-getkeytab instead of just requesting the server's defaults.
 
 Because of this and because it was just too hard *and* useless to try to
 be compatible with existing broken clients and servers the new code
 breaks compatibility for correctness of implementation.
 
 The break in compatibility is mitigated by the fact that ipa-getkeytab
 always falls back to the old setkeytab control in case of error, so
 normal operations will not be disrupted. The only feature that will not
 work if you have a buggy client or a buggy server is the keytab
 retrieval option, as that feature is only available with the new
 control. Given we have only recently introduced CLI and UI to actually
 enable the use of the retrieval option and given the fact 4.x has not
 yet hit major distribution stable releases I think that this patchset
 is acceptable as long as we can land it in 4.1.2 and/or in an
 immediately following patch release (also in 4.0.x possibly) so that it
 can land as a zero day upgrade for Fedora (and an upgrade for Debian).
 
 If you have any questions, please shoot.
 
 This code is fully tested by me on top of master. I think it should
 apply directly on 4.1.2 and 4.0.x with none or minor changes.

Patch 0001:
Typo in the commit message. Otherwise LGTM.

Patch 0002:
I would strongly prefer that generated code live in its own directory
and static library. Then the wrapper around that code should make its
own library and link to the library for the generated code.

Something like:
* asn1/asn1c/libasn1c.a
* asn1/libipaasn1.a

Also, shouldn't the functions in ipa_asn1.[ch] get a prefix? Maybe
ipa_asn1_?

I'd love to see some function documentation in ipa_asn1.h. At the least,
this should cover allocation semantics.

Are there any changes in KeytabModule itself from the previous
implementation? It looks like yes. NewKeys.enctypes for instance used to
be SEQUENCE OF Int16, but is now SEQUENCE OF INTEGER. Isn't the Kerberos
enctype Int32? Why not use this?

Same with GKReply.newkvno (new_kvno), KrbKey.key, KrbKey.salt, etc.

It seems to me like you're trying to resist pulling in the Kerberos
ASN.1 module. If this is the only reason, it seems to me like we'll
probably need it eventually anyway and we can just configure the
compiler to drop the unused symbols. But maybe I'm missing something.

Why does lber.h get added to ipa_krb5.h? Aren't we trying to get rid of
lber with this patch?

Nathaniel






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


Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-18 Thread Simo Sorce
On Tue, 18 Nov 2014 11:23:42 -0500
Nathaniel McCallum npmccal...@redhat.com wrote:

 On Tue, 2014-11-18 at 00:01 -0500, Simo Sorce wrote:
  Hello team,
  
  Recently Alexander opened the following bug:
  https://fedorahosted.org/freeipa/ticket/4718
  
  The problem with this code is that manual ASN.1 encoding is fragile
  and too prone to error, and I found various issues while
  investigating the bug. So I decided to give a shot at replacing the
  fragile manual code with more robust code autogenerated by the
  asn1c compiler.
  
  While working on replacing the code with the autogenerated one I
  discovered additional encoding issues of which the following ticket
  represent only the most evident:
  https://fedorahosted.org/freeipa/ticket/4728
  
  I found numerous encoding errors which basically made the getkeytab
  control we implemented in both the client and the server not respect
  the encoding we actually defined with ASN.1 notation here:
  http://www.freeipa.org/page/V4/Keytab_Retrieval
  
  While digging and testing replacement functions is also became
  evident that the getkeytab feature was only partially working and
  that the bug in 4718 was not just a minor error, but cannot ever
  work on existing servers as there are compounding bugs that would
  prevent using the getkeytab protocol to ever work if the user
  specified enctypes via ipa-getkeytab instead of just requesting the
  server's defaults.
  
  Because of this and because it was just too hard *and* useless to
  try to be compatible with existing broken clients and servers the
  new code breaks compatibility for correctness of implementation.
  
  The break in compatibility is mitigated by the fact that
  ipa-getkeytab always falls back to the old setkeytab control in
  case of error, so normal operations will not be disrupted. The only
  feature that will not work if you have a buggy client or a buggy
  server is the keytab retrieval option, as that feature is only
  available with the new control. Given we have only recently
  introduced CLI and UI to actually enable the use of the retrieval
  option and given the fact 4.x has not yet hit major distribution
  stable releases I think that this patchset is acceptable as long as
  we can land it in 4.1.2 and/or in an immediately following patch
  release (also in 4.0.x possibly) so that it can land as a zero day
  upgrade for Fedora (and an upgrade for Debian).
  
  If you have any questions, please shoot.
  
  This code is fully tested by me on top of master. I think it should
  apply directly on 4.1.2 and 4.0.x with none or minor changes.
 
 Patch 0001:
 Typo in the commit message. Otherwise LGTM.
 
 Patch 0002:
 I would strongly prefer that generated code live in its own directory
 and static library. Then the wrapper around that code should make its
 own library and link to the library for the generated code.
 
 Something like:
 * asn1/asn1c/libasn1c.a
 * asn1/libipaasn1.a

Although I can see the logic, it sounds a little bit extreme to build a
convenience library for a convenience library ... what's the gain ?
The ipa_asn1.c code is intimately tied to the autogenerated code anyway.

 Also, shouldn't the functions in ipa_asn1.[ch] get a prefix? Maybe
 ipa_asn1_?

uhmm maybe we should indeed.
any other opinion ?

 I'd love to see some function documentation in ipa_asn1.h. At the
 least, this should cover allocation semantics.

Yeah, I added a comment or two in the .c file but did not make it
into .h file, I'll fix that.

 Are there any changes in KeytabModule itself from the previous
 implementation? It looks like yes. NewKeys.enctypes for instance used
 to be SEQUENCE OF Int16, but is now SEQUENCE OF INTEGER. Isn't the
 Kerberos enctype Int32? Why not use this?

INTEGER is really all we need.

 Same with GKReply.newkvno (new_kvno), KrbKey.key, KrbKey.salt, etc.
 
 It seems to me like you're trying to resist pulling in the Kerberos
 ASN.1 module. If this is the only reason, it seems to me like we'll
 probably need it eventually anyway and we can just configure the
 compiler to drop the unused symbols.

It would be a lot of code we do not need.
I could import just the Int32 definition, but why ?
INTEGER works fine for our purposes (we use system defined integers so
it is limited to a 'long').

 But maybe I'm missing something.
 
 Why does lber.h get added to ipa_krb5.h? Aren't we trying to get rid
 of lber with this patch?

Because the header file uses struct berval in a function I am not
touching, so it need the include to avoid compile warnings, eventually
we may change things around to improve minor details, but this patch is
blocking for Fedora 21 so I would like to avoid anything that is not a
hard blocker.

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] Fix getkeytab operation

2014-11-18 Thread Nathaniel McCallum
On Tue, 2014-11-18 at 12:40 -0500, Simo Sorce wrote:
 On Tue, 18 Nov 2014 11:23:42 -0500
 Nathaniel McCallum npmccal...@redhat.com wrote:
 
  On Tue, 2014-11-18 at 00:01 -0500, Simo Sorce wrote:
   Hello team,
   
   Recently Alexander opened the following bug:
   https://fedorahosted.org/freeipa/ticket/4718
   
   The problem with this code is that manual ASN.1 encoding is fragile
   and too prone to error, and I found various issues while
   investigating the bug. So I decided to give a shot at replacing the
   fragile manual code with more robust code autogenerated by the
   asn1c compiler.
   
   While working on replacing the code with the autogenerated one I
   discovered additional encoding issues of which the following ticket
   represent only the most evident:
   https://fedorahosted.org/freeipa/ticket/4728
   
   I found numerous encoding errors which basically made the getkeytab
   control we implemented in both the client and the server not respect
   the encoding we actually defined with ASN.1 notation here:
   http://www.freeipa.org/page/V4/Keytab_Retrieval
   
   While digging and testing replacement functions is also became
   evident that the getkeytab feature was only partially working and
   that the bug in 4718 was not just a minor error, but cannot ever
   work on existing servers as there are compounding bugs that would
   prevent using the getkeytab protocol to ever work if the user
   specified enctypes via ipa-getkeytab instead of just requesting the
   server's defaults.
   
   Because of this and because it was just too hard *and* useless to
   try to be compatible with existing broken clients and servers the
   new code breaks compatibility for correctness of implementation.
   
   The break in compatibility is mitigated by the fact that
   ipa-getkeytab always falls back to the old setkeytab control in
   case of error, so normal operations will not be disrupted. The only
   feature that will not work if you have a buggy client or a buggy
   server is the keytab retrieval option, as that feature is only
   available with the new control. Given we have only recently
   introduced CLI and UI to actually enable the use of the retrieval
   option and given the fact 4.x has not yet hit major distribution
   stable releases I think that this patchset is acceptable as long as
   we can land it in 4.1.2 and/or in an immediately following patch
   release (also in 4.0.x possibly) so that it can land as a zero day
   upgrade for Fedora (and an upgrade for Debian).
   
   If you have any questions, please shoot.
   
   This code is fully tested by me on top of master. I think it should
   apply directly on 4.1.2 and 4.0.x with none or minor changes.
  
  Patch 0001:
  Typo in the commit message. Otherwise LGTM.
  
  Patch 0002:
  I would strongly prefer that generated code live in its own directory
  and static library. Then the wrapper around that code should make its
  own library and link to the library for the generated code.
  
  Something like:
  * asn1/asn1c/libasn1c.a
  * asn1/libipaasn1.a
 
 Although I can see the logic, it sounds a little bit extreme to build a
 convenience library for a convenience library ... what's the gain ?
 The ipa_asn1.c code is intimately tied to the autogenerated code anyway.

Moving the files to a new directory and creating a new libtool library
can hardly be called extreme. It would probably take 5 minutes of
work. It probably took you longer to respond to this email. :)

I think it adds a great deal of readability to a new programmer
approaching this code. And that, for me, is worth it.

  Also, shouldn't the functions in ipa_asn1.[ch] get a prefix? Maybe
  ipa_asn1_?
 
 uhmm maybe we should indeed.
 any other opinion ?
 
  I'd love to see some function documentation in ipa_asn1.h. At the
  least, this should cover allocation semantics.
 
 Yeah, I added a comment or two in the .c file but did not make it
 into .h file, I'll fix that.
 
  Are there any changes in KeytabModule itself from the previous
  implementation? It looks like yes. NewKeys.enctypes for instance used
  to be SEQUENCE OF Int16, but is now SEQUENCE OF INTEGER. Isn't the
  Kerberos enctype Int32? Why not use this?
 
 INTEGER is really all we need.
 
  Same with GKReply.newkvno (new_kvno), KrbKey.key, KrbKey.salt, etc.
  
  It seems to me like you're trying to resist pulling in the Kerberos
  ASN.1 module. If this is the only reason, it seems to me like we'll
  probably need it eventually anyway and we can just configure the
  compiler to drop the unused symbols.
 
 It would be a lot of code we do not need.

That would be automatically generated once and probably never touched
again.

 I could import just the Int32 definition, but why ?
 INTEGER works fine for our purposes (we use system defined integers so
 it is limited to a 'long').

It works and is easier, at the expense of readability (and perhaps some
subtle bugs later for the other types). It really just seems odd to me
to 

Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-18 Thread Simo Sorce
On Tue, 18 Nov 2014 14:28:08 -0500
Nathaniel McCallum npmccal...@redhat.com wrote:

 On Tue, 2014-11-18 at 12:40 -0500, Simo Sorce wrote:
  On Tue, 18 Nov 2014 11:23:42 -0500
  Nathaniel McCallum npmccal...@redhat.com wrote:
  
   On Tue, 2014-11-18 at 00:01 -0500, Simo Sorce wrote:
Hello team,

Recently Alexander opened the following bug:
https://fedorahosted.org/freeipa/ticket/4718

The problem with this code is that manual ASN.1 encoding is
fragile and too prone to error, and I found various issues while
investigating the bug. So I decided to give a shot at replacing
the fragile manual code with more robust code autogenerated by
the asn1c compiler.

While working on replacing the code with the autogenerated one I
discovered additional encoding issues of which the following
ticket represent only the most evident:
https://fedorahosted.org/freeipa/ticket/4728

I found numerous encoding errors which basically made the
getkeytab control we implemented in both the client and the
server not respect the encoding we actually defined with ASN.1
notation here: http://www.freeipa.org/page/V4/Keytab_Retrieval

While digging and testing replacement functions is also became
evident that the getkeytab feature was only partially working
and that the bug in 4718 was not just a minor error, but cannot
ever work on existing servers as there are compounding bugs
that would prevent using the getkeytab protocol to ever work if
the user specified enctypes via ipa-getkeytab instead of just
requesting the server's defaults.

Because of this and because it was just too hard *and* useless
to try to be compatible with existing broken clients and
servers the new code breaks compatibility for correctness of
implementation.

The break in compatibility is mitigated by the fact that
ipa-getkeytab always falls back to the old setkeytab control in
case of error, so normal operations will not be disrupted. The
only feature that will not work if you have a buggy client or a
buggy server is the keytab retrieval option, as that feature is
only available with the new control. Given we have only recently
introduced CLI and UI to actually enable the use of the
retrieval option and given the fact 4.x has not yet hit major
distribution stable releases I think that this patchset is
acceptable as long as we can land it in 4.1.2 and/or in an
immediately following patch release (also in 4.0.x possibly) so
that it can land as a zero day upgrade for Fedora (and an
upgrade for Debian).

If you have any questions, please shoot.

This code is fully tested by me on top of master. I think it
should apply directly on 4.1.2 and 4.0.x with none or minor
changes.
   
   Patch 0001:
   Typo in the commit message. Otherwise LGTM.
   
   Patch 0002:
   I would strongly prefer that generated code live in its own
   directory and static library. Then the wrapper around that code
   should make its own library and link to the library for the
   generated code.
   
   Something like:
   * asn1/asn1c/libasn1c.a
   * asn1/libipaasn1.a
  
  Although I can see the logic, it sounds a little bit extreme to
  build a convenience library for a convenience library ... what's
  the gain ? The ipa_asn1.c code is intimately tied to the
  autogenerated code anyway.
 
 Moving the files to a new directory and creating a new libtool library
 can hardly be called extreme. It would probably take 5 minutes of
 work. It probably took you longer to respond to this email. :)
 
 I think it adds a great deal of readability to a new programmer
 approaching this code. And that, for me, is worth it.

Ok, it's not the work that is extreme, it is just the additional
layering that is a bit silly imo, as the asn1/libipaasn1.a would just
be a very thin wrapper, the generated Makefile will probably be bigger
then the wrapper it compiles :)
but ok.

   Also, shouldn't the functions in ipa_asn1.[ch] get a prefix? Maybe
   ipa_asn1_?
  
  uhmm maybe we should indeed.
  any other opinion ?
  
   I'd love to see some function documentation in ipa_asn1.h. At the
   least, this should cover allocation semantics.
  
  Yeah, I added a comment or two in the .c file but did not make it
  into .h file, I'll fix that.
  
   Are there any changes in KeytabModule itself from the previous
   implementation? It looks like yes. NewKeys.enctypes for instance
   used to be SEQUENCE OF Int16, but is now SEQUENCE OF INTEGER.
   Isn't the Kerberos enctype Int32? Why not use this?
  
  INTEGER is really all we need.
  
   Same with GKReply.newkvno (new_kvno), KrbKey.key, KrbKey.salt,
   etc.
   
   It seems to me like you're trying to resist pulling in the
   Kerberos ASN.1 module. If this is the only reason, it seems to me
   like we'll probably need it eventually anyway and we can just
   configure the compiler to 

Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-18 Thread Nathaniel McCallum
On Tue, 2014-11-18 at 14:45 -0500, Simo Sorce wrote:
 On Tue, 18 Nov 2014 14:28:08 -0500
 Nathaniel McCallum npmccal...@redhat.com wrote:
 
  On Tue, 2014-11-18 at 12:40 -0500, Simo Sorce wrote:
   On Tue, 18 Nov 2014 11:23:42 -0500
   Nathaniel McCallum npmccal...@redhat.com wrote:
   
On Tue, 2014-11-18 at 00:01 -0500, Simo Sorce wrote:
 Hello team,
 
 Recently Alexander opened the following bug:
 https://fedorahosted.org/freeipa/ticket/4718
 
 The problem with this code is that manual ASN.1 encoding is
 fragile and too prone to error, and I found various issues while
 investigating the bug. So I decided to give a shot at replacing
 the fragile manual code with more robust code autogenerated by
 the asn1c compiler.
 
 While working on replacing the code with the autogenerated one I
 discovered additional encoding issues of which the following
 ticket represent only the most evident:
 https://fedorahosted.org/freeipa/ticket/4728
 
 I found numerous encoding errors which basically made the
 getkeytab control we implemented in both the client and the
 server not respect the encoding we actually defined with ASN.1
 notation here: http://www.freeipa.org/page/V4/Keytab_Retrieval
 
 While digging and testing replacement functions is also became
 evident that the getkeytab feature was only partially working
 and that the bug in 4718 was not just a minor error, but cannot
 ever work on existing servers as there are compounding bugs
 that would prevent using the getkeytab protocol to ever work if
 the user specified enctypes via ipa-getkeytab instead of just
 requesting the server's defaults.
 
 Because of this and because it was just too hard *and* useless
 to try to be compatible with existing broken clients and
 servers the new code breaks compatibility for correctness of
 implementation.
 
 The break in compatibility is mitigated by the fact that
 ipa-getkeytab always falls back to the old setkeytab control in
 case of error, so normal operations will not be disrupted. The
 only feature that will not work if you have a buggy client or a
 buggy server is the keytab retrieval option, as that feature is
 only available with the new control. Given we have only recently
 introduced CLI and UI to actually enable the use of the
 retrieval option and given the fact 4.x has not yet hit major
 distribution stable releases I think that this patchset is
 acceptable as long as we can land it in 4.1.2 and/or in an
 immediately following patch release (also in 4.0.x possibly) so
 that it can land as a zero day upgrade for Fedora (and an
 upgrade for Debian).
 
 If you have any questions, please shoot.
 
 This code is fully tested by me on top of master. I think it
 should apply directly on 4.1.2 and 4.0.x with none or minor
 changes.

Patch 0001:
Typo in the commit message. Otherwise LGTM.

Patch 0002:
I would strongly prefer that generated code live in its own
directory and static library. Then the wrapper around that code
should make its own library and link to the library for the
generated code.

Something like:
* asn1/asn1c/libasn1c.a
* asn1/libipaasn1.a
   
   Although I can see the logic, it sounds a little bit extreme to
   build a convenience library for a convenience library ... what's
   the gain ? The ipa_asn1.c code is intimately tied to the
   autogenerated code anyway.
  
  Moving the files to a new directory and creating a new libtool library
  can hardly be called extreme. It would probably take 5 minutes of
  work. It probably took you longer to respond to this email. :)
  
  I think it adds a great deal of readability to a new programmer
  approaching this code. And that, for me, is worth it.
 
 Ok, it's not the work that is extreme, it is just the additional
 layering that is a bit silly imo, as the asn1/libipaasn1.a would just
 be a very thin wrapper, the generated Makefile will probably be bigger
 then the wrapper it compiles :)
 but ok.

As I see it, we're setting out a new precedent. All new ASN.1 code will
take this route (which is, indeed, better). So while it is small now, it
won't stay small forever. Being that we are in the business of routinely
handling ASN.1 stuff, this seems to me like a sensible architecture for
the future.

Also, shouldn't the functions in ipa_asn1.[ch] get a prefix? Maybe
ipa_asn1_?
   
   uhmm maybe we should indeed.
   any other opinion ?
   
I'd love to see some function documentation in ipa_asn1.h. At the
least, this should cover allocation semantics.
   
   Yeah, I added a comment or two in the .c file but did not make it
   into .h file, I'll fix that.
   
Are there any changes in KeytabModule itself from the previous
implementation? It looks like yes. NewKeys.enctypes for instance

Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-18 Thread Nathaniel McCallum
On Tue, 2014-11-18 at 17:58 -0500, Simo Sorce wrote:
 On Tue, 18 Nov 2014 15:01:15 -0500
 Nathaniel McCallum npmccal...@redhat.com wrote:
 
  As I see it, we're setting out a new precedent. All new ASN.1 code
  will take this route (which is, indeed, better). So while it is small
  now, it won't stay small forever. Being that we are in the business
  of routinely handling ASN.1 stuff, this seems to me like a sensible
  architecture for the future.
 
 Ok, I think I should have fixed all the issues you brought up.

Still have a typo (wuld) in the commit message of the first patch. :)

 And my tests still work fine :)
 
 Simo.
 


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


Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation

2014-11-18 Thread Simo Sorce
On Tue, 18 Nov 2014 18:00:49 -0500
Nathaniel McCallum npmccal...@redhat.com wrote:

 On Tue, 2014-11-18 at 17:58 -0500, Simo Sorce wrote:
  On Tue, 18 Nov 2014 15:01:15 -0500
  Nathaniel McCallum npmccal...@redhat.com wrote:
  
   As I see it, we're setting out a new precedent. All new ASN.1 code
   will take this route (which is, indeed, better). So while it is
   small now, it won't stay small forever. Being that we are in the
   business of routinely handling ASN.1 stuff, this seems to me like
   a sensible architecture for the future.
  
  Ok, I think I should have fixed all the issues you brought up.
 
 Still have a typo (wuld) in the commit message of the first
 patch. :)

I think I can fix it before pushing if that's the only issue ?

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] Fix getkeytab operation

2014-11-18 Thread Nathaniel McCallum
On Tue, 2014-11-18 at 20:39 -0500, Simo Sorce wrote:
 On Tue, 18 Nov 2014 18:00:49 -0500
 Nathaniel McCallum npmccal...@redhat.com wrote:
 
  On Tue, 2014-11-18 at 17:58 -0500, Simo Sorce wrote:
   On Tue, 18 Nov 2014 15:01:15 -0500
   Nathaniel McCallum npmccal...@redhat.com wrote:
   
As I see it, we're setting out a new precedent. All new ASN.1 code
will take this route (which is, indeed, better). So while it is
small now, it won't stay small forever. Being that we are in the
business of routinely handling ASN.1 stuff, this seems to me like
a sensible architecture for the future.
   
   Ok, I think I should have fixed all the issues you brought up.
  
  Still have a typo (wuld) in the commit message of the first
  patch. :)
 
 I think I can fix it before pushing if that's the only issue ?

I'll do a more thorough review tomorrow. I haven't even looked at the
third patch yet.

Nathaniel

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