Support the b64 header parameter #230
Conversation
|
|
|
This PR is ready for review. /cc @csstaub @mcpherrinm |
jws.go
Outdated
| var serializedProtected string | ||
| var buf bytes.Buffer | ||
|
|
||
| pro := new(rawHeader) |
There was a problem hiding this comment.
This is a nit, but I'd prefer if we gave these variables more descriptive names. It's not clear at first glance what "pro", "enc" and "buf" mean or what they're used for.
There was a problem hiding this comment.
How about calling these protectedHeader, needsBase64, and authData (or something along those lines)?
| @@ -349,6 +350,21 @@ func (parsed rawHeader) getP2S() (*byteBuffer, error) { | |||
| return parsed.getByteBuffer(headerP2S) | |||
| } | |||
|
|
|||
| // getB64 extracts parsed "b64" from the raw JSON, defaulting to true. | |||
| func (parsed rawHeader) getB64() (bool, error) { | |||
There was a problem hiding this comment.
Would it ever be necessary here to distinguish between the header being present, and set, or the header being not present and the default return value being false?
signing.go
Outdated
| input.WriteString(base64.RawURLEncoding.EncodeToString(serializedProtected)) | ||
| input.WriteByte('.') | ||
|
|
||
| enc := true |
There was a problem hiding this comment.
Same comment as above, let's call this something other than "enc". Could be confused to mean "encrypt" for example.
| ExtraHeaders: map[HeaderKey]interface{}{ | ||
| "b64": false, | ||
| }, | ||
| } |
There was a problem hiding this comment.
This seems somewhat non-obvious, would it be better to have this be a first-class option in SignerOptions and have the signing code add the header if that's selected?
|
Hi @philtay, thank you so much for your pull request! I left a couple of comments inline, but I think what's also missing is code to add the b64 header to the list of crit headers (see RFC 7797, Section 6). But otherwise this looks pretty good! |
|
@csstaub thanks for the review. The last commit takes in account your comments and brings some improvements. |
|
@csstaub can you review the updated PR? I'm eager to have this merged. Thanks! |
|
Hi @philtay, changes look good to me! Thank you very much for your contribution. I'll merge this and play around with it a bit and then will try to make a tagged release later this week. |
|
@csstaub thanks! I've another PR to open before making a tagged release. A |
|
Sounds good, let's do it. |
Closes #225