Custom headers and additional header support

[lusis]

The JWE spec supports more than just the two fields allowed currently in go-jose (alg and enc). You can also specify a kid which is really important when using symmetric encryption like AES with JWE.

I'm attempting something like so:

    alg := jose.KeyAlgorithm("dir")
    enc := jose.ContentEncryption("A256GCM")
    encrypter, err := jose.NewEncrypter(alg, enc, resp.Plaintext)
    if err != nil {
        log.Fatalf(err.Error())
    }
    obj, err := encrypter.Encrypt([]byte(contents))
    obj.Header.KeyID = "XXXXXX"

but when I attempt to base64 decode the header (first part), I still only see alg and enc.

I'm proposing either changing the Encrypter interface to support new funcs for adding kid specifically or a more generic one to allow setting headers that are unrelated to the encryption and key methods being used.

I can maybe make a PR on this but I need to know which way you'd rather go first.

Thanks!

[csstaub]

Yeah, we currently don't have good support for that. I'm not sure what would be a good interface for doing this, e.g. how do you control whether you want to set headers for the entire message or only for individual recipients? I think it would make sense to change the signature of e.g. NewEncrypter, AddRecipient to allow for also passing in headers. Another option would be to wrap the key in a new interface and support taking that as input. For example, we could consider adding a new Recipient interface which would contain the key but also metadata that describes it (such as key id or thumbprints). In NewEncrypter we would then distinguish between raw keys and the new recipient interface.

[csstaub]

I think adding a new Recipient interface that can hold keys and metadata together would be a cleaner solution, but I'd love to have some input from more people on this. Thoughts?

[lusis]

I was just looking. Can we maybe as a first pass consider using the same mechanism as SetCompression as a short term fix? I can knock that out fairly quickly (it's what I was going to do in the short term as a fork anyway).

[lusis]

Lemme also poke at the recipient interface as well. Obviously the whole kid bit doesn't play well with jose-util but it doesn't really support dir anyway. In our use case we'd be passing in an AWS KMS alias as the kid and grabbing the key ourselves.

[lusis]

FWIW currently in our case we don't need this to per-recipient

[csstaub]

I'm not sure I would consider merging something into master if we don't intend to keep it there long-term, but I'd be happy to merge it into a different branch (e.g. one that you could pull in via gopkg.in).

[csstaub]

Not supporting jose-util is fine, we don't intend for it to be feature-complete anyway. It's meant for debugging and development purposes and not production use.

[lusis]

okay here's a short git diff of my current fix that appears to accomplish the task:

diff --git a/crypter.go b/crypter.go
index 9aa1e15..6eba7c3 100644
--- a/crypter.go
+++ b/crypter.go
@@ -28,6 +28,7 @@ type Encrypter interface {
        Encrypt(plaintext []byte) (*JsonWebEncryption, error)
        EncryptWithAuthData(plaintext []byte, aad []byte) (*JsonWebEncryption, error)
        SetCompression(alg CompressionAlgorithm)
+       SetKid(kid string)
 }

 // MultiEncrypter represents an encrypter which supports multiple recipients.
@@ -68,6 +69,7 @@ type genericEncrypter struct {
        cipher         contentCipher
        recipients     []recipientKeyInfo
        keyGenerator   keyGenerator
+       kid            string
 }

 type recipientKeyInfo struct {
@@ -80,6 +82,11 @@ func (ctx *genericEncrypter) SetCompression(compressionAlg CompressionAlgorithm)
        ctx.compressionAlg = compressionAlg
 }

+// SetUnprotectedHeader sets a JWE unprotected header
+func (ctx *genericEncrypter) SetKid(kid string) {
+       ctx.kid = kid
+}
+
 // NewEncrypter creates an appropriate encrypter based on the key type
 func NewEncrypter(alg KeyAlgorithm, enc ContentEncryption, encryptionKey interface{}) (Encrypter, error) {
        encrypter := &genericEncrypter{
@@ -247,6 +254,10 @@ func (ctx *genericEncrypter) EncryptWithAuthData(plaintext, aad []byte) (*JsonWe
                obj.protected.Zip = ctx.compressionAlg
        }

+       if ctx.kid != "" {
+               obj.protected.Kid = ctx.kid
+       }
+
        authData := obj.computeAuthData()
        parts, err := ctx.cipher.encrypt(cek, authData, plaintext)
        if err != nil {

I think this would work but as you said it may not be a long term approach. I also still need to write a test case for it. I'm outside of my comfort zone in crypto related stuff and I really wanna make sure that it doesn't break anything/cause a security issue.

[lusis]

Side note related to testing, evidently it's totally valid to have an empty placeholder when using dir without keywrap:

eyJhbGciOiJkaXIiLCJlbmMiOiJBMjU2R0NNIiwia2lkIjoiYXJuOmF3czprbXM6dXMtd2VzdC0yOjg3MDQ3NDQ4NDk1MDprZXkvZWVkNmRkY2UtMTZhMC00YTRjLTljYzctZTk0MTM1YzYxNjhhIn0.._xHSHO-5C37rfgDP.A99ptzA2js0FNQz19M6mAztVPEAkEzzwpT_E.-2tXXuyEJVsD_4VN6xUezw

This may need to be reflected when I update the tests.

[csstaub]

No need to worry about breaking anything, we have a dedicated team of security engineers that can review code and help out :). I can try to come up with a sketch for adding a recipient interface over the weekend.

[csstaub]

I believe https://github.com/square/go-jose/pull/58 is a partial solution to this (in particular, it addresses the key ID problem).

[yawn]

Another important case would be cty (for encrypted and signed JWTs), see https://tools.ietf.org/html/rfc7519#section-5.2

[csstaub]

@yawn: I have a v2 branch where I'm refactoring the interface a bit, mainly to make signers/encrypters immutable. Any ideas how we could fit that in there: https://godoc.org/gopkg.in/square/go-jose.v2 ?

[shaxbee]

@yawn see #125

[hlandau]

FYI the ACME specification, which relies on JOSE, now specifies an url field in the protected header, as well as kid and nonce. https://tools.ietf.org/html/draft-ietf-acme-acme-05#section-5.2

Having to change go-jose every time some specification wants to put something in the protected header seems untenable. How about providing a means of injecting arbitrary keys into the protected header via a map[string]interface{}?

[csstaub]

That's a fair point, I think that's something we could add if you have a pull request for it.