Re: code review request: 6960894: Better AS-REQ creation and processing
Webrev updated: http://cr.openjdk.java.net/~weijun/6960894/webrev.01/ Changes made: KrbAsReqBuilder.java: 1. state and state checks 2. clear() - destroy() KrbAsReq.java: split decrypt() to decryptUsingKeys() and decryptUsingPassword() KrbKdcReq - KdcComm: The internal class KdcCommunication name is retained, Comments inline. On 07/31/2010 08:01 AM, Valerie (Yu-Ching) Peng wrote: Hi, Max, KrbAsRep.java = 1) the javadoc of its decrypt method only documents the first two arguments which seems incomplete. You meant to emphasize that there two are user-provided? Maybe you can just enhance the method description. In addition, only one of the keys and password arguments is used. Would it be clearer to separate this into two methods, one uses keys and the other uses password? Now in two methods. = 2) one thing that I find somewhat confusing is the values of creds. The old model sets all of its fields in the constructor. With the new model, creds is null until decrypt(...) is called. I add some comment to the field. KrbKdcReq.java = 1) for the if-block between line 229 and 232, is it possible for the KRB_ERR_RESPONSE_TOO_BIG when 'useTCP' is true? And don't you have to check ibuf again after line 231? The error should only happens when using UDP. RFC 4120 says: KRB_ERR_RESPONSE_TOO_BIG 52 Response too big for UDP; retry with TCP The only error handled in KrbKdcReq is KRB_ERR_RESPONSE_TOO_BIG error, which I treat as a communication-level error. Other errors are handled in a higher level. = 2) I am open for a name change since the current naming seems to imply an inheritance relationship which you've changed. I thought the name change would make an ugly webrev. It seems not. KrbAsReqBuilder.java = 1) line 72 one and only is non-null may be clearer as one of them must be null. I mean that one of them MUST be null and the other one MUST NOT be null. = 2) keys(..) and pass(..) are initialization methods which must be called before getKeys(), right? Can you rename them so it's clearer? There is no checking in getKeys() and if called out of sequence, it looks to me that it'll error out w/ NPE since eType is still null. There are also methods like target(), options(), addresses() that all must be called before action(). I've added an internal state now. Thanks Max Thanks, Valerie On 07/21/10 12:32, Valerie (Yu-Ching) Peng wrote: On 06/13/10 08:02, Weijun Wang wrote: Hi Valerie and Andrew Please review the following webrev: http://cr.openjdk.java.net/~weijun/6960894/webrev.00 The major enhancement is KrbAsReqBuilder which generates AS-REQ, sends it, parses any response, and returns a Credentials object. The other big change is KrbKdcReq, it's no longer base class for KrbAsReq and KrbTgsReq, but mainly a vehicle for both kinds of KDC-REQ messages. Maybe it needs a name change? Most other changes are about removing duplicate lines. Thanks Max Begin forwarded message: *Change Request ID*: 6960894 *Synopsis*: Better AS-REQ creation and processing === *Description* The current AS-REQ creation and processing implementation: 1. spread into multiple source files and have duplicate codes 2. cannot deal with PA-DATA in AS-REP 3. only use a single salt, and write it into PrincipalName permanently 4. generate too many secret keys and have no consistent way to clear them 5. does not handle the preferences of PA-ETYPE-INFO2, PA-ETYPE-INFO correctly
Re: code review request: 6960894: Better AS-REQ creation and processing
Hi, Max, KrbAsRep.java = 1) the javadoc of its decrypt method only documents the first two arguments which seems incomplete. You meant to emphasize that there two are user-provided? Maybe you can just enhance the method description. In addition, only one of the keys and password arguments is used. Would it be clearer to separate this into two methods, one uses keys and the other uses password? = 2) one thing that I find somewhat confusing is the values of creds. The old model sets all of its fields in the constructor. With the new model, creds is null until decrypt(...) is called. KrbKdcReq.java = 1) for the if-block between line 229 and 232, is it possible for the KRB_ERR_RESPONSE_TOO_BIG when 'useTCP' is true? And don't you have to check ibuf again after line 231? = 2) I am open for a name change since the current naming seems to imply an inheritance relationship which you've changed. KrbAsReqBuilder.java = 1) line 72 one and only is non-null may be clearer as one of them must be null. = 2) keys(..) and pass(..) are initialization methods which must be called before getKeys(), right? Can you rename them so it's clearer? There is no checking in getKeys() and if called out of sequence, it looks to me that it'll error out w/ NPE since eType is still null. Thanks, Valerie On 07/21/10 12:32, Valerie (Yu-Ching) Peng wrote: On 06/13/10 08:02, Weijun Wang wrote: Hi Valerie and Andrew Please review the following webrev: http://cr.openjdk.java.net/~weijun/6960894/webrev.00 The major enhancement is KrbAsReqBuilder which generates AS-REQ, sends it, parses any response, and returns a Credentials object. The other big change is KrbKdcReq, it's no longer base class for KrbAsReq and KrbTgsReq, but mainly a vehicle for both kinds of KDC-REQ messages. Maybe it needs a name change? Most other changes are about removing duplicate lines. Thanks Max Begin forwarded message: *Change Request ID*: 6960894 *Synopsis*: Better AS-REQ creation and processing === *Description* The current AS-REQ creation and processing implementation: 1. spread into multiple source files and have duplicate codes 2. cannot deal with PA-DATA in AS-REP 3. only use a single salt, and write it into PrincipalName permanently 4. generate too many secret keys and have no consistent way to clear them 5. does not handle the preferences of PA-ETYPE-INFO2, PA-ETYPE-INFO correctly
code review request: 6960894: Better AS-REQ creation and processing
Hi Valerie and Andrew Please review the following webrev: http://cr.openjdk.java.net/~weijun/6960894/webrev.00 The major enhancement is KrbAsReqBuilder which generates AS-REQ, sends it, parses any response, and returns a Credentials object. The other big change is KrbKdcReq, it's no longer base class for KrbAsReq and KrbTgsReq, but mainly a vehicle for both kinds of KDC-REQ messages. Maybe it needs a name change? Most other changes are about removing duplicate lines. Thanks Max Begin forwarded message: *Change Request ID*: 6960894 *Synopsis*: Better AS-REQ creation and processing === *Description* The current AS-REQ creation and processing implementation: 1. spread into multiple source files and have duplicate codes 2. cannot deal with PA-DATA in AS-REP 3. only use a single salt, and write it into PrincipalName permanently 4. generate too many secret keys and have no consistent way to clear them 5. does not handle the preferences of PA-ETYPE-INFO2, PA-ETYPE-INFO correctly
