OS-based rng access

rand.rkt

#lang racket/base
(require racket/contract/base  "unix_rand.rkt" "windows_rand.rkt" (only-in file/sha1 bytes->hex-string))
(provide (contract-out [crypto-random-bytes (-> exact-positive-integer? (or/c bytes? string?))]
                       [crypto-random (-> exact-positive-integer? exact-positive-integer?)]))
                       
(define bytes->number (compose (lambda (s) (string->number s 16)) bytes->hex-string))

; (: crypto-random-bytes (-> Positive-Integer Bytes))
; returns n random bytes from the os.
(define (crypto-random-bytes n)
  (case (system-type 'os)
    [(unix macosx) (crypto-random-unix-bytes n)]
    [(windows) (crypto-random-windows-bytes n)]
    [else (raise (make-exn:fail:unsupported
                  "Only UNIX, OS X, and Windows XP or greater are currently supported"
                  (current-continuation-marks)))]))

; (: crypto-random (-> Positive-Integer Nonnegative-Integer))
; returns a random n byte nonnegative-integer
(define (crypto-random n)
  (bytes->number (crypto-random-bytes n)))

unix_rand.rkt

#lang racket/base
(provide crypto-random-unix-bytes)

(define (check-urandom-exists)
  (unless (file-exists? "/dev/urandom")
    (raise (make-exn:fail:filesystem
            "/dev/urandom does not exist"
            (current-continuation-marks)))))

; (: crypto-random-unix-bytes (-> Positive-Integer Bytes))
(define (crypto-random-unix-bytes n)
  (check-urandom-exists)
  (define urandom (open-input-file "/dev/urandom"))
  (read-bytes n urandom))

windows_rand.rkt

#lang racket/base
(provide crypto-random-windows-bytes)

(require ffi/com 
         ffi/unsafe 
         ffi/unsafe/define 
         ffi/winapi)

(define-ffi-definer define-advapi (and (eq? (system-type) 'windows) (ffi-lib "Advapi32.dll"))
  #:default-make-fail make-not-available)

; supposed to be the same csprng as CryptGenRand, but with less overhead
; see Microsoft security dev Michael Howard: http://blogs.msdn.com/b/michael_howard/archive/2005/01/14/353379.aspx
; this is for Windows XP and later only, but I doubt that's a problem
(define-advapi SystemFunction036 
  (_fun #:abi winapi _pointer _ulong -> _bool))

(define (check-SystemFunction036-exists)
  (unless SystemFunction036
    (raise (make-exn:fail:unsupported
            "Unable to load RtlGenRandom (SystemFunction036) from Advapi32.dll"
            (current-continuation-marks)))))

; (: crypto-random-windows-bytes (-> Positive-Integer Bytes))
(define (crypto-random-windows-bytes n)
  (check-SystemFunction036-exists)
  (define rand-bytes-buf (make-bytes n))
  (if (SystemFunction036  rand-bytes-buf n)
      rand-bytes-buf
      (raise (make-exn:fail
              "SystemFunction036 failed to generate bytes"
              (current-continuation-marks)))))

Some initial thoughts:

• Use #lang racket/base instead of #lang racket so that people using #lang racket/base to speed up startup times who happen to use this library don't trigger an import of the whole of racket and lose that speed. Some of the libraries you pull in to implement this like the FFI may themselves use #lang racket, making the speed benefits vanish, but it's still a good practice in the case that those libraries are later optimized to depend on racket/base.
• Provide it with a contract and get rid of the unless positive? check
• The random function takes an upper bound and returns an integer, but secure-random takes a number of bytes to produce and produces bytes. Their names don't imply this difference in return types. I recommend naming it secure-random-bytes and implementing a secure-random function that behaves like random but with input from secure-random-bytes.
• raise should raise some descendant of the exn:fail structure type rather than a string to allow for proper exception handling. There's an exn:fail:unsupported structure type specifically for features that are unavailable due to platform configurations, ideally you could subtype that with some specific exn:fail:unsupported:secure-random structure type.
• Why is there a default number of bytes and why is it 4? I tend to see defaults you'll rarely rely on as a potential issue since it makes it easier to mis-call the function and get incorrect behavior without an exception. I don't think it's likely there's a universal default number of bytes all programs can make use of.
• Do all unix systems guarantee in all cases that reading dev/urandom won't return eof-object?? If so, ditch the check and just return (read-bytes n urandom), relying on the contract of secure-random to throw an exception if eof-object? is returned. It should never happen, but if it does something should blow up, rather than empty bytes being returned. Maybe an exception type for this specifically would be in order? Might not be worth it.
• If you rearrange the (if test body fail) expressions into something like (unless test fail) body ..., then you can use define instead of let to get rid of some nesting and make things clearer. Additionally, you can turn the (unless test fail) expressions into their own functions for readability. Example would be changing this:

(define (random-nix-bytes n)
  (if (file-exists? "/dev/urandom")
      (let* ([urandom (open-input-file "/dev/urandom")]
             [r (read-bytes n urandom)])
        (if (eof-object? r) ; we shouldn't ever hit EOF while reading from urandom
            #"" ; maybe an error should be raised here instead?
            r))
      (raise "ERROR: /dev/urandom does not exist")))

Into this:

(define (check-urandom-exists)
  (unless (file-exists? "/dev/urandom")
    (raise "ERROR: /dev/urandom does not exist")))

(define (random-nix-bytes n)
  (check-urandom-exists)
  (define urandom (open-input-file "/dev/urandom"))
  (define r (read-bytes n urandom))
  (if (eof-object? r) ; we shouldn't ever hit EOF while reading from urandom
       #"" ; maybe an error should be raised here instead?
      r))

• It may be a good idea to move the windows implementation into a separate module, then write this one in typed racket. Reason for the move of the windows part is because the FFI doesn't play nice with typed racket, I don't know if that would make this exceedingly difficult.
• The "unsupported OS" error message should include some information about what the current OS actually is, so that a user doesn't have to inspect the result of (system-type 'os) for themselves to see what kind of system racket thinks they have. This is especially useful if this error occurs somewhere the user doesn't have easy access to a repl, like an app running in the cloud on a black box where log output is all you've got for diagnosing a crash.

These are just my opinions, do not take them as absolutes. Let me know what you think.

Thank you very much for the comments. A couple of silly mistakes / left-over testing artifacts to clear up I didn't catch, good eye. Haha that default parameter cracks me up seeing it now, I don't know, it's way past my bed time :P

I tried writing a contract and got an error about the ->, I think the FFI's use of the -> symbol is messing with something.

I love the de-nesting of conditions into unless checks, definitely will do that.

The EOF thing was a last minute thought and definitely needs looking into. I have a hunch that the check is useless here.

Thanks for the notes on exceptions, still unfamiliar with that stuff.

OK thanks to @jackfirth, I've significantly restructured the program. Still need to change exception handling, and investigate unix EOF issue however.

Edit: on the subject on EOFs, as long as we're reading from /dev/urandom it does seem that we're guaranteed the number of bytes requested.

Note about the contract: positive? returns true for positive reals, false for nonpositive reals, and throws an exception for anything else. So if you give secure-random-bytes a string, for example, you won't get a contract failure - the contract itself will blow up with a positive?: expected real error. Additionally, positive? won't guard against non-integer inputs. You probably want exact-positive-integer?.

Updated, thanks.

reworked error handling in line with @jackfirth's suggestions. Hopefully I did that correctly.
Added crypto-random which returns an integer as opposed to a byte string
added optional argument to crypto-random-bytes that allows hex-string output - is this a good idea?
[and is there a better way to do bytes->number conversion?]

More minor thoughts:

• You could make a (raise-unsupported-error message) function in a private helper module that does (raise (make-exn:fail:unsupported message (current-continuation-marks))) to eliminate some duplication and increase readability.
• Instead of passing a flag to allow hex string output, it would be easier to have a crypto-random-hex-string function. Though that's probably not too useful to provide since it's just (compose bytes->hex-string crypto-random-bytes).
• For implementing crypto-random, it'd be more useful to do it in terms of a bytes->number function. I don't know a better way to make that function than by doing bytes->hex-string followed by string->number. Hand rolling your own in terms of bytes->list is a possibility but it would be messy and doesn't really belong with this feature.
• crypto-random's input n should behave the same way as random's and be an upper bound on the result number, not the number of bytes to use to generate the result number. This is trickier, but it's worth it since it makes crypto-random a drop-in replacement for random (in the one-argument case).

The exception raising looks good to me. I find it strange there isn't a built in bytes->number function to use. Maybe a separate PR should add that? Unless there's some good reason it's not provided.

I decided to remove the hex string option. bytes->hex-string makes it trivial for someone to convert the output of crypto-random-bytes into a hex string if they need it, so it doesn't warrant the added complexity.

a bytes->number would definitely be a useful addition to the bytes pkg.

your point about crypto-random's interface is spot-on. I'll have to think of how best to do this.