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 verifiers #11

Open
jtanguy opened this issue Sep 27, 2018 · 11 comments
Open

Better verifiers #11

jtanguy opened this issue Sep 27, 2018 · 11 comments
Milestone

Comments

@jtanguy
Copy link
Owner

jtanguy commented Sep 27, 2018

The verifier logic could benefit from a small refactoring, mainly from two things:

  • Remove the MonadIO m requirement and introduce a VerifierM monad transformer.
  • Allow verifiers to access multiple caveats in order to verify a single one. Something like the ListZipper from this article from Dave Laing about Comonads

What are your thoughts about it @ptitfred @clementd-fretlink ?

@clementd-fretlink
Copy link
Collaborator

The MonadIO constraint is not that terrible in practice (a pure version alongside the IO would still be nice). I'm not sure what a new monad transformer would bring.

For verifiers that need access to other caveats, I first build verifiers by inspecting caveats, which works okay. Maybe giving access to the other verifiers in a zipper fashion would be nice indeed.

My main gripe with the verifiers currently is that VerifierResult embeds too many cases (macaroon-level errors), so its monoid instance has quite nonsensical combinations (that's why the implementation was partial in the first case).

@clementd-fretlink
Copy link
Collaborator

Something like

data VerifierResult =
    Unrelated
  | Refused Text
  | Verified

would improve things a lot. Having a separate type for macaroon validation (with SigMismatch, and RemainingCaveats (NonEmpty (Caveat, [Text]))) would allow to improve UX

@clementd-fretlink
Copy link
Collaborator

Another idea (even though I realize it's a bit more far-fetched) would be to add Deferred to the verifier result, as well as a Deferred (NonEmpty Caveat) case to the macaroon validation result, to allow multiple-step validation.

@clementd-fretlink
Copy link
Collaborator

@jtanguy what do you think about these ideas?

@jtanguy
Copy link
Owner Author

jtanguy commented Oct 23, 2018

I do like the changes to the VerifierResult. I am less sure about the Deferred/RemainingCaveats part because I believe there may be a way to statically define verifiers as long as they can inspect the caveat structure (and the previous/next caveats the ListZipper offers)

@jtanguy
Copy link
Owner Author

jtanguy commented Oct 23, 2018

That would be a departure from the rest of the macaroons libraries (which I'm not specifically against), and so if the need to have multiple-step macaroon verification arises, that would be a good opportunity to contribute back these propositions to the broader macaroons community.

@clementd-fretlink
Copy link
Collaborator

Yeah the Deferred thingy is a departure from the other implementations. Its need arose in practice, as there was a natural distinction between "technical" and "business" caveats. No problem keeping out of the library

@jtanguy
Copy link
Owner Author

jtanguy commented Oct 30, 2018

I just would like this to be opt-in

@clementd-fretlink
Copy link
Collaborator

clementd-fretlink commented Oct 31, 2018

I'm trying out the new data types I've proposed, it works quite well and it allows a significant cleanup of the validation code.

@fmenou
Copy link

fmenou commented Oct 31, 2018

That would be a departure from the rest of the macaroons libraries (which I'm not specifically against), and so if the need to have multiple-step macaroon verification arises, that would be a good opportunity to contribute back these propositions to the broader macaroons community.

Its need arose in practice, as there was a natural distinction between "technical" and "business" caveats

Those 2 quotes let me believe we should add examples in the library to illustrate those various use-cases, don't you both think?

@clementd-fretlink
Copy link
Collaborator

You may find it funny, but I've given some more thought to this, and I don't think that multi-step validation is necessary for our use case anymore (since we're already accumulating verifiers). I'll give a shot at merging the two steps to see how it goes.

@jtanguy jtanguy mentioned this issue Oct 31, 2018
7 tasks
@jtanguy jtanguy added this to the 1.0 milestone Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants