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

Use Jose #31

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Use Jose #31

wants to merge 5 commits into from

Conversation

ulrikstrid
Copy link

@ulrikstrid ulrikstrid commented Feb 8, 2023

Needs a release of jose with the changes (ulrikstrid/ocaml-jose#56). Release is here: ocaml/opam-repository#23366

Since most of the tests are already in Jose we can remove them from here and I should also cleanup the code a bit extra to remove unsafe things.

The test data is changed because Jose generates slightly different headers

The two big changes (except for the obvious code removal) is:

  • We're moving to Yojson.Safe.t instead of Yojson.Basic.t.
  • The internal key is now a Jose.Jwk.priv Jose.Jwk.t instead of a X509.Private_key.t. This is easily changed back however and we can hide that detail if we want to.

@ulrikstrid ulrikstrid force-pushed the ulrikstrid--jose branch 2 times, most recently from 81aad67 to 86dfcb0 Compare February 17, 2023 14:54
@@ -8,7 +8,7 @@ let sha256_and_base64 a = Primitives.sha256 a |> B64u.urlencode

let ( let* ) = Result.bind

module J = Yojson.Basic
module J = Yojson.Safe
Copy link
Author

Choose a reason for hiding this comment

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

This is the other big change, moving from Basic to Safe since that's what is "supposed" to be used and also what Jose uses.

@ulrikstrid ulrikstrid marked this pull request as ready for review February 17, 2023 14:59
@dinosaure
Copy link
Contributor

I'm not against deleting code but your PR adds a new dependency to letsencrypt. Is there any particular reason why we should use Jose instead of internal code (a feature we don't handle for example due to the limitation of our implementation)?

@ulrikstrid
Copy link
Author

ulrikstrid commented Mar 21, 2023

I understand your point of view, Jose doesn't have any dependencies we don't already have in this package I think so it's a minimal dependency to add so to speak. We have greater support for different keys in Jose that is currently supported here. And from a personal point of view it's good to de-duplicate code across the ecosystem since it's non-trivial. :)

@hannesm
Copy link
Collaborator

hannesm commented Mar 21, 2023

Thanks @ulrikstrid for the PR. I also think that deduplicating code is a good path forward. Unfortunately I've not found the time to review this PR (and look whether it increases dramatically build times or binary sizes).

@ulrikstrid
Copy link
Author

No problem @hannesm, Jose had landed the needed improvements and I fixed some mirage-unfriendlyness while I was at it.
I can probably create a small app to see if there is a big size difference, I don't expect there to be much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants