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

Initial Asymmetric Key support #48

Closed
wants to merge 50 commits into from
Closed

Conversation

bblfish
Copy link

@bblfish bblfish commented Dec 31, 2021

This is a PR for asymmetric key support.

@bblfish
Copy link
Author

bblfish commented Dec 31, 2021

I can see the Java Test Results now. But I could not find the ScalaJS browser based ones.

@bblfish bblfish reopened this Dec 31, 2021
@bblfish
Copy link
Author

bblfish commented Jan 3, 2022

This is getting close to something that could be checked in (It's up to maintainers to decide how to proceed).
I will see how well this works by using it in my "Signing HTTP Messages" implementation https://github.com/bblfish/httpSig .

@armanbilge
Copy link
Member

armanbilge commented Jan 3, 2022

This is getting close to something that could be checked in

Thanks, that's fantastic! I think there's still a couple things missing:

  1. Node.js implementation.
  2. Removing the dependency to scala-js-dom and internalizing the facades, as previously discussed. Do you have any thoughts on this? The reason I would prefer to avoid this dependency is because it has historically caused problems when used with Node.js and generally does not make sense to be used in a Node.js library since it describes browser APIs. Since the goal of bobcats is to provide cross-platform implementations across JVM, Node.js, and browsers, internalizing the facades seems like the safest strategy.

Besides that, this PR is very large. Probably more code here than in the entire repository right now 😉 rather than review it all at once, I will do it in the following order:

  1. Changes to the Key hierarchy, to add support for asymmetric keys.
  2. The new interfaces for signing/verifying.
  3. JVM/browser/Node.js implementations.

In fact, if you wouldn't mind splitting the PR into smaller PRs, then we could even merge each part as it's reviewed. (1) and (2) deserve a focused review, because they are introducing new public APIs. (3) should be straightforward implementations and you are very thorough with tests, so I have less concerns there.

Let me know what you think. Thanks again for the tremendous effort here, this is a great addition to this library :)

@bblfish
Copy link
Author

bblfish commented Jan 3, 2022

I think it would be quite difficult to cut this into smaller PRs because these work as a whole with the test suite. Signing and verifying just go together. Thinking of this as a sculpture: one has to first get the general form and then hack away to get the details smoothed out.

Would it work if I made a PR to a branch on this repo, and then you could make the changes the way you see fit? I could then comment on those as future PRs with feedback on how it fits with the httpSig implementation I am writing.

You are right the Node.js implementation is missing. I have nearly no experience of node.js . I think for someone who does that would be very easy - now that the tests are in place.

The API definitely needs to be looked at carefully. What I did got things going which is useful as it helps one test better ideas.

I don't have an opinions on using scala-js-dom or internalising the API. I just used the dom to reduce my work (if I had added that the PR would be even bigger :-) But it should be easy I think to internalise it and even to improve it.

I think you have experience with that so you'll find that quite easy to do. It could also be added as a todo in the issue DB for a new PR, just as node js support could.

My best contribution I think would be to start using it to see what is missing when used.

I think one should call this "gestalt programming": get the whole working, then refine the smaller pieces, once the whole is understood to work.

@armanbilge
Copy link
Member

I think it would be quite difficult to cut this into smaller PRs because these work as a whole with the test suite. Signing and verifying just go together.

I agree, I didn't mean to suggest that we separate signing and verifying :) but rather that we focus separately on interface/API and implementation/testing, since I believe those are the two important parts of this PR. As you say yourself, the API deserves a careful look, and I thought that may be best served in its own PR, of course taking all the learnings from the "gestalt programming" you have done so diligently :)

You are right the Node.js implementation is missing. I have nearly no experience of node.js . I think for someone who does that would be very easy - now that the tests are in place.

I think you have experience with that so you'll find that quite easy to do. It could also be added as a todo in the issue DB for a new PR, just as node js support could.

It sounds like you won't be able to work on this, which is no problem 👍 unfortunately, I will not have time to work on this right now.

Would it work if I made a PR to a branch on this repo, and then you could make the changes the way you see fit?

We can keep your PR open, and it can be picked up at any time in the future by you, me, or anyone else.

My best contribution I think would be to start using it to see what is missing when used.

That sounds great :)

@bblfish
Copy link
Author

bblfish commented Jan 11, 2022

The last commit adds Ed25519 support only for Java as discussed in issue #58
It can be removed later if needed. I just wanted to see how far it works and point the HTTP-WG team to a commit.

@bblfish
Copy link
Author

bblfish commented Feb 9, 2022

The branch for this PR is at https://github.com/bblfish/bobcats and needs to be updated with recent changes here.
I published artifacts for that branch (see README) which is being used by https://github.com/bblfish/httpSig

I'll close this here, and re-open an PR when I succeed it making the branches mergeable again.

@bblfish bblfish closed this Feb 9, 2022
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.

None yet

3 participants