Change suggestion or yubico-j
aha42
changes, added 2, explicit in 5 and added question in 9.
1. Major: Remove import of org.apache.log4j.Logger in Modhex.java and
Display.java. Is not used and is a show stopper for any who are not
using that logging API.
2. Major: Write unit tests before doing more changes to the code. I
suggest for now not to use JUnit or any other framework. Reason: Code
then get dependent on external API such as JUnit. For this small API
we can probably ensure quality by a main program running unit tests in
a test class in a test package. Write in xUnit style so can easily be
ported to a framework later if desirable.
3. Minor: In Modhex.java: Probably ok to remove commented away line 14
and variable b on line 29 is not used.
4. Minor: In Display.java: Remove not used import
java.io.ByteArrayOutputStream.
5. Request: Code checked out into my eclipse environment look really
bad formated. Not sure of cause of this. If the cause is the use of
hard tabs, consider using soft tabs (spaces). Suggestion: use soft tab
of two characters.
6. Suggestion: Write missing Java doc, and check in generated html in
svn with MIME type property text/html: Can be viewed directly from
Google code project site.
7. Idea: Good idea not to assume lowercase input to the Modhex decode
method?
8. Request: In comments at line 17 in Pof.java refers to pof_check_*.
Where do one find this, in another API? Perhaps information to be
provided in a better java doc.
9. Idea/request: Would it not been user friendly to also provide the
properties sessionCounter, timeStampLow, timeStampHigh and timesUsed
of Token as int values? Perhaps need to consider interface in light of
writing OTP checking code?
While 1 and 2 are important all but 8 and 9 is trivial so given we
agree on some of these points I would not mind do the hacking if you
want me to. (formal request to join yubico-j project I guess..:-).
Simon Josefsson
Re 1, getting rid of external dependencies is great.
Re 2, using a xUnit style seems like a very good idea. The code should
be minimal.
Re 8, I believe the comment refers to old code that we don't have any
more (it ended up in the yubico-c project).
I have added you to the yubico-j project. Feel free to make these
changes. Ping the list and I'll review and revert anything that I don't
like. :)
Thanks,
Simon
PS. The google code svn server seems broken sometimes, so if you get a
'Bad Gateway' error just retry later.
Simon Josefsson
release your contributions under the 2-clause BSD license:
http://code.google.com/p/yubico-j/source/browse/trunk/COPYING
Are you ok with this license?
Thanks,
/Simon
Arne Halvorsen
code or as a user as a potential developer.
Arne Halvorsen
Simon Josefsson
with your commits.
/Simon
Arne Halvorsen
Made Modhex alphabet String a public constant: Other code may want to know, this is the
proper location for it. Renamed to ALPHABET: Modhex.ALPHABET reads well in code.
Made decode accept both lower and upper case.
Added java doc.
Private constructor.
Formated to soft tab of two white spaces.
Removed commented away line and not used variable b.
---
Checked in test package com.yubico.base.test with test class for Modhex.
Simon Josefsson
Simon Josefsson
[javac] /home/jas/src/yubico-j/src/com/yubico/base/test/ModhexTest.java:4: static import declarations are not supported in -source 1.4
[javac] (try -source 1.5 to enable static import declarations)
[javac] import static com.yubico.base.test.UnitUtil.assertEqual;
[javac] ^
[javac] 1 error
Is there a simple fix to this other than to bump the java requirement to
1.5? I'm not sure if bumping the java requirement will be a problem,
but if it can be avoided easily that is always good.
One alternative may be to bump it to 1.5 only for the test directory.
Thanks,
Simon
Arne Halvorsen
I was thinking, should keep this <1.5, no reason to go higher with this.... Habit. Will fix!
Arne Halvorsen
Simon Josefsson
> Sorry!
>
> I was thinking, should keep this <1.5, no reason to go higher with this....
> Habit. Will fix!
Works fine now, thanks!
I have linked the JavaDoc manual from the project front page too. They
look very nice. How do you generate these files?
I've incremented the version from 1.0 to 1.1. Maybe we should do
another release when you are finished with your changes. No need to
delay it, release early release often and all that...
Btw, 'ant test' fails but maybe you are working on this?
[junit] Running com.yubico.base.test.ModhexTest
[junit] Tests run: 1, Failures: 1, Errors: 0, Time elapsed: 0.005 sec
[junit] Test com.yubico.base.test.ModhexTest FAILED
[junit] Running com.yubico.base.test.PofTest
[junit] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 0.327 sec
/Simon
Arne Halvorsen
Recent changes:
====
Checked in generated java doc.
URL to java doc home: http://yubico-j.googlecode.com/svn/trunk/doc/generated/index.html
Please review.
You may consider providing a project front page link (above url) to java doc.
====
Added UnitTest class for Pof and Token.
====
Token.java:
Added get methods.
Reformatted to soft tab of two white spaces.
Added java doc.
====
Pof.java:
Made private constructor.
Added java doc.
Formatted to soft tab using two spaces.
===
Display.java:
Added private constructor.
Exit codes.
Java doc.
Formated to use soft tabs of two spaces.
=============
Arne Halvorsen
Arne Halvorsen
I have been using junit but is not a BIG expert... Is it possible to do so without making project link dependent on junit jars?
I dont mind converting to junit tests, my eclipse env. is by default ready for it.
Simon Josefsson
/Simon
Simon Josefsson
junit and make 'ant test' use the internal self-tests instead? I'm
always in favor of reducing external dependencies if doing so is simple.
I'm not a strong java programmer, so I don't know what the Best Practice
within the java community is with regards to ant vs eclipse, junit vs
other frameworks.
/Simon
Arne Halvorsen
'' favor of reducing external dependencies". I do not understand why nobody has
made a xUnit framework purely using reflection (at least that I know of).
Eclipse I am told by my coworker is a good Ant friend, I used to do a lot of Ant
hacking in the old days so I can look into this... But soon going to meet some
friends at the pub seeing Sverige win so doubt get much done tonight ;-(
Arne Halvorsen
-Use the java tool javadoc usual found in jdk's bin dir.
-It has a tremendous set of option, but most IDE's provide wizard interface to do this: In eclipse it is important to select project root and then choose to generate doc or you get only partial java doc generated (thats a feature but will feel as a bug most of the time).
-Beside the java doc comments I will say each package directory must contain a file package.html containing one paragraph of introduction to package. It may not be of much help, but thing is that you will get a package table generated on doc home page, and that looks so sad with right column empty.
-Also provide a overview.html which location is told by the javadoc option overview to be included in the doc home page (overview). Put that in a separate dir. then where doc is generated or it is easy to delete it if you nuke all generated doc, something that is usefull to do...
-I also used options
-doctitle yubico-j
-windowtitle yubico-j
-header target='new' href='http://code.google.com/p/yubico-j/'>Project home
-footer target='new' href='http://code.google.com/p/yubico-j/'>Project home
And when checking in to select doc root dir and recursively set svn property svn:mime-type to text/html
This let readers surfe doc in svn database directly.
aha42
The next I was thinking of was get function that is more user friendly
towards validator programmers.
But perhaps should be discussed in terms of what plans one has with
this library..?..
On Jun 18, 6:20 pm, Simon Josefsson <si...@yubico.com> wrote:
Simon Josefsson
ModhexTest classes: there are the new classes under
src/com/yubico/base/test/ and the old classes under
test/com/yubico/base/test. The ModhexTest class are called the same.
Ant uses the one in src/, and since it isn't written for junit, the
junit self test fails...
Putting the self-tests in src/ will enlarge the core library *.JAR, so I
would be in favor of putting all self tests under test/ and building
them to a separate JAR. And if we separate the files, I don't think it
matters if we use a external unit framework because it will only enlarge
the *-test JAR, not the core JAR.
Arne, what do you think? Is moving your tests from src/ to test/ and
rewriting them to use the junit framework ok with you?
Simon Josefsson
> We are on extremely same page when it comes to
> '' favor of reducing external dependencies". I do not understand why nobody
> has
> made a xUnit framework purely using reflection (at least that I know of).
Maybe junit 4 supports it? Anyway, it seems junit is rather popular.
> Eclipse I am told by my coworker is a good Ant friend, I used to do a lot of
> Ant
> hacking in the old days so I can look into this... But soon going to meet
> some
> friends at the pub seeing Sverige win so doubt get much done tonight ;-(
You must have seen a different match than I did... :-(
/Simon
Simon Josefsson
this, but it is not a high priority.
Simon Josefsson
> Going up from 1.0, 1.1 might be ok now...
Yup, if we can sort out the self test stuff, I think we should release
1.1.
> The next I was thinking of was get function that is more user friendly
> towards validator programmers.
> But perhaps should be discussed in terms of what plans one has with
> this library..?..
The idea is that this library should be small and only contain core
functions -- it can then be used like larger packages like the
'yubico-server-j' project that implements a complete server.
However, if it is possible to write a validator function that abstracts
away some of the details in verifying an OTP that may fit into this
library. I'm not sure it is possible to write this in a abstract and
flexible enough way though. But a patch would prove me wrong. Please
don't commit any validation stuff without discussion first, though.
/Simon
Arne Halvorsen
for any reasons. My thinking was that for this small lib it would be nice if developers could
drop the code in their dev areas and that was it. But being JUnit dependent is probably no
big deal. Actual most IDE environments I guess comes with it, I know eclipse does.
I do not think you need to split in different jars: In production tests methods will not be
invoked so no 'ClassNotFoundException's should be thrown. And you have the option
to run test on a jar if so need to. Probably the enlargment of the jar will not be a problem.
But to put them under /test could be an idea.
So, is this what we should do:
1. Put tests in: test/com/yubico/base/test
2. Refactory to JUnit
If you want to I can probably get that done before tonight matches (feel bad for Sverige!).
Sorry about causing these problems.
Simon Josefsson
because you could probably remove Hex.java and UnitUtil.java. I'll be
leaving for midsommar later today, but I will probably have time review
any patches while I'm away anyway but you never know.
Btw, what are your thoughts on log4j? Right now log4j is only used by
the old self-tests, which seems rather pointless to me -- if there are
problems with the self-tests, a developer needs to look at the code
anyway.
I think we should remove all traces of log4j in this project.
Arne Halvorsen
will come back to that:-)
I do not like any of the logging framework. In our company we have made our own
logging code, one for client side and one for server side and are happy with that.
That a side, logging is important special in servers. In open source code I would probably
go along and use log4j if situation dictated. But I do not think the type of methods this
library has should do logging or dictate logging framework. Mine thinking...
Simon Josefsson
> I do not like any of the logging framework. In our company we have made our
> own
> logging code, one for client side and one for server side and are happy with
> that.
> That a side, logging is important special in servers. In open source code I
> would probably
> go along and use log4j if situation dictated. But I do not think the type of
> methods this
> library has should do logging or dictate logging framework. Mine thinking...
I've removed all log4j stuff from yubico-j now. Servers can and
probably chose to use a logging framework, but that is beyond the scope
of the yubico-j project.
Thanks,
/Simon
Arne Halvorsen
Another thing, the Hex thing...
A suggestion I was going to make was to change Display to accept AES key as hex encoded.
Now it is Modhex encoded which it not very usable because only output from Yubikey are
found as Modhex in the wild. AES keys are more likely to be communicated as hex.
What do you think? That would of course make the need for Hex and even be made public
since would like to use in test. Perhaps make a util package for stuff that is internal but
need to be public, document other code should not use to become dependent on artifacts.
Simon Josefsson
somewhat hesitant about adding it to the core library (instead it could
be added in the tools/ directory together with Display) since it isn't
core functionality. On the other hand, the size of this code is quite
small so I don't care strongly.
/Simon
Arne Halvorsen
Arne Halvorsen
aha42
Suggestion for further changes:
(These are ideas for discussions :-)
++++++++++++
Part one:
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.
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); }
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)
By the way, is there a need to review terms over projects? You tell
me, but if so,
better early then later...
++++++++++++
Part two:
Considering cookbook validation implementation... but let discuss
above first ;-)
>
> > > We are on extremely same page when it comes to
> > > '' favor of reducing external dependencies". I do not understand why
> > nobody
> > > has
> > > made a xUnit framework purely using reflection (at least that I know of).
>
> > > Eclipse I am told by my coworker is a good Ant friend, I used to do a lot
> > of
> > > Ant
> > > hacking in the old days so I can look into this... But soon going to meet
> > > some
> > > friends at the pub seeing Sverige win so doubt get much done tonight ;-(
>
> > > On Wed, Jun 18, 2008 at 6:44 PM, Simon Josefsson <si...@yubico.com>
> > wrote:
>
> > >> I'm not deeply married to the concept of using junit. Can we get rid of
> > >> junit and make 'ant test' use the internal self-tests instead? I'm
> > >> always in favor of reducing external dependencies if doing so is simple.
>
> > >> I'm not a strong java programmer, so I don't know what the Best Practice
> > >> within the java community is with regards to ant vs eclipse, junit vs
> > >> other frameworks.
>
> > >> /Simon
>
>
> > >> > Oh... you are using junit on those, did not know that was possible. I
> > >> simply
> > >> > run the mains and get no exception so the test are ok as I made them.
>
> > >> > I have been using junit but is not a BIG expert... Is it possible to
> > do
> > >> so
> > >> > without making project link dependent on junit jars?
>
> > >> > I dont mind converting to junit tests, my eclipse env. is by default
> > >> ready
> > >> > for it.
>
> > >> > On Wed, Jun 18, 2008 at 6:26 PM, Arne Halvorsen <
>
> > >> >> You are ahead of me now :-), hang on, will have a look at those
> > tests...
> > >> >> hangon
>
> > >> >> On Wed, Jun 18, 2008 at 6:23 PM, Arne Halvorsen <
Arne Halvorsen
The project was created based on the code of the present yubico-j project but with the changes outlined in the previous test.
Next I want for to be able to generate synthetic password and have implemented
com.yubico.base.tools.PasswordGenerator
Used it to write more unit test for Token:
com.yubico.base.test.TokenTest
I also think the generator will be of / is of use to generate test data for more high level systems.
Arne Halvorsen
Arne Halvorsen
Changed the name on getCleanCounterBytes methods of Token to getCleanSessionCounterBytes to be consistent.
Added getCounter() method to Token, convenient to have for basic validation (not taking lost OTP or timestamp into account).
aha42
On Jul 17, 2:25 pm, "Arne Halvorsen" <arne.halvor...@gmail.com> wrote:
> Updates:
>
> Changed the name on getCleanCounterBytes methods of Token to
> getCleanSessionCounterBytes to be consistent.
>
> Added getCounter() method to Token, convenient to have for basic validation
> (not taking lost OTP or timestamp into account).
>
>
> > Added isStatic() method to Token.
>
> > wrote:
>
> >> Hopes ok that I writes on updates for the proposing yubico-j project<http://code.google.com/p/arne-proposed-yubikey-j/>here.
> >> The project was created based on the code of the present yubico-j project
> >> but with the changes outlined in the previous test.
>
> >> Next I want for to be able to generate synthetic password and have
> >> implemented
> >> Used it to write more unit test for Token:
> >> I also think the generator will be of / is of use to generate test data
> >> for more high level systems.
>
>
> read more »