directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yan, Yan A" <yan.a....@intel.com>
Subject RE: [kerby] ASN1 Quick review
Date Thu, 24 Dec 2015 01:12:39 GMT
Thanks for Emmanuel and Kai's discussion.
I'm wondering if any contributor would help with such.
I'd like to help with the issues, and by the way be familiar with the core part codes.

Best regards,
Yan


-----Original Message-----
From: Zheng, Kai [mailto:kai.zheng@intel.com] 
Sent: Thursday, December 24, 2015 8:31 AM
To: Apache Directory Developers List <dev@directory.apache.org>; kerby@directory.apache.org
Subject: RE: [kerby] ASN1 Quick review

Thanks Emmanuel for the review and great comments! The questions are hard but fortunately
I'm still kept in the loop so my pleasure to address them. My comments are embedded and marked
by [Kai].

-----Original Message-----
From: Emmanuel L├ęcharny [mailto:elecharny@gmail.com]
Sent: Wednesday, December 23, 2015 8:54 PM
To: Apache Directory Developers List <dev@directory.apache.org>
Subject: [kerby] ASN1 Quick review

Hi band,

I'm having a quick look at the kerby-asn1 code, as I wanted to paly around the idea of porting
the LDAP codec to use this piece of nice code. AFAIU, when you want to declare an object that
can be encoded or decoded, you have to extend the correct Asn1Object child. Looking at the
Ticket object, here is what I see :

[Kai] Glad you want a try. This is also something I wish to help with as discussed before,
but am not able to do it immediately because of bandwidth. Sorry for that. I certainly would
and will also be able to try to provide any help if needed.

...
import static org.apache.kerby.kerberos.kerb.type.ticket.Ticket.MyEnum.*;
...
public class Ticket extends KrbAppSequenceType {
    public static final int TKT_KVNO = KrbConstant.KRB_V5;
    public static final int TAG = 1;

    protected enum MyEnum implements EnumType {
        TKT_VNO,
        REALM,
        SNAME,
        ENC_PART;

        @Override
        public int getValue() {
            return ordinal();
        }

        @Override
        public String getName() {
            return name();
        }
    }

    static Asn1FieldInfo[] fieldInfos = new Asn1FieldInfo[] {
            new ExplicitField(TKT_VNO, 0, Asn1Integer.class),
            new ExplicitField(REALM, 1, KerberosString.class),
            new ExplicitField(SNAME, 2, PrincipalName.class),
            new ExplicitField(ENC_PART, 3, EncryptedData.class)
    };

I like the idea of defining the fields this way, except that I would suggest some slight modifications
:

- get read of the import ...Ticket.MyEnum.*; [Kai] Hmmm, I guess you mean "get rid of", good
point. It's like this because, initially we didn't use enum, but int constants. And some weeks
ago when we want to dump user defined type objects like this with meaningful field info, we
switched to use enum because it can give a friendly name. We were in a hurry at that time
and wanted to do it as less effort as possible, thus it's like this now: the enum constant
is rather like int constant and used as before.

- make the enum private (there is no reason we wuld like to expose it anyway) [Kai] Well,
actually there are some reasons to make it protected and disciplined exposed. Such user defined
types can be extended and these fields may be accessed there. Ref. child classes of ContentInfo
(also some other examples I remembered when defining protected int constants).

- name it something more meaningful, like TicketField inseatd if MyEnum [Kai] Right agree.
Again it was like this because it's out as a simple pattern in a whole project scope replacement.

- use it with the enum name, like in new ExplicitField(TicketField.TKT_VNO, 0, Asn1Integer.class)
[Kai] Agree.

<side note>
I find the "import static xxx.*;" atrocious, most of the time. It makes it barely readable
: you have to go to the top of your file to actually
*know* where the constant comes from... That may seems of for small files, but otherwise...
I accept it for Asserts, because it's really clear, in a test context - pretty much like annotations.
</side note>
[Kai] I thought you may be right, but ...

Here is what I come with :

public class Ticket extends KrbAppSequenceType {
    public static final int TKT_KVNO = KrbConstant.KRB_V5;
    public static final int TAG = 1;

    private enum TicketField implements EnumType {
        TKT_VNO,
        REALM,
        SNAME,
        ENC_PART;

        @Override
        public int getValue() {
            return ordinal();
        }

        @Override
        public String getName() {
            return name();
        }
    }

    static Asn1FieldInfo[] fieldInfos = new Asn1FieldInfo[] {
            new ExplicitField(TicketField.TKT_VNO, 0, Asn1Integer.class),
            new ExplicitField(TicketField.REALM, 1, KerberosString.class),
            new ExplicitField(TicketField.SNAME, 2, PrincipalName.class),
            new ExplicitField(TicketField.ENC_PART, 3, EncryptedData.class)
    };

(note that it's just a suggestion at this point...) [Kai] They're very good suggestions and
should become true, though a tiresome work because there are so many user defined types now.
I'm wondering if any contributor would help with such. 

Now, let's foduc on the fields declarations. Here, we create ExplicitFields, passing a Xxx.class
as the third parameter, and a number as the second parameter.

First of all, as the enum being used has an implicit ordering, can't we simply avoid passing
those numbers ? There is already an ExplicitField constructor that takes only 2 parameters,
deducing the number from teh enum...
[Kai] Yeah sure we can just use the 2 parameters constructor for this type. You're good at
finding this as the example. We relied on the enum order value mostly but this one and possible
many slipped out.

Second, why don't we write things like :


            new ExplicitField(TicketField.TKT_VNO, 0, new Asn1Integer()),

instead of passing a class ? Bottom line, the instance will be created the same way. I don't
think it will make any difference in term of performances, and as all the object *must* extends
the Asn1Object (or implement the Asn1Type), this should be good enough.
[Kai] Well, let the framework determine when to create the field instances would be most flexible.
We may want to be lazy on creating them and create them on demand; we may need to avoid creating
them in decoding because they're nulls; in the framework codes there're some places that uses
(value == null) to check some conditions. For Any, the actual value type can only be set by
applications in specific contexts.

wdyt ?

Mime
View raw message