WIP: Implement XEP-0420 Stanza Content Encryption #385
No reviewers
Labels
No Label
bug
CI
documentation
duplicate
enhancement
good first issue
help wanted
i18n
invalid
needs-investigation
ops
proposal
proposal-accepted
proposal-declined
question
refactor
security
testing
upstream
wontfix
Kind: Breaking
Kind: Bug
Kind: Documentation
Kind: Enhancement
Kind: Feature
Kind: Maintenance
Kind: Question
Kind: Security
Kind: Testing
Priority: Critical
Priority: High
Priority: Low
Priority: Medium
Reviewed: Confirmed
Reviewed: Duplicate
Reviewed: Invalid
Status: Blocked
Status: Completed
Status: Help wanted
Status: In progress
Status: Needs feedback
Status: Stale
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: mellium/xmpp#385
Loading…
Reference in New Issue
There is no content yet.
Delete Branch "xep-0420"
Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?
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 aXSFOMEMO2SceEnvelope
that expects the XML to be formatted as specified in theOMEMO:2
XEP-0384 v0.8.0 upwards.Please review and let me know what you think should be improved.
7993f09e2e
to8a9d567c4f
8e1149c010
to60a3933fbf
60a3933fbf
to7eaa494c60
7eaa494c60
to06bbcafc70
06bbcafc70
to778e1a74d2
778e1a74d2
toe24979aa01
e24979aa01
to1a523b31a6
@ -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).
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?
I accidentally commited that one.
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.
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.
@ -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?@ -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 useclear
on 1.21 or anything. Let's juts useclearSlice
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.
@ -0,0 +6,4 @@
package sce
func clear_slice(a []byte) {
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.@ -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 thesce
package so no let's just drop theSce
from the start of all identifiers.@ -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.
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.but I'm not sure how that interacts with the rest of your code, I'm just thinking out loud.
@ -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.
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 :)
Sure, I'll implement it from scratch, but it's not only going to be two lines ;)
A few more notes; thanks again for doing this!
@ -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 currentlygc
cannot inline methods unless that has been fixed in the last version or two.@ -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.
@ -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 theEncrypt
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).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 theauthentication key
and is instantiated in theEncryptedEnvelope
.The
cipher.Block
API asks the user to provide bothsrc
anddst
. What's the advantage of that approach over what I did here? i.e. takingsrc
and returning a newly allocateddst
?cipher.Block
is a bit different than our case, since we have differently sized in- and outputs, whereascipher.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 theaes.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()
andDecrypt()
calls will require a different sized buffer fordst
, i.e. the user would need to do the same dance as with the Base64 API. We would have to provide aLen
API for both directions, so the user knows the amount of memory it needs to allocate for thedst
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 aLen
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.
@ -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
andSceEnvelope
. We should pick a name that makes it clear how they're distinct. Or do they need to be distinct at all?I renamed the
SceEnvelope
toEncryptedEnvelope
, better?That makes much more sense to me; tells me what it is without having to dig around for context. Thanks!
@ -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.Yeah, this should be in the
omemo
module, I've only added it as example here since there's noomemo
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.
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.
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 ofmellium.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.It's a requirement for
omemo:2
and the trust messages part.Why do you think that?
Sure, we can move it if you think it's better :)
1a523b31a6
to8b39b16fdd
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.
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.
8b39b16fdd
to1cbf9f371d
@ -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
1cbf9f371d
to3a77b7a9ce
3a77b7a9ce
to99b14de597
@ -0,0 +205,4 @@
}
// AuthedCrypter is an interface for authenticated encryption
type AuthedCrypter interface {
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)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 takedst []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 builtinappend
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?
Step 1:
From your project repository, check out a new branch and test the changes.Step 2:
Merge the changes and update on Forgejo.