-
Notifications
You must be signed in to change notification settings - Fork 337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JWE Support #220
JWE Support #220
Conversation
Signed-off-by: ortyomka <iurin.art@gmail.com>
Signed-off-by: ortyomka <iurin.art@gmail.com>
Signed-off-by: ortyomka <iurin.art@gmail.com>
Signed-off-by: ortyomka <iurin.art@gmail.com>
Add tests Signed-off-by: ortyomka <iurin.art@gmail.com>
Signed-off-by: ortyomka <iurin.art@gmail.com>
I think this was an oversight on our part. This likely doesn't belong in this package. https://github.com/golang-jwt/jwt#signing-vs-encryption Not sure if our stance on this has changed cc @oxisto |
As I understand it, this section was written before this issue was created. And in the issue, the author of the section writes that he doesn't mind supporting JWE.
Moreover, some people are waiting for this support.
I purposely put JWE in a separate module so as not to affect the main part of the project. Developers need to intentionally include the JWE module if they want to use it. |
Personally, I wouldn’t mind JWE support. We probably need to come up with a good naming / packaging scheme. I hopefully can allocate some time next week to give this a first review round. |
Signed-off-by: ortyomka <iurin.art@gmail.com>
Before we go any further with this, I wonder whether it would be more appropriate to move this into a separate repository, @mfridman? Since JWT and JWE are more sibling references, and JWE support within the JWT repository might be confusing. So we would have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of reviews
"strings" | ||
) | ||
|
||
func NewJWE(alg KeyAlgorithm, key interface{}, method EncryptionType, plaintext []byte) (*jwe, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation for exported function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation
jwe/jwe.go
Outdated
} | ||
|
||
type jwe struct { | ||
protected map[string]string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to make this header a struct with specific values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to struct
@@ -0,0 +1,56 @@ | |||
package jwe_test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if we could re-use example data from https://datatracker.ietf.org/doc/html/rfc7516#appendix-A
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reused, This required setting a random, so I made a global random reader similar to the crypto.rand
package.
Here
"strings" | ||
) | ||
|
||
func NewJWE(alg KeyAlgorithm, key interface{}, method EncryptionType, plaintext []byte) (*jwe, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plaintext is the JWT in compact form, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any sensitive info. It can be JWT in compact form.
jwe/keycrypter.go
Outdated
|
||
var RSA_OAEP = KeyAlgorithm("RSA-OAEP") | ||
|
||
func rsaEncrypt(key *rsa.PublicKey, cek []byte, alg KeyAlgorithm) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any particular reason why you are using functions here instead of a struct with methods? Just wondering. The latter might be better if we want to make an interface out of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to struct with Encrypt and Decrypt. For the future
Change map to struct with json tags Rename enum values Add documentation Signed-off-by: ortyomka <iurin.art@gmail.com>
@oxisto thank you for the review. I fixed all comments, ask for re-review. |
Yes, I think this should be moved to its own repository. |
@mfridman, who can create new repo? I will forward this PR to there |
|
@oxisto need some init commit, I cannot fork or open PR |
Moved to golang-jwt/jwe#1 |
Issue #67
Add support of JWE with one key (Compact)
Add AES GCM cipher to encrypt content
Add RSA-OAEP to encrypt key
Add test from RFC7516 Section 3.3
cc @mfridman