Re: Fwd: About yubico java hacking
Simon Josefsson
>> Hi Arne!
>>
>> I'm not sure which forum post you refer to, could you give me a link?
>
> Ah sorry, not the forum, I meant my last post at the group discussion here:
> http://groups.google.com/group/yubico-devel/browse_thread/thread/b2cb22babd3f6701/1da74d80e40a8a4f
> I attached it also to this mail.
I hadn't received that. :-(
Seems to be some problem between google and my mail server, sigh. I'm
cc'ing the list now to bring the discussion back there.
>> We'd prefer to avoid forks if possible, mostly in order to reduce
>> duplicated work. But if you think you can proceed faster by doing so,
>> by all means do it, the license permits it.. We do like to avoid
>> external dependencies too. The core library should be pretty minimal,
>> so hopefully it could be changed to serve your needs.
>
> You are using it as it is in the project yubikey-server-j and perhaps
> other places, thats why I thought changing the signature as I proposed
> would be dificult in current project.
>
> Also with a separate project it would perhaps be more apropiate to
> experiment with controversial stuff as application independent cook
> book validation method(s).
>
> One always has the possibility at a later stage to merge I if one
> thinks new stuff is good out of alpha. Or perhaps one say ok to live
> with two flavors...
>
> Well, what do you think?
I believe the yubikey-server-j, and any other projects, can stick with
the current 'yubico-j' version until those other projects have been
updated to conform with the changes in 'yubico-j'. Thus, I think we can
apply your ideas.
Let's try to do the right thing from the start, and then sort out
breakage in the bad things later on, rather than trying to design for
bad things initially.
>> Regarding caps lock; yes, I believe you are correct. Please check in a
>> patch to change the 0 to 1. :)
>
> So, guess getCleanCounter method also is in error then?
Yes, it seems that way.
> Could you provide test vectors with cap flag set for unit tests? I
> think for ensuring quality test vectors should come from the hardcore
> experts!
Good idea, I don't have one readily available right now but will try to
do this.
Returning to your list e-mail:
> 1.1
>
> Token get methods now returns references to internal arrays, consider
> returning copies to
> protect state. If optimization is a consern consider the idiom:
>
> byte[] Token.getSomething(){ return getSomething(null); }
> byte[] Token.getSomething(byte[] val){ ... }
>
> If val==null an array is allocated and assigned if not given array is
> assigned and returned.
I'm not a java expert... does java return arrays by reference? Can't we
get it to copy the array? Returning references to internal arrays seems
like a bad idea to me.
Alternatively, to preserve library compatibility, if we make this
change, can't we retain the old functions anyway, for compatibility?
They shouldn't conflict with the new functions, would they? They have
different signatures.
> 1.2
>
> High performance servers handling heavy traffic may utilize that
> pof.parse can accept a
> Token to initialize to reduce the number Token creations. If so, the
> todos:
>
> a:
>
> All new of arrays away from parse code, do in declaration.
>
> b:
>
> Parse code moved away from constructor to new public set(byte[] b),
> current constructor
> will call this of course.
>
> c:
>
> Provide empty constructor so clients can make uninitialized ones
> (perhaps to keep in a pool).
>
> d:
>
> Provide
>
> Token Pof.parse(String block, byte[] key, Token t)
> {
> t=(t==null) ? new Token() : t;
> byte[] decoded = Modhex.decode(block);
> byte[] b = decrypt(key, decoded);
> t.set(b);
> return t;
> }
>
> in additon to current message:
>
> Pof.parse(String block, byte[] key){ return parse(block, key,
> null); }
I'm not sure I follow this, would this only be for optimization? If so,
I think we should wait until we have real performance measurements to
back up the claim that this is a bottle-neck. Premature optimization is
often bad.
If this also leads to cleaner code, I would be in favor of this, but I
don't know if that will be the case.
> 1.3
>
> Validation code probably would like get methods to get unsigned
> values delivered as
> ints for the counters and time stamp values.
>
> Think okey to keep internal representation as bytes:
>
> a. Keep core unit tests.
> b. Logic bit operation to convert is *fast*.
> c. In production likely to be invoked only once for each OTP
> validated.
>
> What to name these, we kind of used names for raw access... probably
> not to
> late to change the API (or is it???), suggestion:
>
> Rename current getSomething to getSomethingByte(s)
I like this.
> By the way, is there a need to review terms over projects? You tell
> me, but if so,
> better early then later...
Yes, we could clear up the terminology. The term 'pof' should be
replaced with 'Yubikey', I've done so for the yubico-c project but
haven't had time to do it for the java project as well.
I think we should allow ourselves to change the names of these objects
right now. I trust you to suggest good ideas here. :)
> Part two:
>
> Considering cookbook validation implementation... but let discuss
> above first ;-)
Yup.
However, before we move ahead with committing patches for any of these
ideas, I'd like to get 'ant' building to work again. I'm not an
eclipse-person, so I need to build the project from the command line.
This will also be necessary for automated build robots and so on. Right
now, building the yubico-j package fails but I haven't looked into why.
/Simon
Arne Halvorsen
I fixed the bug that cap lock bit was read from the least significant byte and not the most significant byte.
Please review this edit Simon... :
http://code.google.com/p/yubico-j/source/diff?r=47&format=side&path=/trunk/src/com/yubico/base/Token.java
It passes unit tests, but that do not say much I think, this probably bug was not caught anyway.
I think Yubico should produce a set of test vectors for a key being in all possible states and
stages in life. Then it will be easy to make unit tests not only for low level libraries and across
languages. I think this should have priority.
bye.
Simon Josefsson
> Hi,
>
> I fixed the bug that cap lock bit was read from the least significant byte
> and not the most significant byte.
>
> Please review this edit Simon... :
>
> http://code.google.com/p/yubico-j/source/diff?r=47&format=side&path=/trunk/src/com/yubico/base/Token.java
>
> It passes unit tests, but that do not say much I think, this probably bug
> was not caught anyway.
The patch looks fine to me, thank you
/Simon