changed crypto/olm to use goolm (no cgo needed anymore)

[DerLukas15]

Hello.
I was in need for a matrix client written in go. I saw your package and wanted to use it, but I didn't want any c bindings (usage of cgo). To fix this issue I created goolm which is a pure go implementation of libolm. I took some inspiration from your crypto/olm package and included it in goolm so that the integration in your package was easier.

Goolm uses the same pickle algorithm as libolm. So this change should work with existing sessions which were pickled with libolm. However, I've not checked that. Most of the tests included in libolm have been directly ported to goolm.

I've been running my client with your package and goolm for some time now and it works fine so far. So now I want to contribute something back to you.

If you prefer to use libolm, I could also change the PR to use goolm only with a build tag (e.g. goolm) and keep libolm as the default.

Please let me know what you think.

[tulir]

Looks interesting, I've wanted to build that myself for a while but never got around to it.

Definitely can't make it the default right away though, that's a very sensitive part of the code

[DerLukas15]

I agree that this change should still be tested and is truly a sensitive part. I've changed the code to again include your libolm implementation and moved the goolm part into seperate files with the build tag 'goolm'.
Your checks should not fail, as they are not using this build tag.

Maybe a short message could be added to the README if this PR is to be merged. Something like:

Usage without CGO (experimental)

Instead of depending on libolm with CGO, an alternative pure go implementation of olm can be used by specifying the build tag goolm.

Should I add this?

[mraerino]

any news on getting this merged?

[mraerino]

i've been trying this with a bot i've been building and it seems to work fine btw

thanks for implementing it!

[meschbach]

Pulled the PR and test with go test --tags=goolm ./... from the repo root with current master. Results in a test failure:

--- FAIL: TestRatchetMegolmSession (0.00s)
    machine_test.go:65: 
                Error Trace:    mautrix-go/crypto/machine_test.go:65
                Error:          Not equal: 
                                expected: 0xa
                                actual  : 0x0
                Test:           TestRatchetMegolmSession

A current state of the merge is at https://github.com/meschbach/mautrix-go . Assuming this does not happen on master since the tests pass, however I can not verify since I do not have C OLM installed. Looks related to InboundGroupSession.RatchetTo however I am not familiar enough to track it down.

Could this be related to Internal.Export changing from string to []byte ?

[DerLukas15]

--- FAIL: TestRatchetMegolmSession (0.00s)
    machine_test.go:65: 
                Error Trace:    mautrix-go/crypto/machine_test.go:65
                Error:          Not equal: 
                                expected: 0xa
                                actual  : 0x0
                Test:           TestRatchetMegolmSession

The error is now fixed. There was a missing initialization of the initial ratchet on an inbound megolm session in goolm.

Could this be related to Internal.Export changing from string to []byte ?

I fixed this too and merged the current master as well.
Thank you for your report!

[meschbach]

Thank you for making those changes! 🥇

Tested with a derivative of the code in example/main.go with the tag goolm and worked like a charm. 🥳 🍾 🎆

[tulir]

@DerLukas15 do you mind if I just copy goolm into mautrix-go? Circular dependencies in go.mod aren't nice even though they technically work, and there's a bunch of code quality stuff that needs improving.

[DerLukas15]

Please go ahead with the integration.
I tried to keep the code structure close to libolm for a better comparison. It makes sense to improve the overall structure of goolm if that is not needed and you are doing the work of integration anyway.
Please be so kind to keep a reference/link to the goolm package in a comment in the code somewhere.

[tulir]

Merged the base implementation plus a bunch of tweaks (fixing typos, underscores in JSON field names, switching to base64.RawStdEncoding instead of implementing it manually, reverting unnecessary changes to error names, dropping usage of github.com/pkg/errors in favor of the stdlib errors package).

I think the remaining things are:

• Changing the goolm interface to prefer raw bytes instead of base64 everywhere possible, and only doing base64 en/decoding at the edges (probably mostly the olm package that calls goolm)
  • Includes removing the base64 functions that return bytes, should just return strings if the return value is base64.
• Switch methods to use pointer receivers so they wouldn't make copies
• Maybe switch the private key []byte types to fixed length arrays

[DerLukas15]

I'd give those remaining tasks a try if you don't mind.

Another thing which I noticed now:
We could change the crypto package to use interfaces instead of direct calls to the crypto/olm package. Than it is really simple to add potentially other olm-wrappers in the future. Furthermore the call to goolm doesn't have to go through crypto/olm and those wrapper functions in the *_goolm.go files are not needed anymore. What do you think about this? Should I open a new issue for this?

[tulir]

Hmm yeah, that could make sense. It'd probably be cleaner even if it didn't reduce the amount of code. Someone might want to use vodozemac too (I don't because rust<->go interop is not nice, but having interfaces to make it possible to implement would be good)