Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Sodium monad #77

Closed
wants to merge 2 commits into from
Closed

Add Sodium monad #77

wants to merge 2 commits into from

Conversation

olorin
Copy link
Contributor

@olorin olorin commented Dec 28, 2016

I don't like this monad much, but it's the nicest way I can think of to ensure sodiumInit gets called before everything that needs it (without handling init errors individually everywhere). Other suggestions very welcome.

! @thumphries @erikd-ambiata

@thumphries
Copy link
Contributor

This approach seems reasonable, given init is idempotent and threadsafe. 🍯

If I were using it I'd probably want a transformer and MonadIO instance, but that'd let me interleave stupid IO things that could affect timing, so maybe it is a good idea to not provide those.

Does the library require any cleanup? or does it just leave stuff in the C heap indefinitely

@olorin
Copy link
Contributor Author

olorin commented Dec 29, 2016

Does the library require any cleanup? or does it just leave stuff in the C heap indefinitely

sodium_init() doesn't allocate anything on the heap. It does create one file descriptor (to /dev/urandom) which it doesn't clean up; I'm happy to let the kernel take care of that one. Will add a comment.

If I were using it I'd probably want a transformer and MonadIO instance, but that'd let me interleave stupid IO things that could affect timing, so maybe it is a good idea to not provide those.

That's my reasoning too, or part of it. The other part is that I want to keep the surface area of the codebase as minimal/simple as possible, to reduce the complexity of answering "is this thing going to behave sanely in all the ways I can imagine someone using it" for new additions - afaik there's no particular problem with deriving say MonadIO here, I just know I'm not going to need it in the immediate future. I suspect it will end up not being exposed to library consumers, the high-level operations will be things like encryptFile :: SecretKey -> SourcePath -> DestPath -> EitherT FileEncryptError IO () (or a streaming interface or whatever).

@thumphries
Copy link
Contributor

@erikd-ambiata
Copy link

👍

@charleso
Copy link
Contributor

charleso commented Jan 2, 2017

Another suggestion would be to have a dummy data type with a hidden constructor, which can only be created by init, and is required by all libsodium functions.

@olorin olorin changed the base branch from topic/c2 to master January 3, 2017 03:40
@olorin
Copy link
Contributor Author

olorin commented Jan 3, 2017

Another suggestion would be to have a dummy data type with a hidden constructor, which can only be created by init, and is required by all libsodium functions.

Thanks, I didn't think of doing that. Tempted to go with this instead of the monad, thoughts @erikd-ambiata @thumphries?

@thumphries
Copy link
Contributor

Tempted to go with this instead of the monad, thoughts

If users will be making isolated calls to individual Sodium functions, it's not so bad. It does communicate pretty well the fact that you need to set up some abstract FD to use the library. It's nice to make this explicit vs hiding it in some typeclass mechanism. If they will actually be using (>>=), explicit baton-passing will be mildly annoying.

However, that kinda inverts the design of libsodium, which goes out of its way to have a single global FD that you init early and often. Repeated invocations of init are pretty much free. Passing our own baton around serves mostly rhetorical ends.

tldr I dunno both are good

@olorin
Copy link
Contributor Author

olorin commented Jan 3, 2017

If users will be making isolated calls to individual Sodium functions

Users shouldn't be seeing this at all, regardless of how it's implemented; it's not going to be exported outside Tinfoil.Internal.*. The external interface should just be an EitherT IO; probably one of the error constructors will be TinfoilFailedToInitSodium or something, and the exported functions will each explicitly have sodium init + error handling as part of the control structure.

However, that kinda inverts the design of libsodium, which goes out of its way to have a single global FD that you init early and often. Repeated invocations of init are pretty much free. Passing our own baton around serves mostly rhetorical ends.

Yep I agree with that. The rhetorical end I have in mind is explicitness and clarity: if I see a function like frobnicateWidgets :: Widgets -> MonadFrobnication Widgets, where MonadFrobnication implements MonadIO, or (more relevant example) derivePublicKey :: SecretKey -> Sodium PublicKey, I can't tell from the type what kind of magic is happening when I call it. Compare derivePublicKey :: SodiumIsInitialised -> SecretKey -> PublicKey: I know there's something I need to do before I call the function, but I also know I've got referential transparency (no randomness required for generating a public key) et cetera. Does that make sense? I guess I could achieve the same thing by making Sodium a monad transformer.

@thumphries
Copy link
Contributor

If you are passing a SodiumIsInitialised baton around, would the subsequent functions be in IO or EitherT SodiumError IO? If the former, that's pretty cool.

@olorin
Copy link
Contributor Author

olorin commented Jan 3, 2017

If you are passing a SodiumIsInitialised baton around, would the subsequent functions be in IO or EitherT SodiumError IO? If the former, that's pretty cool.

They won't be EitherT SodiumError IO (I should rename that type SodiumInitError come to think of it). I don't get a free pass out of handling library errors, so everything which calls out to a libsodium function which can fail (most of them) will be in some kind of EitherT FooError IO or similar. I could do sodiumGenRandomWord32 :: SodiumInitMarker -> IO Word32 though (maybe for #38).

@erikd-ambiata
Copy link

I don't have any strong opinions either way, but I am slightly in favour of the monad solution, but thats mainly because it avoids explicit baton passing.

@olorin
Copy link
Contributor Author

olorin commented Jan 4, 2017

Closing in favour of #78.

@olorin olorin closed this Jan 4, 2017
@olorin olorin deleted the topic/c6 branch January 4, 2017 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants