You are viewing a plain text version of this content. The canonical link for it is here.
Posted to kerby@directory.apache.org by Emmanuel Lécharny <el...@gmail.com> on 2016/01/03 21:50:15 UTC
AP-REP message
Hi,
another class, another question ;-)
(Note that this class is currently not used in Kerby (it will be at some
point, as this message is sent back to the client as a response to a
KRB-AP-REQ message when the mutual authent AP-Options is set).)
We have a private EncAPRepPart encRepPart; filed defined. What is it
good for ? I suspect it's colliding with the ENC_PART field... if so,
can we get rid of it ?
Thanks !
Re: AP-REP message
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 04/01/16 12:52, Zheng, Kai a écrit :
> I don't think we'd better change the scheme as long as we only claim the right things in the right release.
Ok, that's fine with me.
I have committed the updated AP-REP/AP-REQ classes with some added
Javadoc. We can close this (long) thread !
Thanks a lot for all the explainations
RE: AP-REP message
Posted by "Zheng, Kai" <ka...@intel.com>.
I don't think we'd better change the scheme as long as we only claim the right things in the right release.
Regards,
Kai
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Monday, January 04, 2016 7:03 PM
To: kerby@directory.apache.org
Subject: Re: AP-REP message
Le 04/01/16 11:40, Zheng, Kai a écrit :
> Hi Emmanuel,
>
> I understand your review comments and actually they're very insightful. I may be a little nervous but I did have to convey the current situation the codes in so that you can aware it why I would insist on some points for myself. Some of the codes are still initial and lacking something, that does not mean we shouldn't the project. As you may understand, a project may be released out with some features still in its experimental status unless the feature itself isn't claimed to be available. Kerberos contains quite a few extensions and updates since 4210, as you may be noted, we're incrementally implementing them one by one, and some of them may come across some RCs or even formal releases. So all in all, please don't be surprised when you see still immature codes when you're doing the great review. Thanks.
No worry at all. I see no reason to block a release because there is something missing (we do that all the time...).
I just wanted to stress out the fact that it's important to convey teh right message to our users. This is a disucssion we must have asside this thread : what version scheme should we use ?
Re: AP-REP message
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 04/01/16 11:40, Zheng, Kai a écrit :
> Hi Emmanuel,
>
> I understand your review comments and actually they're very insightful. I may be a little nervous but I did have to convey the current situation the codes in so that you can aware it why I would insist on some points for myself. Some of the codes are still initial and lacking something, that does not mean we shouldn't the project. As you may understand, a project may be released out with some features still in its experimental status unless the feature itself isn't claimed to be available. Kerberos contains quite a few extensions and updates since 4210, as you may be noted, we're incrementally implementing them one by one, and some of them may come across some RCs or even formal releases. So all in all, please don't be surprised when you see still immature codes when you're doing the great review. Thanks.
No worry at all. I see no reason to block a release because there is
something missing (we do that all the time...).
I just wanted to stress out the fact that it's important to convey teh
right message to our users. This is a disucssion we must have asside
this thread : what version scheme should we use ?
RE: AP-REP message
Posted by "Zheng, Kai" <ka...@intel.com>.
Hi Emmanuel,
I understand your review comments and actually they're very insightful. I may be a little nervous but I did have to convey the current situation the codes in so that you can aware it why I would insist on some points for myself. Some of the codes are still initial and lacking something, that does not mean we shouldn't the project. As you may understand, a project may be released out with some features still in its experimental status unless the feature itself isn't claimed to be available. Kerberos contains quite a few extensions and updates since 4210, as you may be noted, we're incrementally implementing them one by one, and some of them may come across some RCs or even formal releases. So all in all, please don't be surprised when you see still immature codes when you're doing the great review. Thanks.
While you're reviewing, I would try to address your questions in timely manner because not only we should, but also I think it's important for the project's long term, though it's not so exciting as writing something new.
Regards,
Kai
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Monday, January 04, 2016 5:51 PM
To: kerby@directory.apache.org
Subject: Re: AP-REP message
Le 04/01/16 08:39, Zheng, Kai a écrit :
> I know and agree with the point you made here. What I would say is that, our Kerberos implementation codes are still lacking many things and to be evolved fast. We may see many classes, variables, methods and codes are not referenced in the existing codes, it's normal because we have so much to fill yet. Given you have reviewed quite a few of the codes, I guess you might understand that why we define and write those classes or methods not used yet, because in many times we follow a RFC spec, or a fixed pattern and make up many of them, either for completeness, for the future we'll use them at all, or just for our customers they need the library to do something.
I'm fine with that. It's just that as there is no documentation in the code, it's hard to know where it's coming from, and what it might be good for in the near future. A simple Javadoc saying 'not used atm, will be in the near future', or 'part of a RFC draft http://blah' would help.
>
> Decrypted result in Kerberos is surely to be passed elsewhere and handled in various procedures or components, considering it often contains many information. We just can't do all the logics in a single place because that would result in a very large function or class. I guess the codes you noted are still very initial for now, which means we have many things to fill in the places. Sorry I haven't checked the codes you mentioned, but I would if our Kerby codes are going to be close to the final status with all the available functionalities existing in MIT Kerberos already fine implemented.
We are talking about releasing RC2, and we already have released a RC1 (Release Candidate) ;-) So maybe it's premature to call it a Release Candidate, and we should name them Milestone ?
> Obviously, we're not. Sadly, we won't in a fair term future. Otherwise it's unfair, Kerberos has evolved past more than twenty years, we just can't catch up with the mainstream simply in one or two years.
The simple fact that our Kerberos Implementation at ApacheDS haven't reach a full coverage of the Kerberos specifications in 10 years is just making your point ;-)
Sorry if my mails make you think I find it wrong, I'm just trying to point fingers on parts that, in my opinion, might need some improvements. If I'm lucky, I find bugs, or areas where more work need to be done, and that's the purpose of this review.
It's also for me a way to get involved in this code...
Thanks Kai ! I'll keep the methods at the moment.
Re: AP-REP message
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 04/01/16 08:39, Zheng, Kai a écrit :
> I know and agree with the point you made here. What I would say is that, our Kerberos implementation codes are still lacking many things and to be evolved fast. We may see many classes, variables, methods and codes are not referenced in the existing codes, it's normal because we have so much to fill yet. Given you have reviewed quite a few of the codes, I guess you might understand that why we define and write those classes or methods not used yet, because in many times we follow a RFC spec, or a fixed pattern and make up many of them, either for completeness, for the future we'll use them at all, or just for our customers they need the library to do something.
I'm fine with that. It's just that as there is no documentation in the
code, it's hard to know where it's coming from, and what it might be
good for in the near future. A simple Javadoc saying 'not used atm, will
be in the near future', or 'part of a RFC draft http://blah' would help.
>
> Decrypted result in Kerberos is surely to be passed elsewhere and handled in various procedures or components, considering it often contains many information. We just can't do all the logics in a single place because that would result in a very large function or class. I guess the codes you noted are still very initial for now, which means we have many things to fill in the places. Sorry I haven't checked the codes you mentioned, but I would if our Kerby codes are going to be close to the final status with all the available functionalities existing in MIT Kerberos already fine implemented.
We are talking about releasing RC2, and we already have released a RC1
(Release Candidate) ;-) So maybe it's premature to call it a Release
Candidate, and we should name them Milestone ?
> Obviously, we're not. Sadly, we won't in a fair term future. Otherwise it's unfair, Kerberos has evolved past more than twenty years, we just can't catch up with the mainstream simply in one or two years.
The simple fact that our Kerberos Implementation at ApacheDS haven't
reach a full coverage of the Kerberos specifications in 10 years is just
making your point ;-)
Sorry if my mails make you think I find it wrong, I'm just trying to
point fingers on parts that, in my opinion, might need some
improvements. If I'm lucky, I find bugs, or areas where more work need
to be done, and that's the purpose of this review.
It's also for me a way to get involved in this code...
Thanks Kai ! I'll keep the methods at the moment.
RE: AP-REP message
Posted by "Zheng, Kai" <ka...@intel.com>.
I know and agree with the point you made here. What I would say is that, our Kerberos implementation codes are still lacking many things and to be evolved fast. We may see many classes, variables, methods and codes are not referenced in the existing codes, it's normal because we have so much to fill yet. Given you have reviewed quite a few of the codes, I guess you might understand that why we define and write those classes or methods not used yet, because in many times we follow a RFC spec, or a fixed pattern and make up many of them, either for completeness, for the future we'll use them at all, or just for our customers they need the library to do something.
Decrypted result in Kerberos is surely to be passed elsewhere and handled in various procedures or components, considering it often contains many information. We just can't do all the logics in a single place because that would result in a very large function or class. I guess the codes you noted are still very initial for now, which means we have many things to fill in the places. Sorry I haven't checked the codes you mentioned, but I would if our Kerby codes are going to be close to the final status with all the available functionalities existing in MIT Kerberos already fine implemented. Obviously, we're not. Sadly, we won't in a fair term future. Otherwise it's unfair, Kerberos has evolved past more than twenty years, we just can't catch up with the mainstream simply in one or two years.
Please pardon if what I said is over too much. Thanks!
Regards,
Kai
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Monday, January 04, 2016 2:38 PM
To: kerby@directory.apache.org
Subject: Re: AP-REP message
Le 04/01/16 06:12, Zheng, Kai a écrit :
> It's good to look at the life cycle of these decrypted objects but I don't think it will help a lot if we would do otherwise. Generally decrypting the part and using the decrypted result are not the same place.
Here is where the encrpted data is decrypetd and used, in KdcRequest :
/**
* Get the armor key.
*
* @throws org.apache.kerby.kerberos.kerb.KrbException e
* @param fastArmor krb fast armor
*/
private void armorApRequest(KrbFastArmor fastArmor) throws KrbException {
if (fastArmor.getArmorType() == ArmorType.ARMOR_AP_REQUEST) {
ApReq apReq = KrbCodec.decode(fastArmor.getArmorValue(),
ApReq.class);
...
Authenticator authenticator = EncryptionUtil.unseal(apReq.getEncryptedAuthenticator(),
encKey, KeyUsage.AP_REQ_AUTH, Authenticator.class);
EncryptionKey armorKey =
FastUtil.cf2(authenticator.getSubKey(), "subkeyarmor",
encKey, "ticketarmor");
setArmorKey(armorKey);
}
}
or in TgsRequest :
public void verifyAuthenticator(PaDataEntry paDataEntry) throws KrbException {
ApReq apReq = KrbCodec.decode(paDataEntry.getPaDataValue(),
ApReq.class);
...
Authenticator authenticator =
EncryptionUtil.unseal(apReq.getEncryptedAuthenticator(),
encKey, KeyUsage.TGS_REQ_AUTH, Authenticator.class);
...
In both case, we get the encrypted data, we decypt it locally, put the result in a local variable, which is immediately used and discarded. Its quite what I'm expecting to see, btw. At no point in the whole kerby code we use the decrypted value, which is by the way not set in the encapsulating data structure.
At this point, I can see why it has been added in the structure - in the spirit that it may be used in a different place in the code than where it has been decrypted -. But actually, this is not the case.
If you feel like it's better to keep those fields in the classes, so be it. I'm just afraid we keep them for the sake of keeping them, not respecting the YAGNI principle
(https://en.wikipedia.org/wiki/You_aren't_gonna_need_it)
Your call !
Re: AP-REP message
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 04/01/16 06:12, Zheng, Kai a écrit :
> It's good to look at the life cycle of these decrypted objects but I don't think it will help a lot if we would do otherwise. Generally decrypting the part and using the decrypted result are not the same place.
Here is where the encrpted data is decrypetd and used, in KdcRequest :
/**
* Get the armor key.
*
* @throws org.apache.kerby.kerberos.kerb.KrbException e
* @param fastArmor krb fast armor
*/
private void armorApRequest(KrbFastArmor fastArmor) throws
KrbException {
if (fastArmor.getArmorType() == ArmorType.ARMOR_AP_REQUEST) {
ApReq apReq = KrbCodec.decode(fastArmor.getArmorValue(),
ApReq.class);
...
Authenticator authenticator =
EncryptionUtil.unseal(apReq.getEncryptedAuthenticator(),
encKey, KeyUsage.AP_REQ_AUTH, Authenticator.class);
EncryptionKey armorKey =
FastUtil.cf2(authenticator.getSubKey(), "subkeyarmor",
encKey, "ticketarmor");
setArmorKey(armorKey);
}
}
or in TgsRequest :
public void verifyAuthenticator(PaDataEntry paDataEntry) throws
KrbException {
ApReq apReq = KrbCodec.decode(paDataEntry.getPaDataValue(),
ApReq.class);
...
Authenticator authenticator =
EncryptionUtil.unseal(apReq.getEncryptedAuthenticator(),
encKey, KeyUsage.TGS_REQ_AUTH, Authenticator.class);
...
In both case, we get the encrypted data, we decypt it locally, put the
result in a local variable, which is immediately used and discarded. Its
quite what I'm expecting to see, btw. At no point in the whole kerby
code we use the decrypted value, which is by the way not set in the
encapsulating data structure.
At this point, I can see why it has been added in the structure - in the
spirit that it may be used in a different place in the code than where
it has been decrypted -. But actually, this is not the case.
If you feel like it's better to keep those fields in the classes, so be
it. I'm just afraid we keep them for the sake of keeping them, not
respecting the YAGNI principle
(https://en.wikipedia.org/wiki/You_aren't_gonna_need_it)
Your call !
RE: AP-REP message
Posted by "Zheng, Kai" <ka...@intel.com>.
It's good to look at the life cycle of these decrypted objects but I don't think it will help a lot if we would do otherwise. Generally decrypting the part and using the decrypted result are not the same place. The decrypted info will be used and passed to elsewhere. We need a place to hold the result so keeping it in the relevant class is a natural place. The whole object will be end of life after the request is processed, as it's per request.
Regards,
Kai
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Monday, January 04, 2016 5:39 AM
To: kerby@directory.apache.org
Subject: Re: AP-REP message
Le 03/01/16 22:30, Emmanuel Lécharny a écrit :
> Le 03/01/16 21:50, Emmanuel Lécharny a écrit :
>> Hi,
>>
>> another class, another question ;-)
>>
>> (Note that this class is currently not used in Kerby (it will be at
>> some point, as this message is sent back to the client as a response
>> to a KRB-AP-REQ message when the mutual authent AP-Options is set).)
>>
>> We have a private EncAPRepPart encRepPart; filed defined. What is it
>> good for ? I suspect it's colliding with the ENC_PART field... if so,
>> can we get rid of it ?
> s/filed/field/
>
> Otherwise, same question for the AP-REQ message, field authenticator,
> which sounds spurious to me.
>
Looking a bit forward in teh code, I think (AFAOU) that those two fields are used to store the decrypted version fo the field. And it sounds like a bad idea ! The Authenticator is decrypted elsewhere, and immediately used, then discarded. I don't think it's reasonnable to keep the decrypted version in a data structure for a long time (here, as long as the message instance will last.
Re: AP-REP message
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 03/01/16 22:30, Emmanuel Lécharny a écrit :
> Le 03/01/16 21:50, Emmanuel Lécharny a écrit :
>> Hi,
>>
>> another class, another question ;-)
>>
>> (Note that this class is currently not used in Kerby (it will be at some
>> point, as this message is sent back to the client as a response to a
>> KRB-AP-REQ message when the mutual authent AP-Options is set).)
>>
>> We have a private EncAPRepPart encRepPart; filed defined. What is it
>> good for ? I suspect it's colliding with the ENC_PART field... if so,
>> can we get rid of it ?
> s/filed/field/
>
> Otherwise, same question for the AP-REQ message, field authenticator,
> which sounds spurious to me.
>
Looking a bit forward in teh code, I think (AFAOU) that those two fields
are used to store the decrypted version fo the field. And it sounds like
a bad idea ! The Authenticator is decrypted elsewhere, and immediately
used, then discarded. I don't think it's reasonnable to keep the
decrypted version in a data structure for a long time (here, as long as
the message instance will last.
Re: AP-REP message
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 03/01/16 21:50, Emmanuel Lécharny a écrit :
> Hi,
>
> another class, another question ;-)
>
> (Note that this class is currently not used in Kerby (it will be at some
> point, as this message is sent back to the client as a response to a
> KRB-AP-REQ message when the mutual authent AP-Options is set).)
>
> We have a private EncAPRepPart encRepPart; filed defined. What is it
> good for ? I suspect it's colliding with the ENC_PART field... if so,
> can we get rid of it ?
s/filed/field/
Otherwise, same question for the AP-REQ message, field authenticator,
which sounds spurious to me.