search

google / keyczar

Easy-to-use crypto toolkit
Apache License 2.0
1.1k stars 141 forks source link

Long standing issue of aes keys greater than 128bit not creating the correct hash in Java #165

Closed jbtule closed 8 years ago

jbtule commented 9 years ago

Fixes issue #105, fixes issue #107, refs issue #108

devinlundberg commented 9 years ago

I'm not going to nit pick on this too much, but there are areas where code is duplicated and could be refactored. I don't see this as a hurdle to merging. Also not sure if the google linter has been applied, but I didn't notice anything bad on that front.

In my opinion, there are two major hurdles to merging this: 1) Needs to be rebased with the current repo 2) Stream cache/queue changes should be evaluated in more depth. I believe they were added as a method of thread safety and I'm fairly certain this change doesn't provide the same guarantees. Someone more familiar with this code should look at them in more depth and suggest a way to refactor.

divegeek commented 8 years ago

I'd like to merge this. Can you rebase? Also, Devin are you happy with the fix to the cache thread safety issue?

devinlundberg commented 8 years ago

@divegeek The fix looks like it satisfies the thread safety problems. You could conceive cases where that queue of streams will grow really large if there are regularly a stampede of key accesses across different threads which would cause memory issues, but it probably won't come up (and would probably be limited by the number of threads). This is actually a really bad cross implementation bug so I would rather merge and optimize later if thats even a problem. This should be mergeable with a rebase.

divegeek commented 8 years ago

Thanks @devinlundberg

@jbtule, if you can rebase, I'll merge.

googlebot commented 8 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

jbtule commented 8 years ago

I signed it! (I updated my CLA to have my github username)

googlebot commented 8 years ago

CLAs look good, thanks!

jbtule commented 8 years ago

@divegeek I rebased off of master. There were conflicts because logging has been removed in master since I originally rebased these commits years ago when asked. But I merged it and it built and tests pass.

divegeek commented 8 years ago

Merged. Thanks!

divegeek commented 8 years ago

Oops, I neglected to run the tests before merging, and this commit causes a failure.

Jay, can you look in to why org.keyczar.BadHashTest.testBadJavaHashAesDecrypt is failing?

divegeek commented 8 years ago

Actually, Jay, I'm going to revert this so I can tag the release. I've got a flight tomorrow; I'll see if I can figure out what's up with that test, unless you beat me to it.

jbtule commented 8 years ago

on Ubuntu (java 1.6) it works: Running org.keyczar.BadHashTest

Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.006 sec Tests run: 102, Failures: 0, Errors: 0, Skipped: 0

But I just ran it on my laptop with OS X (java 1.8) and it's throwing an error in the test, maybe jvm crypto exception difference. I'm looking into it.

On Tue, May 3, 2016 at 2:14 PM, Shawn Willden notifications@github.com wrote:

Actually, Jay, I'm going to revert this so I can tag the release. I've got a flight tomorrow; I'll see if I can figure out what's up with that test, unless you beat me to it.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

jbtule commented 8 years ago

Hah, turns out there was no issue, it's because you need a 192 or 256 bit key to test the bad hash, and if you don't have the Java Cryptography Extension (JCE) Unlimited Strength Jurisdiction Policy installed you can't test either of those key sizes.

On Tue, May 3, 2016 at 8:05 PM, Jay Tuley jay@tuley.name wrote:

on Ubuntu (java 1.6) it works: Running org.keyczar.BadHashTest

Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.006 sec Tests run: 102, Failures: 0, Errors: 0, Skipped: 0

But I just ran it on my laptop with OS X (java 1.8) and it's throwing an error in the test, maybe jvm crypto exception difference. I'm looking into it.

On Tue, May 3, 2016 at 2:14 PM, Shawn Willden notifications@github.com wrote:

Actually, Jay, I'm going to revert this so I can tag the release. I've got a flight tomorrow; I'll see if I can figure out what's up with that test, unless you beat me to it.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

divegeek commented 8 years ago

Heh. You got it in one. I ran the tests on my Macbook. I won't bother reverting, then, though it would be good if we could find a way to avoid the failure on OS X.

jbtule commented 8 years ago

I think this is all platforms, it's just likely to occur if you have a fresh installation of java which won't have the Java Cryptography Extension (JCE) Unlimited Strength Jurisdiction Policy unless you do it manually.

Here are some suggestions on how to avoid it or test for it.

http://stackoverflow.com/questions/1179672/how-to-avoid-installing-unlimited-strength-jce-policy-files-when-deploying-an

On Tue, May 3, 2016 at 10:39 PM, Shawn Willden notifications@github.com wrote:

Heh. You got it in one. I ran the tests on my Macbook. I won't bother reverting, then, though it would be good if we could find a way to avoid the failure on OS X.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/google/keyczar/pull/165#issuecomment-216733991