Hashing API #258

unixninja92 · 2014-07-08T18:35:36Z

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.

bertm · 2014-07-08T20:25:53Z

src/freenet/crypt/Hash.java

+
+	/**
+	 * Generates the hash of all the bytes added using addBytes() methods
+	 * @return Hash of all the bytes added since last reset.


[…] since last reset

I don't find a method that allows for resetting, nor one that specifies this as a side-effect.

The finally block is wrong, isn't it? We don't want to recycle the digest if we have a successful construction?

bertm · 2014-07-08T21:36:21Z

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.

bertm · 2014-07-08T21:46:47Z

src/freenet/crypt/Hash.java

+		try {
+			digest = type.get();
+		} finally {
+			type.recycle(digest);


Recycling a digest to which we still have references (namely in the member variabledigest) is bound to give problems at some point: another thread may obtain it by means of HashType.get() while we are still using it!

If you insist on recycling digests, I guess this could be done in the finalizer of Hash. Note that the semantics of finally don't have anything to do with finalizers!

I'm perfectly happy stripping the recycling code, I just included it as that is what we are currently doing.

Where? The above is plainly wrong, is there similar code somewhere in fred at present? The problem here is we want to recycle it if it throws, but not otherwise. Actually we probably don't want to recycle it if it throws...

No, this is not what we are currently doing. Read src/freenet/crypt/MultiHashInputStream.java and src/freenet/crypt/MultiHashOutputStream.java, and closely watch the digest = null; immediately after the recycle.

I'll move this to some sort of close() method.

unixninja92 · 2014-07-09T02:09:37Z

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?

bertm · 2014-07-09T08:32:55Z

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.

bertm · 2014-07-09T08:41:18Z

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).

bertm · 2014-07-10T00:31:49Z

src/freenet/crypt/HashResult.java

+	public boolean equals(Object otherObject){
+		//check not null
+		if(otherObject == null){
+			throw new NullPointerException();


No need for throwing an exception since we can rest assured this ≠ otherObject holds when we arrive here, and thereby have a valid answer to return. The null case is comparable to !(otherObject instanceof HashResult) case (even better: !(null instanceof HashResult) always holds. so we can effectively entirely skip the null check here).

I don't think that I've ever come across an implementation that throws an NPE in equals, now I think of it.

return false; is more appropriate here, or even better, skip this entire check (the next check will do it for you).

Thynix · 2014-07-29T01:14:14Z

What's the different between byte[] and ByteBuffer? The former seems to be used in KeyGenUtils a lot, and the latter here. Why?

Thynix · 2014-07-29T01:17:11Z

src/freenet/crypt/Hash.java

+	public final byte[] genHash(){
+		byte[] result = digest.digest();
+		if(type == HashType.ED2K){
+			//ED2K does not reset after generating a digest. Work around this issue


Does Bouncy Castle have bugs filed for these? It may make sense to add tests to check that these are still broken, so that when / if those tests break the workaround can be removed.

Would this be more concise as a switch statement?

These are not BC classes. They are third party classes we are using.

unixninja92 · 2015-03-22T22:33:54Z

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.

unixninja92 · 2015-03-22T23:21:51Z

@Thynix Does this commit message meet the commit message standard, or should I add a newline between the hashing api and jceloader lines?

Thynix · 2015-03-25T04:22:50Z

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 · 2015-03-25T04:31:57Z

src/freenet/crypt/JceLoader.java

+		        kgen.init(256);
+		    }
+		    catch(Throwable e) {
+		        final String msg = "Error with SunJCE. Unlimited policy file not installed.";


It looks like this string was intended to also be printed to stdout / stderr? Otherwise I'd expect it to be within the logger call.

Longer-term I'm interested in Freenet moving to an existing logging framework and doing stdout/stderr with log level configuration instead of manually.

That makes sense. I think at the time I was just copying convention used at the file, but I'll change it.

unixninja92 · 2015-03-25T04:41:59Z

@Thynix The policy seems reasonable to me.

Working on splitting the commit right now.

Users with limited policy files should have an indication that performance is known to be degraded and why.

unixninja92 · 2015-03-25T05:11:32Z

Should be good now. @Thynix should I merge it with master or should I leave that to you?

Thynix · 2015-03-25T05:16:56Z

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.

Thynix · 2015-04-04T12:22:22Z

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.

Thynix · 2015-04-04T12:39:32Z

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)

Thynix · 2015-04-04T13:03:04Z

I've merged this into a HashingAPI branch for inspection.

bertm · 2015-04-08T08:57:36Z

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.

unixninja92 · 2015-12-27T02:27:08Z

@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.

unixninja92 · 2015-12-27T03:55:52Z

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.

This makes it easier to use and change hashing algorithms.

unixninja92 · 2015-12-27T05:29:41Z

@Thynix Fixed up the commits

Thynix · 2015-12-30T14:18:27Z

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 notifications@github.com
wrote:

@Thynix https://github.com/Thynix Fixed up the commits

—
Reply to this email directly or view it on GitHub
#258 (comment).

xor-freenet · 2018-03-16T13:52:58Z

@unixninja92

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.

They need to be enabled by passing -ea / -enableassertions to the JVM, and I would consider assertions being disabled in unit tests as a bug which should please be fixed or documented on the bugtracker before this is closed.

With the Ant builder it works like this.

Now that we've replaced Ant with Gradle on next it would be done like that:

tasks.withType(Test) {
    enableAssertions = true
}

Notice: It may be the default to enable them in Gradle as I obtained this code snipped from a stackexchange question where someone asked how to disable them.

If you want to increase the probability of this getting merged then please merge next backwards into this and apply the fix for Gradle if necessary instead of just fixing it for the Ant code you had based this upon.

bertm reviewed Jul 8, 2014
View reviewed changes

bertm reviewed Jul 10, 2014
View reviewed changes

Thynix reviewed Jul 29, 2014
View reviewed changes

unixninja92 force-pushed the HashingAPI branch from 3527d18 to 3fbcc84 Compare March 22, 2015 22:04

unixninja92 force-pushed the HashingAPI branch from 4e6ffc2 to f0b5d57 Compare March 22, 2015 22:45

Thynix reviewed Mar 25, 2015
View reviewed changes

JceLoader: log warning on limited policy file

48c8ad3

Users with limited policy files should have an indication that performance is known to be degraded and why.

unixninja92 force-pushed the HashingAPI branch from d7031ea to 3d4ed5e Compare March 25, 2015 05:00

Add hashing API and tests

1691971

This makes it easier to use and change hashing algorithms.

unixninja92 force-pushed the HashingAPI branch from 0b08ab8 to 1691971 Compare December 27, 2015 05:28

nextgens mentioned this pull request Jan 1, 2016

Replace our HMAC implementation by whatever JCA provides #467

Merged

ArneBab added the afterNextRelease looks good, but would delay the next release or make it too big label Jun 9, 2018

Merge next into HashingAPI

ad6e653

unixninja92 force-pushed the HashingAPI branch 2 times, most recently from 507d166 to 73cb448 Compare September 1, 2018 03:03

Enable assertions in gradle

9db7ea6

unixninja92 force-pushed the HashingAPI branch from 73cb448 to 9db7ea6 Compare September 1, 2018 03:16

ArneBab merged commit 84b3929 into hyphanet:next Nov 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hashing API #258

Hashing API #258

unixninja92 commented Jul 8, 2014

bertm Jul 8, 2014

toad Jul 8, 2014

bertm commented Jul 8, 2014

bertm Jul 8, 2014

unixninja92 Jul 8, 2014

toad Jul 8, 2014

bertm Jul 8, 2014

unixninja92 Jul 9, 2014

unixninja92 commented Jul 9, 2014

bertm commented Jul 9, 2014

bertm commented Jul 9, 2014

bertm Jul 10, 2014

Thynix commented Jul 29, 2014

Thynix Jul 29, 2014

unixninja92 Jul 29, 2014

unixninja92 commented Mar 22, 2015

unixninja92 commented Mar 22, 2015

Thynix commented Mar 25, 2015

Thynix Mar 25, 2015

unixninja92 Mar 25, 2015

unixninja92 commented Mar 25, 2015

unixninja92 commented Mar 25, 2015

Thynix commented Mar 25, 2015

Thynix commented Apr 4, 2015

Thynix commented Apr 4, 2015

Thynix commented Apr 4, 2015

bertm commented Apr 8, 2015

unixninja92 commented Dec 27, 2015

unixninja92 commented Dec 27, 2015

unixninja92 commented Dec 27, 2015

Thynix commented Dec 30, 2015

xor-freenet commented Mar 16, 2018 •

edited

Hashing API #258

Hashing API #258

Conversation

unixninja92 commented Jul 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertm commented Jul 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unixninja92 commented Jul 9, 2014

bertm commented Jul 9, 2014

bertm commented Jul 9, 2014

Choose a reason for hiding this comment

Thynix commented Jul 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unixninja92 commented Mar 22, 2015

unixninja92 commented Mar 22, 2015

Thynix commented Mar 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unixninja92 commented Mar 25, 2015

unixninja92 commented Mar 25, 2015

Thynix commented Mar 25, 2015

Thynix commented Apr 4, 2015

Thynix commented Apr 4, 2015

Thynix commented Apr 4, 2015

bertm commented Apr 8, 2015

unixninja92 commented Dec 27, 2015

unixninja92 commented Dec 27, 2015

unixninja92 commented Dec 27, 2015

Thynix commented Dec 30, 2015

xor-freenet commented Mar 16, 2018 • edited

xor-freenet commented Mar 16, 2018 •

edited