search

frasertweedale / hs-jose

Haskell JOSE and JWT library
http://hackage.haskell.org/package/jose
Apache License 2.0
122 stars 46 forks source link

Removing errors from API #25

Closed jkarni closed 7 years ago

jkarni commented 7 years ago

A lot of functions in the API of this package throw errors, when it seems like if there were tighter types they wouldn't need to. This matters a lot to me since I'm writing a library (servant-auth) where I end up having to expose users to a lot of Eithers - even if they're using keys or whatever that will never fail.

For example, toCompact - in the case JWS, I'd imagine parameterizing by a Traversable

data JWS t a = JWS Types.Base64Octets (t (Signature a))

with t as Identity, we know encoding will succeed w.r.t the number of signatures, as there is only one signature. Alternatively, a different datatype representing encodable JWSs could be created, though if the signature of the toCompact method were to change, the current JWS couldn't be an instance of ToCompact (which is intuitively correct, since a lot of them are not encodable as compact!).

Would a PR with this be considered, or is it too substantial an API change (or might there be issues I'm not aware of)?

frasertweedale commented 7 years ago

On Mon, Oct 10, 2016 at 04:35:09AM -0700, Julian Arni wrote:

A lot of functions in the API of this package throw errors, when it seems like if there were tighter types they wouldn't need to. This matters a lot to me since I'm writing a library (servant-auth) where I end up having to expose users to a lot of Eithers - even if they're using keys or whatever that will never fail.

For example, toCompact - in the case JWS, I'd imagine parameterizing by a Traversable

data JWS t a = JWS Types.Base64Octets (t (Signature a))

with t as Identity, we know encoding will succeed w.r.t the number of signatures, as there is only one signature. Alternatively, a different datatype representing encodable JWSs could be created, though if the signature of the toCompact method were to change, the current JWS couldn't be an instance of ToCompact (which is intuitively correct, since a lot of them are not encodable as compact!).

Would a PR with this be considered, or is it too substantial an API change (or might there be issues I'm not aware of)?

I'm definitely interested to see how this would work out in practice. Happy to adopt significant API changes but having just released v0.5 it would probably be a while before it gets released (if I do adopt it).

One area that would need to be worked out is what to do about newJWS, signJWS, etc. Perhaps preserve these functions for working with JWS [] and define a new function for JWS Identity, e.g.

singletonJWS
  :: (HasJWSHeader a, HasParams a, MonadRandom m, AsError e, MonadError e m)
  => ByteString -- ^ payload
  -> a          -- ^ header
  -> JWK        -- ^ signing key
  -> m (JWS Identity a)
frasertweedale commented 7 years ago

@jkarni so I did some work towards this change. See https://github.com/frasertweedale/hs-jose/compare/feature/jws-parameterise-sig-container for diff.

There are a couple of stumbling blocks:

  1. Although we can declare FromCompact and ToCompact instances for JWS Identity a only, we still cannot discard the error because compact serialisation cannot represent a JWS (or JWE) with an unprotected header. There is no doubt a way to express this in the types too but it's certainly not obvious; the design of headers in JOSE is a shambles IMO.

  2. The FromJSON and ToJSON instances provided by aeson do not do what we want (exemplified by the failing tests). I would have to define a new container type. I'd probably call it Singleton or something.

Comments?

frasertweedale commented 7 years ago

There isn't a straightforward way forward on this so I'm going to close it. If anyone has strong feels about it, or ideas, I'm happy to accept your patches or advice.

frasertweedale commented 7 years ago

I had an insight, and have worked out how to do it.

Please provide feedback on https://github.com/frasertweedale/hs-jose/compare/feature/jws-parameterise-sig-container

@sophie-h could you please test this also. I'd like to include it in the upcoming release. There are some breaking changes; from user point of view this is a new type parameter for JWS, removal of newJWS, and changing signJWS to take the payload and a traversable of (header, JWK) all in one hit.

sophie-h commented 7 years ago

I might need a few days to look into that.

frasertweedale commented 7 years ago

On Fri, Mar 31, 2017 at 04:41:05AM -0700, Sophie Herold wrote:

I might need a few days to look into that.

No worries; take your time.

sophie-h commented 7 years ago

First observation: I think the "additional header parameters" example is failing with signJWS due to the new Cons s s Word8 Word8 requirement for the payload.

sophie-h commented 7 years ago

Ah, just forgot to change the payload to bytestring. That was easy to fix ;)

sophie-h commented 7 years ago

Everything works as expected for me. Looking forward to 0.6!

frasertweedale commented 7 years ago

On Wed, Apr 05, 2017 at 01:53:50AM -0700, Sophie Herold wrote:

Everything works as expected for me. Looking forward to 0.6!

Awesome, thanks for testing! I will release later this week.

frasertweedale commented 7 years ago

Fixed in f0aa0c7a5d63a2e63cb1a747a42fa0d64a4dfda2