fred
fred copied to clipboard
freenet
Hashing API
Here is the Hashing API I have written as part of my GSoC project. Also added a check of policy files for JCE in JCELoader.
Please add tests on cases that you expect to fail (using assertFalse and such, obviously) as well: you should have caught the problem in Hash.verify(…) that way.
I'm trying to figure out the best way to use the AutoCloseable interface with Hash to fix the recycling problem. One idea is to make Hash an AutoCloseable that is interfaced by a different class that would use try-with-resources. Or I could push this on the user, but that doesn't feel like the right solution to me. My other idea is to write a MessageDigest that implements AutoCloseable that would be an interface to the underlying MessageDigests. I'm not sure which one would make the most sense though. Any thoughts/ideas?
My other idea is to write a MessageDigest that implements AutoCloseable that would be an interface to the underlying MessageDigests.
Something like the decorator pattern? Sounds good, but if we only use it in Hash, making Hash AutoClosable may make more sense.
Please be aware of the following: from MessageDigest Javadoc: "The digest method can be called once for a given number of updates. After digest has been called, the MessageDigest object is reset to its initialized state."
That may influence your choices in Javadoc writing. If getHash() really must reset the digest, please strongly emphasize this behaviour or even change the name of the method. Might intermediate hashes be considered useful, then you might want to clone the digest before invoking digest (if the digest implementation allows for this).
What's the different between byte[] and ByteBuffer? The former seems to be used in KeyGenUtils a lot, and the latter here. Why?
@Thynix The reason for the use of ByteBuffer is because the underlying classes support it and I have found several places throughout the Freenet codebase that use ByteBuffers.
@unixninja92 ByteBuffers seem higher-level. Given the option between byte[] and ByteBuffer are ByteBuffers preferable?
@Thynix If we want to move to higher-level things, @nextgens likes BitSets. From what I can tell moving to something higher level would be a lot of work in for example the JFK exchange and might not be worth it.
I believe it's all set. Could maybe do with some slight doc changes IIRC, but other than that I think it's good.
Oh yeah, I was gonna combine commits, so it looked cleaner. I can do that tomorrow night.
If you're already squashing, would you be willing to bring the commit messages up to the commit message standards? If they're unclear please ask. Not doing it and getting this grandfathered in is an option, but it's liable to look bad in the git shortlog otherwise. An example of shortlog gone wrong is the 1467-pre1 shortlog.
This needs to be looked at very carefully if we're actually going to use it. The crypto code that I merged in purge-db4o-crypto had some serious bugs and API issues.
@toad Were there actually serious bugs? @nextgens seemed to think that it was more that the API did not match your expectations.
Whether they were or not is irrelevant :)
Merge with extra caution is what toad meant
There was at least one serious bug, see the comments on devl. But yes, all I mean is be careful - and note that poor APIs can cause serious bugs; the API should be fully documented and reasonably well thought through before merging.
IIRC all the issues @toad had with my other APIs was centered around how I was working with ByteBuffers. This API only has one method that uses ByteBuffers and it is only passing it on to a different method.
I've gone through all the comments on here, and fixed anything that hadn't already been fixed that should be (which was just rewriting a comment).
I've squashed everything down to one commit, and am working on writing up a nice commit message right now.
@Thynix Does this commit message meet the commit message standard, or should I add a newline between the hashing api and jceloader lines?
It's close. The standards (won't be merged until at least someone else says they're happy with them - do they seem reasonable to you?) are here.
Looking at the standards, that message needs two changes for formatting:
- The summary needs to be imperative: "added" should be "add".
- The second line must be blank.
Other than that, this looks like it should be two commits (after all, it was trying to include two summaries) - one for adding the hashing API and another for the JCELoader changes. My initial reaction is that some parts of the commit body seem unnecessary to mention - this is intended to give relevant background information to someone reviewing the change when it is made, seeing the summary in the changelog, or looking back at it to get details on why the change was made and what it was intended to do.
Therefore I'd suggest something more like:
Add hashing API and tests
This makes it easier to use and change hashing algorithms.
and
JceLoader: log warning on limited policy file
Users with limited policy files should have an indication that
performance is known to be degraded and why.
Thoughts? Is this a reasonable response for me to make? I'd do something like this at my job but I'm having a lot of trouble figuring out what kind of feedback to give in open-source projects.
@Thynix The policy seems reasonable to me.
Working on splitting the commit right now.
Should be good now. @Thynix should I merge it with master or should I leave that to you?
Thanks! I'll have to make a 1468 release branch in order to merge this to next, but I plan to do so ... soon. Maybe tomorrow - my schedule is still fuzzy currently.
I haven't looked at the severity of the merge conflicts yet and might enlist your assistance in figuring them out if it's not obvious. If you're up for merging with next locally and fixing the conflicts that would be helpful.
Can you remind me why this is a single Hash class that supports multiple hash types instead of an interface and multiple implementing classes? I might have asked this before but I haven't been able to find it. I ask this not so much to suggest a change but to document the reasoning.
I get failing tests:
[junit] null
[junit] junit.framework.AssertionFailedError
[junit] at freenet.crypt.HashResult.<init>(HashResult.java:30)
[junit] at freenet.crypt.HashTest.testVerifyHashResultByteArrayWrongSizeMac(HashTest.java:333)
That assertion is expected to fail there. Consider the constructor code, and the name of the test where it fails.
public HashResult(HashType hashType, byte[] bs) {
[...]
assert(bs.length == type.hashLength);
}
We'd need to get rid of that assertion (at least for unit testing). Adding another (package-private) constructor without the assertion to accomplish this may be the easiest way out, without having to drop this assertion where it is useful.
@unixninja92 please update your configuration to enable assertions on unit testing.
@Thynix It's a single hash class to make it as easy as possible to upgrade to different hash types in the future.
I'm gonna try setting up my comp for freenet dev again and resolving the test issues. I think I did a fresh install since I last worked on this.
So I've fixed the assertion issue as @bertm suggested. I can't seem to figure out how to enable assertions for unit testing, so please let me know if there are any other places this needs to be fixed. @Thynix I'll fix up commit messages after that has been tested.
Thanks! 1471 is settling down for release, so we can look at merging this after its release. 1471-pre3 - hopefully its last prerelease - is scheduled for this weekend.
On Sun, Dec 27, 2015, 12:29 AM Charles Teese [email protected] wrote:
@Thynix https://github.com/Thynix Fixed up the commits
— Reply to this email directly or view it on GitHub https://github.com/freenet/fred/pull/258#issuecomment-167381969.