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

bindings/crypto-nodejs: Expose function to sign arbitrary objects #797

Closed
Tracked by #236
turt2live opened this issue Jun 29, 2022 · 13 comments · Fixed by #803
Closed
Tracked by #236

bindings/crypto-nodejs: Expose function to sign arbitrary objects #797

turt2live opened this issue Jun 29, 2022 · 13 comments · Fixed by #803
Assignees
Labels
bindings enhancement New feature or request

Comments

@turt2live
Copy link
Member

This was available off the OlmMachine in the old bindings: https://github.com/matrix-org/matrix-rust-sdk-bindings/blob/main/crates/matrix-sdk-crypto-nodejs/src/machine.rs#L375

@turt2live turt2live changed the title feat(bindings/crypto-nodejs): Expose function to sign arbitrary objects bindings/crypto-nodejs: Expose function to sign arbitrary objects Jun 29, 2022
@Hywan Hywan self-assigned this Jun 30, 2022
@Hywan Hywan added enhancement New feature or request bindings labels Jun 30, 2022
@poljar
Copy link
Contributor

poljar commented Jun 30, 2022

Out of interest, why is this needed? What does the bot-sdk sign with this?

@turt2live
Copy link
Member Author

It signs various requests on behalf of the consumer for ease of use - mostly key uploads at the moment, but potentially arbitrary signing in the future (as part of MSC work).

@poljar
Copy link
Contributor

poljar commented Jun 30, 2022

Hmm, but /keys/upload requests are already signed by the machine. Are those some other keys you are talking about?

@turt2live
Copy link
Member Author

the bot-sdk exposes a function for calling /upload without going through the machine - it's a bit complicated, but it's for a weird usecase.

The eventual plan is to use it for some upcoming MSC work though rather than /upload

@poljar
Copy link
Contributor

poljar commented Jun 30, 2022

So you're signing keys that the machine didn't provide using keys that the machine did provide? This doesn't sound like something I would like to support. What's the use-case here?

What's the MSC about?

@turt2live
Copy link
Member Author

So you're signing keys that the machine didn't provide using keys that the machine did provide? This doesn't sound like something I would like to support. What's the use-case here?

it's the bot-sdk supporting it, not the bindings. We just need the function exposed.

What's the MSC about?

There's plenty of opportunities for device-signed events that haven't been written up yet, like account portability, membership, voip, etc. These are very much scifi at the moment.


(this feels all a bit academic considering a PR is already open?)

@poljar
Copy link
Contributor

poljar commented Jun 30, 2022

So you're signing keys that the machine didn't provide using keys that the machine did provide? This doesn't sound like something I would like to support. What's the use-case here?

it's the bot-sdk supporting it, not the bindings. We just need the function exposed.

Well yes and no, any broken crypto in the bot-sdk will likely end up opening an issue in the rust-sdk repo. Is there a reason why you're not willing to share the use-case? Being mysterious about this feature makes me even more worried and pushes me further towards not supporting this.

(this feels all a bit academic considering a PR is already open?)

Exposing this is a ~3 line patch, it was never about the work that is needed to add support for this, the problem is that this is a feature which might be misused and end up making more trouble than it's worth it.

@turt2live
Copy link
Member Author

It's not exactly context I wish to share on this forum, and isn't really relevant to the request at hand. It's not likely to show up as a bug report on the bindings.

@turt2live
Copy link
Member Author

(I can see how it's extremely questionable for the bot-sdk to offer functions which both use and don't use the OlmMachine though)

@turt2live
Copy link
Member Author

having finally been able to test the PR though, I don't think the bot-sdk needs the signing functions for /upload anymore - just for stuff that's not currently written into MSC form.

@poljar
Copy link
Contributor

poljar commented Jul 1, 2022

having finally been able to test the PR though, I don't think the bot-sdk needs the signing functions for /upload anymore - just for stuff that's not currently written into MSC form.

That gives me at least some relief.

@Hywan
Copy link
Member

Hywan commented Jul 1, 2022

Should I close #803 then?

@turt2live
Copy link
Member Author

*merge, yes ;)

@Hywan Hywan closed this as completed in #803 Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bindings enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants