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

Better verifier results #12

Merged
merged 4 commits into from
Nov 9, 2018

Conversation

clementd-fretlink
Copy link
Collaborator

@clementd-fretlink clementd-fretlink commented Oct 31, 2018

I gave a shot at improving the verifiers API, as per #11
I'll try to split up changes in separated commits. There is one commit for imports cleanup, don't pay it too much attention.

ToDo:

  • split up ValidationError
  • drop the monadIO constraint
  • investigate the use of a monad transformer for effects and validation -> Later
  • investigate letting verifiers access all caveats at the same time (through eg. a zipper) -> Later

First change: split up ValidationError into:

  • VerifierResult (for… well… verifier results).
  • VerifierError for caveats understood, but not discharged
  • ValidationError for macaroon validation errors (either an issue with the macaroon itself, or with caveats discharge. Caveat discharge errors are accumulated in a NonEmpty (Caveat, [VerifierError]) case, which allows to know which caveats failed and why.

This allows to drop the Monoid ValidationError previously necessary for error accumulation. It was lossy, and some combinations didn't make sense (to the point that the first <> implementation was partial). The caveats validation is now a bit cleaner (even though keeping the caveats around makes it more complex than it could be).

Second change: relax the unused MonadIO constraint. This allows to have synchronous verification thanks to Identity (I've added a helper to make it even easier).

Third change: introduce a monad transformer to encapsulate both the effects needed for verifier computation and for the verifier results (maybe through a MonadError constraint). I'm not convinced it's really necessary, as it would increase complexity a lot, while providing little benefit IMO (macaroon validation errors should be treated right away, not carried on in the context).

@clementd-fretlink clementd-fretlink force-pushed the better-verifier-results branch 2 times, most recently from 5f730d2 to 560b2f8 Compare October 31, 2018 14:59
@clementd-fretlink
Copy link
Collaborator Author

There are two things I'm not 100% sure of with the new VerifierResult ADT.

  • should the error value be Text or can we make it parametric?
  • should OutstandingCaveat carry the whole caveat or just its id? Currently it carries the whole caveat, and it requires a small cleanup function in tests. In application code, I think it's less an issue though.

Copy link
Owner

@jtanguy jtanguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an aside, and related to #1, I wonder if the proposed structure can accommodate attaching discharge macaroons to the result, and recursively verify all the macaroons.

I think it is feasible without the need for a complete refactor.

src/Crypto/Macaroon/Verifier/Internal.hs Outdated Show resolved Hide resolved
src/Crypto/Macaroon/Verifier/Internal.hs Show resolved Hide resolved
Copy link
Owner

@jtanguy jtanguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that OutstandingCaveat should bear the hole caveat.

For first-party caveats this is equivalent to storing just the id, but for third-party it could be useful to group the non-discharged caveats by origin

src/Crypto/Macaroon/Verifier/Internal.hs Show resolved Hide resolved
@clementd-fretlink
Copy link
Collaborator Author

We can leave 3rd party caveats for another PR I think

This allows to get rid of an ill-defined monoid instance as well
as providing more detailed information about the validation errors,
while structurally eliminating impossible cases.
This lets the users use arbitrary monads (including `MonadIO` instances),
but this also allows to use `Identity`. A `verifySync` helper is provided
to have synchronous verification, freeing the user to use `Identity` themselves
@clementd-fretlink
Copy link
Collaborator Author

clementd-fretlink commented Nov 2, 2018

I've renamed OutstandingCaveats? (squashed in the first commit), and made VerifierError parametric in a separate commit. Doing so make the types a bit more messy (it shows in the tests, where I've used type aliases and TypeApplications).

I've tried to keep the new extensions to a minimum, this PR currently adds:

  • StandaloneDeriving to keep deriving Eq and Show on parametric result types
  • ScopedTypeSignatures to add intermediate annotations on the caveats check function
  • TypeApplications (only in the tests) to avoid splitting a small one liner.

@clementd-fretlink clementd-fretlink changed the title [WIP] Better verifier results Better verifier results Nov 2, 2018
@clementd-fretlink
Copy link
Collaborator Author

I've thought a bit more about a monad tranformer and zippers, I don't have a clear view of what's going to be possible yet, so I'll keep to the current scope for this PR.

I have a few ideas for both transformers and zippers, I can discuss them in relevant issues.

@clementd-fretlink
Copy link
Collaborator Author

I've opened issue #13 to sum up my ideas about zippers.

@jtanguy jtanguy mentioned this pull request Nov 4, 2018
@clementd-fretlink
Copy link
Collaborator Author

@jtanguy so, what's left to do?

@jtanguy
Copy link
Owner

jtanguy commented Nov 6, 2018

There is still an unchecked TODO in the PR description.
The zipper idea has been extracted to issue #13

What about the last item ? Is it tracked somewhere ? If so, we can merge this and then #14

@jtanguy jtanguy merged commit 28c239a into jtanguy:master Nov 9, 2018
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.

2 participants