Finished first version of PRNG functionality #14

Merged
merged 30 commits into from Apr 10, 2012

Conversation

Projects
None yet
2 participants

eadmund commented Mar 26, 2012

I feel pretty good about this...let me know if you think anything is a show-stopper.

Tests run cleanly, docs build cleanly, the code looks good. Some more PRNG-specific tests would be nice of course.

How about (make-array 16 :element-type '(unsigned-byte 8) :initial-element 0)?

Owner

eadmund replied Mar 2, 2012

Yeah, that was a relic of when I used to use a non-zero initial value. Fixed.

Nit: The indentation in a lot of places seems to be off when viewing the diffs on GitHub (e.g. ironclad.asd). Could you make sure you're using spaces instead of tabs for indentations?

Nit: You have a number of places where forms are separated by two newlines; please use one newline for separation.

Owner

eadmund replied Mar 3, 2012

Doh! I had a setq instead of setq-default for indent-tabs-mode. Fixed that, and converted all tabs.

Also removed all the newlines.

Nit: This is not quite true; you don't support floats, the calling convention is somewhat different, etc.

Please make sure (adding a test?) that this DTRT for negative numbers.

Owner

eadmund replied Mar 3, 2012

Added support for floats and refusal to support negative numbers.

The calling convention has to be a little bit different, unless you're willing to always load & initialise IRONCLAD:PRNG, in which case STRONG-RANDOM could have an identical convention. In order for prime generation and other things I'd like to add, Ironclad probably needs to have an internal PRNG anyway--do you have any strong feelings one way or the other?

I think IRONCLAD:PRNG (using pseudo-random-number-generator here would not be so bad, or perhaps pseudo-random-state for similarity to the built-in one) would be a fine idea. My quick opinion is that it should be NIL by default so that the user must initialize it explicitly for their application.

Owner

eadmund replied Mar 3, 2012

I added IRONCLAD:PRNG, initially NIL. I kinda like the idea of automatically loading it in an intelligent fashion, but that's not a big deal. I might add an IRONCLAD:INITIALIZE-PRNG at some point in the future if it seems to matter much.

I like this; more useful functionality is a good thing. Same nits on horizontal and vertical spacing apply.

Owner

eadmund replied Mar 3, 2012

All those should be fixed now.

Could you also make ironclad:fortuna acceptable here, for consistency with other MAKE-FOO functions in Ironclad?

Owner

eadmund replied Mar 3, 2012

Done. Did it the lame way, by just adding another MAKE-PRNG definition.

Where is +IV-CONSTANT (+IV-CONSTANT+ ?)? I don't see it anywhere in this patch.

Owner

eadmund replied Mar 2, 2012

Another relic. Fixed.

Typo: NIL.

I'm inclined to delete this class; data-less abstract base classes are much less useful in Common Lisp and I don't see anywhere that you'd dispatch on this class specifically.

I'd suggest allocating (DYNAMIC-EXTENT, perhaps) a buffer into which you can PRODUCE-DIGEST. That avoids allocating SEED and APPLY #'CONCATENATE, which I doubt is particularly fast.

Oh, hm, the loop doesn't just run to (LENGTH POOLS), it has this other exit condition. Is it possible to precompute beforehand?

Owner

eadmund replied Mar 2, 2012

Good call. I pre-allocate a properly-sized seed now and fill it accordingly. Didn't bother declaring DYNAMIC-EXTENT; do you think it's worth it?

I am all for long, descriptive names, but I think just using prng here and elsewhere would be perfectly suitable.

Owner

eadmund replied Mar 2, 2012

I would personally prefer 'prng,' but thought that 'pseudo-random-number-generator' might look nicer in documentation, e.g. an SBCL completion prompt.

Should this bare 16 be the block size of the cipher being used?

Owner

eadmund replied Mar 2, 2012

Another case of Fortuna only being specified for 16-byte cyphers.

Should this bare 128 be dependent on the block size of the cipher being used?

Owner

eadmund replied Mar 2, 2012

Well, Fortuna is only defined for 128-bit cyphers...for any larger size it'd need to be re-engineered.

OK, that's fair; I didn't do my homework extensively, then. Can we define some constants (+fortuna-cipher-block-size+?) and check for this property when initializing the generator?

Owner

eadmund replied Mar 3, 2012

Done. I also added the ability to specify the internal cypher used by Fortuna.

I don't know how ugly it would make INTEGER-TO-OCTETS, but if you wanted to add some sort of interface permitting INTEGER-TO-OCTETS to dump into a buffer directly, you could avoid this final CONCATENATE call.

Owner

eadmund replied Mar 2, 2012

Definitely something to investigate. Adding that to TODO.

Nit: Docstring does not match the arguments here.

Owner

eadmund replied Mar 3, 2012

Fixed.

The bare 16s in this function should actually be requesting the block size from the internal cipher, right?

No, I don't think so. The name is a little undescriptive, but that's OK.

Owner

eadmund replied Mar 2, 2012

The advantage of the doubled-SHA construction is that it prevents extension attacks. Dunno if it's worth making visible to other folks or not.

The name is not mine--comes from Schneier, Ferguson & Kohno. They normally spell it as SHAd-256.

I think you want something more like (produce-digest digest :digest seed :digest-start (* i 32)) to avoid allocating.

Owner

eadmund replied Mar 3, 2012

Updated to do the spiritual equivalent (i.e., I'm performing a proper SHAd-256).

How about (reinitialize-instance digest) instead?

Owner

eadmund replied Mar 3, 2012

Done!

Using at least (length pools) here would be good; a paranoid person can use more pools than the default. I'd like to use DIGEST-LENGTH here and below even though Fortuna's specified to use sha-256.

Owner

eadmund replied Mar 3, 2012

Well, it's specified to have 32 pools (that's the reason for the 100-millisecond reseed interval), but you're right that someone could add another one. With the new seed-length calculation, that doesn't matter.

I'm not opposed to using DIGEST-LENGTH. I've changed to a hardcoded (digest-length :sha256); I hardcoded it because this whole thing assumes a 16-byte cypher and 32-byte key. We'll need to wait until there are 32-byte cyphers with 64-byte keys to worry about a future Fortuna^2...

froydnj commented on 51f156c Mar 2, 2012

One thing I realized when looking at all the code is that you :INITFORM KEY, only to through it away in RESEED. And when computing data to feed to RESEED, you SUBSEQ it.

How about something like this for RESEED instead:

(defun reseed (generator seed start &optional end)
(with-slots (key counter cipher cipher-name) generator
(let ((digest (make-digest :sha256)))
(update-digest digest key)
(update-digest digest seed :start start :end end)
(produce-digest digest :digest key)
(reinitialize-digest digest)
(digest-sequence digest :digest key)
(incf counter)
(setf cipher
(make-cipher cipher-name :key key :mode :ecb)))))

That would enable you to get rid of SHAD-256, too; maybe a comment on what step we're doing here would be appropriate.

Bonus points for allocating DIGEST once for the generator and just REINITIALIZE-INSTANCE'ing it here. Also bonus points for REINITIALIZE-INSTANCE on the cipher if it exists already.

Owner

eadmund replied Mar 3, 2012

Hmmm, I like your idea about eliminating the need for SHAD-256. With the new properly-allocated seed, I don't need to bother SUBSEQing it, so I'm just going to remove the SUBSEQ and not bother with START/END.

I'll ASSERT that CIPHER exists, since a generator should always have one. This will let me eliminate CIPHER-NAME, since I can just REINITIALIZE-INSTANCE.

Great input!

eadmund added some commits Mar 3, 2012

(generator): revamp quite a bit, with a more efficient RESEED, a 256-…
…bit initial KEY (as it should always have been) and so forth
(random-data): changed hard-coded 32 to (DIGEST-LENGTH :sha256); remo…
…ved subseq because the seed length is properly calculated now
(*prng*): added an internal PRNG
(strong-random): use *PRNG* instead of a passed-in PRNG

(make-fortuna): fixed bug

I think it's worth making this function callable like RANDOM with an &OPTIONAL (PRNG PRNG) argument.

Owner

eadmund replied Mar 3, 2012

I think those are sufficient, but I also think that trying to make your interfaces conform to what the standard's interfaces look like is valuable in its own right. After all, you added the random float bits, even though you don't need that functionality currently. :)

Owner

eadmund replied Mar 9, 2012

You convinced me...87481b4 has it.

eadmund commented Mar 26, 2012

Reviewing the code this morning, there's one more update I'd like to make: updating arguments to make the PRNG default to \*PRNG\*. Making that now...will post commits soonish.

eadmund commented Mar 27, 2012

Alright, now I think it's ready to go! Let me know what you think.

froydnj added a commit that referenced this pull request Apr 10, 2012

Merge pull request #14 from eadmund/prng
Finished first version of PRNG functionality

@froydnj froydnj merged commit 6428165 into froydnj:master Apr 10, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment