WIP: Implement XEP-0420 Stanza Content Encryption #385

Draft
jaeckel wants to merge 2 commits from xep-0420 into main
Collaborator

This adds support for XEP-0420 Stanza Content Encryption.

The Encryption part is generic via a SceEnvelope. The Decryption part is SCE profile specific and therefore done via a XSFOMEMO2SceEnvelope that expects the XML to be formatted as specified in the OMEMO:2 XEP-0384 v0.8.0 upwards.

Please review and let me know what you think should be improved.

This adds support for XEP-0420 Stanza Content Encryption. The Encryption part is generic via a `SceEnvelope`. The Decryption part is SCE profile specific and therefore done via a `XSFOMEMO2SceEnvelope` that expects the XML to be formatted as specified in the `OMEMO:2` XEP-0384 v0.8.0 upwards. Please review and let me know what you think should be improved.
jaeckel added 3 commits 2023-09-10 16:25:39 +00:00
90650e6716 all: expect go1.20
This is kind of a fixup of b95dbca3c3

Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>
dfd6441528 sce: add initial version of XEP-0420 Stanza Content Encryption
Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>
ci/woodpecker/push/dco Pipeline was successful Details
ci/woodpecker/push/test Pipeline failed Details
ci/woodpecker/push/integration unknown status Details
ci/woodpecker/push/validate Pipeline failed Details
ci/woodpecker/pr/dco Pipeline was successful Details
ci/woodpecker/pr/test Pipeline failed Details
ci/woodpecker/pr/integration unknown status Details
ci/woodpecker/pr/validate Pipeline failed Details
7993f09e2e
sce: implement Stanza Encryption and Decryption
This adds a generic encryption method for XEP-0420.
As the decryption is SCE Profile specific, there's currently only an
implementation for OMEMO based on XEP-0384 v0.8.0 upwards, i.e. `omemo:2`.

Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>
jaeckel force-pushed xep-0420 from 7993f09e2e to 8a9d567c4f 2023-09-10 16:26:55 +00:00 Compare
jaeckel added 1 commit 2023-09-10 16:29:56 +00:00
ci/woodpecker/push/dco Pipeline was successful Details
ci/woodpecker/pr/dco Pipeline was successful Details
ci/woodpecker/push/validate Pipeline failed Details
ci/woodpecker/pr/validate Pipeline failed Details
ci/woodpecker/push/test Pipeline failed Details
ci/woodpecker/push/integration unknown status Details
ci/woodpecker/pr/test Pipeline failed Details
ci/woodpecker/pr/integration unknown status Details
8e1149c010
all: update dependencies after running `go mod tidy`
Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>
jaeckel force-pushed xep-0420 from 8e1149c010 to 60a3933fbf 2023-09-10 16:33:35 +00:00 Compare
jaeckel force-pushed xep-0420 from 60a3933fbf to 7eaa494c60 2023-09-10 16:43:43 +00:00 Compare
jaeckel force-pushed xep-0420 from 7eaa494c60 to 06bbcafc70 2023-09-10 17:47:52 +00:00 Compare
jaeckel force-pushed xep-0420 from 06bbcafc70 to 778e1a74d2 2023-09-10 17:53:45 +00:00 Compare
jaeckel force-pushed xep-0420 from 778e1a74d2 to e24979aa01 2023-09-10 18:24:32 +00:00 Compare
jaeckel force-pushed xep-0420 from e24979aa01 to 1a523b31a6 2023-09-10 18:46:08 +00:00 Compare
jaeckel requested review from SamWhited 2023-09-10 19:58:06 +00:00
SamWhited reviewed 2023-09-11 22:21:42 +00:00
Makefile Outdated
@ -1,5 +1,5 @@
.POSIX:
.SILENT:
#.SILENT:

Let's drop the makefile and go.mod changes and discuss them separately if we want them; it helps to keep PRs small and tidy (if we do want this change I have some notes, but we can discuss later).

Let's drop the makefile and go.mod changes and discuss them separately if we want them; it helps to keep PRs small and tidy (if we do want this change I have some notes, but we can discuss later).
Poster
Collaborator

OK, I'll open a separate PR with only those changes and will base this PR then on top of that one.

OK, I'll open a separate PR with only those changes and will base this PR then on top of that one.

That works too, but I'm not sure we need these changes at all. It just seems to be complicating the make file. If we do add this rule, it needs to be marked as .PHONY since it doesn't create any files. Also, we shouldn't comment out the .SILENT (you can always do it locally if you need to debug, or we can discuss removing it entirely if you want, but overall I just think it makes the experience of using Make nicer)

As for the go.mod file, why update it at all? is this needed for something?

That works too, but I'm not sure we need these changes at all. It just seems to be complicating the make file. If we do add this rule, it needs to be marked as .PHONY since it doesn't create any files. Also, we shouldn't comment out the `.SILENT` (you can always do it locally if you need to debug, or we can discuss removing it entirely if you want, but overall I just think it makes the experience of using Make nicer) As for the go.mod file, why update it at all? is this needed for something?
Poster
Collaborator

[...] we shouldn't comment out the .SILENT (you can always do it locally if you need to debug, or we can discuss removing it entirely if you want, but overall I just think it makes the experience of using Make nicer)

I accidentally commited that one.

As for the go.mod file, why update it at all? is this needed for something?

That was required for the external dependency, but it's not required anymore now.

> [...] we shouldn't comment out the `.SILENT` (you can always do it locally if you need to debug, or we can discuss removing it entirely if you want, but overall I just think it makes the experience of using Make nicer) I accidentally commited that one. > As for the go.mod file, why update it at all? is this needed for something? That was required for the external dependency, but it's not required anymore now.

Sounds good! Let's just drop these changes for now then. If you want the extra makefile recipe, feel free to open that as a separate PR and we can discuss out of band.

Sounds good! Let's just drop these changes for now then. If you want the extra makefile recipe, feel free to open that as a separate PR and we can discuss out of band.
jaeckel marked this conversation as resolved
SamWhited requested changes 2023-09-11 22:40:00 +00:00
SamWhited left a comment
Owner

This is a very quick pass for a few stylistic things. I haven't reviewed functionality or API at all, so no rush to make changes in case you end up making changes to a bunch of things that end up changing or going away later :)

I'll try to do a longer review as soon as I can; sorry for the delays.

This is a *very* quick pass for a few stylistic things. I haven't reviewed functionality or API at all, so no rush to make changes in case you end up making changes to a bunch of things that end up changing or going away later :) I'll try to do a longer review as soon as I can; sorry for the delays.
crypto/rand.go Outdated
@ -0,0 +11,4 @@
// Intn returns, as an int, a non-negative random number in the half-open interval [0,n).
// It panics if n <= 0.
func Intn(n byte) int {

This probably isn't something we want to expose publicly. If we need it as a helper function in multiple packages we can put it in internal/whatever somewhere, otherwise can we just leave it unexported and use it in whatever package we need it in?

This probably isn't something we want to expose publicly. If we need it as a helper function in multiple packages we can put it in `internal/whatever` somewhere, otherwise can we just leave it unexported and use it in whatever package we need it in?
SamWhited marked this conversation as resolved
@ -0,0 +1,13 @@
//go:build go1.20

This doesn't appear to be necessary, we're still using clear_slice either way, no? I don't see an alternate file to use clear on 1.21 or anything. Let's juts use clearSlice for now on both and we can add an issue to update it later.

This doesn't appear to be necessary, we're still using `clear_slice` either way, no? I don't see an alternate file to use `clear` on 1.21 or anything. Let's juts use `clearSlice` for now on both and we can add an issue to update it later.

This got left in, but I think this means "go 1.20 and above" so it's still not actually needed; on Go 1.21 this function can just shadow the builtin function and that's fine since it's limited to this package and will be removed later, so we can remove this build constraint, I think.

This got left in, but I think this means "go 1.20 and above" so it's still not actually needed; on Go 1.21 this function can just shadow the builtin function and that's fine since it's limited to this package and will be removed later, so we can remove this build constraint, I think.
jaeckel marked this conversation as resolved
@ -0,0 +6,4 @@
package sce
func clear_slice(a []byte) {

Per go convention this should be named clearSlice

Per go convention this should be named `clearSlice`

Another quick thought: we can just name this clear and then remove it in January or February when Go 1.22 comes out. Then this is shadowing the builtin but code is already using clear and everything should just continue to work as expected since this signature is a subset of the generic clear's one.

Another quick thought: we can just name this `clear` and then remove it in January or February when Go 1.22 comes out. Then this is shadowing the builtin but code is already using clear and everything should just continue to work as expected since this signature is a subset of the generic clear's one.
SamWhited marked this conversation as resolved
sce/sce.go Outdated
@ -0,0 +21,4 @@
)
// SceProfileFlags one flag for each potential element of an XEP-0420 envelope
type SceProfileFlags struct {

General nit: per go convention acronyms should share case so this (and all similar functions) should be SCEProfileFlags.

However, in Go all functions will contain their import path, so the actual name of this function as it's used in code would be sce.SCEProfileFlags which is redundant. We're already in the sce package so no let's just drop the Sce from the start of all identifiers.

General nit: per go convention acronyms should share case so this (and all similar functions) should be `SCEProfileFlags`. *However*, in Go all functions will contain their import path, so the actual name of this function as it's used in code would be `sce.SCEProfileFlags` which is redundant. We're already in the `sce` package so no let's just drop the `Sce` from the start of all identifiers.
SamWhited marked this conversation as resolved
sce/sce.go Outdated
@ -0,0 +36,4 @@
Time time.Time `xml:"time"`
To jid.JID `xml:"to"`
From jid.JID `xml:"from"`
// Content is expected to be a xml.TokenReader on Marshaling

This probably shouldn't behave this way; it's going to be confusing and likely result in errors using the type. We can discuss later how best to handle it.

This probably shouldn't behave this way; it's going to be confusing and likely result in errors using the type. We can discuss later how best to handle it.

Thinking about this some more my initial thought would be to create a (*Envelope) Wrap(xml.TokenReader) xml.TokenReader method that creates a new token stream with the appropriate content. There would be a matching (*Envelope) Unwrap(xml.TokenReader) xml.TokenReader method that would marshal the contents into the envelope and return the inner part of the reader. This would also let the user do the content however they want by embedding a reader as well, eg.

type MyCustomEnvelope struct{
  Envelope
  Content []byte `xml:",innerxml"`
}

but I'm not sure how that interacts with the rest of your code, I'm just thinking out loud.

Thinking about this some more my initial thought would be to create a `(*Envelope) Wrap(xml.TokenReader) xml.TokenReader` method that creates a new token stream with the appropriate content. There would be a matching `(*Envelope) Unwrap(xml.TokenReader) xml.TokenReader` method that would marshal the contents into the envelope and return the inner part of the reader. This would also let the user do the content however they want by embedding a reader as well, eg. ``` type MyCustomEnvelope struct{ Envelope Content []byte `xml:",innerxml"` } ``` but I'm not sure how that interacts with the rest of your code, I'm just thinking out loud.
SamWhited reviewed 2023-09-12 12:19:21 +00:00
sce/sce.go Outdated
@ -0,0 +13,4 @@
"io"
"time"
"github.com/andreburgaud/crypt2go/padding"

Oops, I missed this the first time through. We definitely can't have this in the main XMPP library per our long-standing convention that this library is minimal and uses only the standard library and subrepos. We could discuss changing this, but I think it's probably easier to have this be another module if it's really required, or finding another way to do this. I'll look into it more shortly.

Oops, I missed this the first time through. We definitely can't have this in the main XMPP library per our long-standing convention that this library is minimal and uses only the standard library and subrepos. We could discuss changing this, but I think it's probably easier to have this be another module if it's really required, or finding another way to do this. I'll look into it more shortly.

Actually, if the only thing we need this dependency for is padding we can just do that; they don't even do it in a particularly nice way in this package, but this is just appending two slices. Let's definitely not bring in an entire import just for two lines of slice manipulation :)

Actually, if the only thing we need this dependency for is padding we can just do that; they don't even do it in a particularly nice way in this package, but this is just appending two slices. Let's definitely not bring in an entire import just for two lines of slice manipulation :)
Poster
Collaborator

Sure, I'll implement it from scratch, but it's not only going to be two lines ;)

Sure, I'll implement it from scratch, but it's not only going to be two lines ;)
SamWhited marked this conversation as resolved
SamWhited requested changes 2023-09-12 13:05:59 +00:00
SamWhited left a comment
Owner

A few more notes; thanks again for doing this!

A few more notes; thanks again for doing this!
sce/sce.go Outdated
@ -0,0 +57,4 @@
ErrSceNSMismatch = errors.New(`XML Namespace of encrypted element does not match`)
)
func (env Envelope) startElement() xml.StartElement {

For efficiency reasons this should be a function. We're not using anything from env here anyways and currently gc cannot inline methods unless that has been fixed in the last version or two.

For efficiency reasons this should be a function. We're not using anything from `env` here anyways and currently `gc` cannot inline methods unless that has been fixed in the last version or two.
jaeckel marked this conversation as resolved
sce/sce.go Outdated
@ -0,0 +69,4 @@
}
// MarshalXML satisfies the xml package's Marshaler interface.
func (env Envelope) MarshalXML(e *xml.Encoder, _ xml.StartElement) error {

Thanks for doing this; sorry you had to do all the boilerplate :) I have plans to make this unnecessary at some point, but for now we just have to keep copy/pasting.

Thanks for doing this; sorry you had to do all the boilerplate :) I have plans to make this unnecessary at some point, but for now we just have to keep copy/pasting.
jaeckel marked this conversation as resolved
sce/sce.go Outdated
@ -0,0 +210,4 @@
// SceCrypter is an interface to en-/decrypt data
type SceCrypter interface {
Encrypt(p []byte) ([]byte, error)

The signatures on these methods will potentially require heap-allocating a new byte slice to return if we can't re-use the input slice and more or less preclude any sort of streaming use.

Is there a reason we can't use cipher.Block for this? I'm assuming that we'll always use a block cipher (though if this is a bad assumption I guess we can't use it) and that encryption should never error. Looking at the Encrypt implementation below it looks like the only error is from applying padding, but I also can't see how that would fail in a way that requires returning an error (I suppose we could fail to allocate memory, but a panic seems fine there).

The signatures on these methods will potentially require heap-allocating a new byte slice to return if we can't re-use the input slice and more or less preclude any sort of streaming use. Is there a reason we can't use [`cipher.Block`](https://pkg.go.dev/crypto/cipher#Block) for this? I'm assuming that we'll always use a block cipher (though if this is a bad assumption I guess we can't use it) and that encryption should never error. Looking at the `Encrypt` implementation below it looks like the only error is from applying padding, but I also can't see how that would fail in a way that requires returning an error (I suppose we could fail to allocate memory, but a panic seems fine there).
Poster
Collaborator

The signatures on these methods will potentially require heap-allocating a new byte slice to return if we can't re-use the input slice and more or less preclude any sort of streaming use.

Maybe I'm missing some details, but how do you want to do stream processing of an encrypted stanza? You have to have the complete stanza already in order to be able to verify its authenticity before you process the encrypted stanza any further.

I've just realized, that I forgot doing the authenticity check. I'll add an Authenticator interface which takes the authentication key and is instantiated in the EncryptedEnvelope.

Is there a reason we can't use cipher.Block for this? I'm assuming that we'll always use a block cipher (though if this is a bad assumption I guess we can't use it) and that encryption should never error. Looking at the Encrypt implementation below it looks like the only error is from applying padding, but I also can't see how that would fail in a way that requires returning an error (I suppose we could fail to allocate memory, but a panic seems fine there).

The cipher.Block API asks the user to provide both src and dst. What's the advantage of that approach over what I did here? i.e. taking src and returning a newly allocated dst? cipher.Block is a bit different than our case, since we have differently sized in- and outputs, whereas cipher.Block doesn't have that IIUC.

Sure, I can change the API to be more in line with cipher.Block, i.e. in a first step remove the returned error. For the encryption it's indeed only the aes.NewCipher() call that could fail in reality (and should panic). For the decryption the base64 decoder could fail if invalid base64 data is received and the PKCS#7 unpadding could fail because there's an invalid pad value. If it's fine that we panic in those two cases I'll change that.

The Encrypt() and Decrypt() calls will require a different sized buffer for dst, i.e. the user would need to do the same dance as with the Base64 API. We would have to provide a Len API for both directions, so the user knows the amount of memory it needs to allocate for the dst buffer. Is that also what we want?

> The signatures on these methods will potentially require heap-allocating a new byte slice to return if we can't re-use the input slice and more or less preclude any sort of streaming use. Maybe I'm missing some details, but how do you want to do stream processing of an encrypted stanza? You have to have the complete stanza already in order to be able to verify its authenticity before you process the encrypted stanza any further. I've just realized, that I forgot doing the authenticity check. I'll add an `Authenticator` interface which takes the `authentication key` and is instantiated in the `EncryptedEnvelope`. > Is there a reason we can't use [`cipher.Block`](https://pkg.go.dev/crypto/cipher#Block) for this? I'm assuming that we'll always use a block cipher (though if this is a bad assumption I guess we can't use it) and that encryption should never error. Looking at the `Encrypt` implementation below it looks like the only error is from applying padding, but I also can't see how that would fail in a way that requires returning an error (I suppose we could fail to allocate memory, but a panic seems fine there). The `cipher.Block` API asks the user to provide both `src` and `dst`. What's the advantage of that approach over what I did here? i.e. taking `src` and returning a newly allocated `dst`? `cipher.Block` is a bit different than our case, since we have differently sized in- and outputs, whereas `cipher.Block` doesn't have that IIUC. Sure, I can change the API to be more in line with `cipher.Block`, i.e. in a first step remove the returned error. For the encryption it's indeed only the `aes.NewCipher()` call that could fail in reality (and should panic). For the decryption the base64 decoder could fail if invalid base64 data is received and the PKCS#7 unpadding could fail because there's an invalid pad value. If it's fine that we panic in those two cases I'll change that. The `Encrypt()` and `Decrypt()` calls will require a different sized buffer for `dst`, i.e. the user would need to do the same dance as with the Base64 API. We would have to provide a `Len` API for both directions, so the user knows the amount of memory it needs to allocate for the `dst` buffer. Is that also what we want?

Sorry for the confusion, the answers here are the same:

Something([]byte) []byte means we have to allocate new byte slices on the heap. Something(dst, src []byte) means we can re-use byte slices, eg. by pulling out one that's long enough from a pool.

I don't think cipher.Block mandates any sort of specific input and output size, it should be good there. We'd just want to have a Len method or similar as you previously mentioned or some other way of knowing the size.

I was forgetting about the base64 part, that's probably a valid reason to return an error on decrypt if that' still something you want. I'm not sure that should panic as it's something coming from user input, we probably don't want to crash on that unless it's going to result in leaking data or something.

Sorry for the confusion, the answers here are the same: `Something([]byte) []byte` means we have to allocate new byte slices on the heap. `Something(dst, src []byte)` means we can re-use byte slices, eg. by pulling out one that's long enough from a pool. I don't think `cipher.Block` mandates any sort of specific input and output size, it should be good there. We'd just want to have a `Len` method or similar as you previously mentioned or some other way of knowing the size. I was forgetting about the base64 part, that's probably a valid reason to return an error on decrypt if that' still something you want. I'm not sure that should panic as it's something coming from user input, we probably don't want to crash on that unless it's going to result in leaking data or something.
sce/sce.go Outdated
@ -0,0 +277,4 @@
}
// SceEnvelope binds together an Envelope, the required XML Namespace, a crypto method and an optional header
type SceEnvelope struct {

The naming feels a bit odd to me here; I'm having trouble understanding the distinction between Envelope and SceEnvelope. We should pick a name that makes it clear how they're distinct. Or do they need to be distinct at all?

The naming feels a bit odd to me here; I'm having trouble understanding the distinction between `Envelope` and `SceEnvelope`. We should pick a name that makes it clear how they're distinct. Or do they need to be distinct at all?
Poster
Collaborator

I renamed the SceEnvelope to EncryptedEnvelope, better?

I renamed the `SceEnvelope` to `EncryptedEnvelope`, better?

That makes much more sense to me; tells me what it is without having to dig around for context. Thanks!

That makes much more sense to me; tells me what it is without having to dig around for context. Thanks!
SamWhited marked this conversation as resolved
sce/sce.go Outdated
@ -0,0 +350,4 @@
}
// XSFOMEMO2SceEncryptedEnvelope is declaring the structure of the XML document
type XSFOMEMO2SceEncryptedEnvelope struct {

Should this stuff live here or in the omemo module? I could see it being in either, but I lean towards making this package a generic mid-level package and having the actual implementations for specific protocols maintained in their own modules by their own maintainers. Up to you though.

Should this stuff live here or in the `omemo` module? I could see it being in either, but I lean towards making this package a generic mid-level package and having the actual implementations for specific protocols maintained in their own modules by their own maintainers. Up to you though.
Poster
Collaborator

Yeah, this should be in the omemo module, I've only added it as example here since there's no omemo module yet.

Should I create omemo/omemo2.go and add this there? Or leave it here and move it later when we have figured out how the OMEMO implementations are structured?

Yeah, this should be in the `omemo` module, I've only added it as example here since there's no `omemo` module yet. Should I create `omemo/omemo2.go` and add this there? Or leave it here and move it later when we have figured out how the OMEMO implementations are structured?

Maybe you could add this stuff to the PR in the omemo repo we've been working on and make it depend on this package?

This may require some workspace setup and having this branch checked out to build the OMEMO module until we get this PR merged, but that's easy enough.

Maybe you could add this stuff to the PR in the omemo repo we've been working on and make it depend on this package? This may require some workspace setup and having this branch checked out to build the OMEMO module until we get this PR merged, but that's easy enough.
Poster
Collaborator

Maybe you could add this stuff to the PR in the omemo repo we've been working on and make it depend on this package?

hmm, that'd mean that we don't have a working test of the encrypt/decrypt part in this repo anymore. Do we really want that?

> Maybe you could add this stuff to the PR in the omemo repo we've been working on and make it depend on this package? hmm, that'd mean that we don't have a working test of the encrypt/decrypt part in this repo anymore. Do we really want that?

We'd still want to write encrypt/decrypt tests but I don't see why we need this specific implementation to test those functions. This specific implementation would still be tested too, just as part of the OMEMO package where it lives.

I could be wrong too, this just doesn't seem related to this package to me, it seems like it's part of OMEMO that depends on this package.

We'd still want to write encrypt/decrypt tests but I don't see why we need this specific implementation to test those functions. This specific implementation would still be tested too, just as part of the OMEMO package where it lives. I could be wrong too, this just doesn't seem related to this package to me, it seems like it's part of OMEMO that depends on this package.

Another thought (that we can worry about later, I just want to put it on the table but it's not important right now): I'm slightly worried that sce has no real adoption and is, IMO, frankly dangerous. We have other experimental XEPs implemented in this module, but they're widely used ones that just haven't been moved forward in the standardization process for one reason or another. This one feels different. Maybe we should have some sort of mellium.im/experimental module where stuff like this can go? This way we don't wind up stuck with it later if it turns out no one ever adopts SCE? If it does get more adoption, it can always be moved into this module as well. This is all stuff that's not important right now while we're pre-1.0, we could always move this to another module, then back here, etc. at a later time, I just wanted to think a bit about where it belongs early on to save us time later.

Another thought (that we can worry about later, I just want to put it on the table but it's not important right now): I'm slightly worried that `sce` has no real adoption and is, IMO, frankly dangerous. We have other experimental XEPs implemented in this module, but they're widely used ones that just haven't been moved forward in the standardization process for one reason or another. This one feels different. Maybe we should have some sort of `mellium.im/experimental` module where stuff like this can go? This way we don't wind up stuck with it later if it turns out no one ever adopts SCE? If it does get more adoption, it can always be moved into this module as well. This is all stuff that's not important right now while we're pre-1.0, we could always move this to another module, then back here, etc. at a later time, I just wanted to think a bit about where it belongs early on to save us time later.
Poster
Collaborator

Another thought (that we can worry about later, I just want to put it on the table but it's not important right now): I'm slightly worried that sce has no real adoption [...]

It's a requirement for omemo:2 and the trust messages part.

[...] and is, IMO, frankly dangerous.

Why do you think that?

We have other experimental XEPs implemented in this module, but they're widely used ones that just haven't been moved forward in the standardization process for one reason or another. This one feels different. Maybe we should have some sort of mellium.im/experimental module where stuff like this can go? This way we don't wind up stuck with it later if it turns out no one ever adopts SCE? If it does get more adoption, it can always be moved into this module as well. This is all stuff that's not important right now while we're pre-1.0, we could always move this to another module, then back here, etc. at a later time, I just wanted to think a bit about where it belongs early on to save us time later.

Sure, we can move it if you think it's better :)

> Another thought (that we can worry about later, I just want to put it on the table but it's not important right now): I'm slightly worried that `sce` has no real adoption [...] It's a requirement for `omemo:2` and the trust messages part. > [...] and is, IMO, frankly dangerous. Why do you think that? > We have other experimental XEPs implemented in this module, but they're widely used ones that just haven't been moved forward in the standardization process for one reason or another. This one feels different. Maybe we should have some sort of `mellium.im/experimental` module where stuff like this can go? This way we don't wind up stuck with it later if it turns out no one ever adopts SCE? If it does get more adoption, it can always be moved into this module as well. This is all stuff that's not important right now while we're pre-1.0, we could always move this to another module, then back here, etc. at a later time, I just wanted to think a bit about where it belongs early on to save us time later. Sure, we can move it if you think it's better :)
jaeckel force-pushed xep-0420 from 1a523b31a6 to 8b39b16fdd 2023-09-21 14:18:02 +00:00 Compare

It's a requirement for omemo:2 and the trust messages part. … Why do you think that [it's dangerous]?

Oh yah, we definitely have to implement it unfortunately, but I'm just worried about the implications or it being abandoned later after there ends up being a security issue. It's not really important here but the danger that I mostly see is that it violates XML parser expectations. E2E encryption of only part of the stream at the serialization layer (eg. not just the content) means that the parser that parses the encrypted XML either has to be separate from the parser that handles the unencrypted XML, which makes namespacing and what not very hard, or you have to use one parser and treat the encrypted XML as part of the wider stream, but now the parser has mixed state across the encryption boundary which could potentially leak information about namespaces, stanza counts, etc. this seems like it would be extremely difficult to do safely.

You also mitigate the servers ability to remove eg. XML expansion bombs and the like (or whatever new unsolved problem comes up, more likely). This pushes the need to deal with XML vulnerabilities down to the clients, and that's a lot more surface area for something to go wrong.

TL;DR mixing encrypted and non-encrypted token stacks seems like a bad idea, but not doing it will likely result in malformed XML documents without correct namespacing.

All that said, we have to implement it, and we don't have to move it or anything right now, I just am not sure it's a technology that has been well thought out or that will last so I want us to be thinking about this sort of thing as we work on it.

Sure, we can move it if you think it's better :)

Let's finish this PR and then we can decide what to do. I'm not 100% set on this or anything, just worried. We can always move it pre-1.0 anyways, so it's not important right this second.

> It's a requirement for omemo:2 and the trust messages part. … Why do you think that [it's dangerous]? Oh yah, we definitely have to implement it unfortunately, but I'm just worried about the implications or it being abandoned later after there ends up being a security issue. It's not really important here but the danger that I mostly see is that it violates XML parser expectations. E2E encryption of only part of the stream at the serialization layer (eg. not just the content) means that the parser that parses the encrypted XML either has to be separate from the parser that handles the unencrypted XML, which makes namespacing and what not very hard, or you have to use one parser and treat the encrypted XML as part of the wider stream, but now the parser has mixed state across the encryption boundary which could potentially leak information about namespaces, stanza counts, etc. this seems like it would be *extremely* difficult to do safely. You also mitigate the servers ability to remove eg. XML expansion bombs and the like (or whatever new unsolved problem comes up, more likely). This pushes the need to deal with XML vulnerabilities down to the clients, and that's a lot more surface area for something to go wrong. TL;DR mixing encrypted and non-encrypted token stacks seems like a bad idea, but not doing it will likely result in malformed XML documents without correct namespacing. All that said, we have to implement it, and we don't have to move it or anything right now, I just am not sure it's a technology that has been well thought out or that will last so I want us to be thinking about this sort of thing as we work on it. > Sure, we can move it if you think it's better :) Let's finish this PR and then we can decide what to do. I'm not 100% set on this or anything, just worried. We can always move it pre-1.0 anyways, so it's not important right this second.
jaeckel force-pushed xep-0420 from 8b39b16fdd to 1cbf9f371d 2023-09-21 14:31:17 +00:00 Compare
SamWhited reviewed 2023-09-21 14:32:33 +00:00
SamWhited requested changes 2023-09-21 14:37:02 +00:00
sce/sce.go Outdated
@ -0,0 +47,4 @@
// A list of potential errors returned by this package
var (
// ErrSceProfileMissingFrom the SCE Profile requires a 'from' element, but none is given
ErrSceProfileMissingFrom = errors.New(`SCE Profile requires a 'from' element, but none is given`)

nit: these errors should also drop the Sce

nit: these errors should also drop the `Sce`
jaeckel marked this conversation as resolved
jaeckel force-pushed xep-0420 from 1cbf9f371d to 3a77b7a9ce 2023-09-21 14:42:03 +00:00 Compare
jaeckel force-pushed xep-0420 from 3a77b7a9ce to 99b14de597 2023-09-21 14:46:49 +00:00 Compare
jaeckel requested review from SamWhited 2023-09-21 15:05:38 +00:00
jaeckel reviewed 2023-09-22 09:39:18 +00:00
@ -0,0 +205,4 @@
}
// AuthedCrypter is an interface for authenticated encryption
type AuthedCrypter interface {
Poster
Collaborator

I thought about re-using the AEAD interface, but that one expects the tag/MAC to be appended and we need it separate, therefore I've created a new one.

Regarding the change to a similar signature as Blockmode I'm still waiting for an answer to #385 (comment)

I thought about re-using the [`AEAD`](https://pkg.go.dev/crypto/cipher#AEAD) interface, but that one expects the tag/MAC to be appended and we need it separate, therefore I've created a new one. Regarding the change to a similar signature as [`Blockmode`](https://pkg.go.dev/crypto/cipher#BlockMode) I'm still waiting for an answer to https://codeberg.org/mellium/xmpp/pulls/385#issuecomment-1209320

Seems okay, though I don't understand this well enough to know why it would be different.

Because this is a streaming type of encryption we might do something like the AEAD interface does though where we take dst []byte as an argument, but also return a byte slice.
The idea is that if dst is long enough we can return it and no allocation has taken place. If it is not long enough we can expand its length to fill the full capacity, or allocate under the hood and return the new slice. Either way the returned slice replaces dst. For a simpler example, look at the builtin append function.

Seems okay, though I don't understand this well enough to know why it would be different. Because this is a streaming type of encryption we might do something like the `AEAD` interface does though where we take `dst []byte` as an argument, but also return a byte slice. The idea is that if `dst` is long enough we can return it and no allocation has taken place. If it is not long enough we can expand its length to fill the full capacity, or allocate under the hood and return the new slice. Either way the returned slice replaces dst. For a simpler example, look at the builtin [`append`](https://pkg.go.dev/builtin#append) function.

Going back through this there's a lot of stuff I'm just unsure about until we get some implementation experience. It all seems generally okay though, so how would you feel about creating a new module, https://mellium.im/exp or similar that contains experimental new stuff? This way we could go ahead and get this merged, you can get the payout for it, and we can merge it back into the main XMPP module after we've tried it out and decided on a final API?

Going back through this there's a lot of stuff I'm just unsure about until we get some implementation experience. It all seems generally okay though, so how would you feel about creating a new module, https://mellium.im/exp or similar that contains experimental new stuff? This way we could go ahead and get this merged, you can get the payout for it, and we can merge it back into the main XMPP module after we've tried it out and decided on a final API?
Some checks failed
ci/woodpecker/push/dco Pipeline was successful
ci/woodpecker/pr/dco Pipeline was successful
ci/woodpecker/push/test Pipeline was successful
ci/woodpecker/push/validate Pipeline was successful
ci/woodpecker/pr/test Pipeline was successful
ci/woodpecker/pr/validate Pipeline was successful
ci/woodpecker/pr/integration Pipeline failed
ci/woodpecker/push/integration Pipeline failed
This pull request is marked as a work in progress.
This branch is out-of-date with the base branch
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b xep-0420 main
git pull origin xep-0420

Step 2:

Merge the changes and update on Forgejo.
git checkout main
git merge --no-ff xep-0420
git push origin main
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: mellium/xmpp#385
There is no content yet.