Add parsed JWK header parameter #240

johanfylling · 2020-10-30T09:43:24Z

The JWS and JWE specifications dictate that the 'jwk' header parameter should be represented as a JWK (https://tools.ietf.org/html/rfc7515#section-4.1.3 & https://tools.ietf.org/html/rfc7516#section-4.1.5). The JWK representation is a JSON dictionary, and not a string, as currently implemented by JOSESwift.

This pull request is a suggested solution. It is also a work in progress, as the implementation of the 'jwk' header getters is sub-optimal and a better approach may be more desirable.

This pull request also does not provide test coverage, and short-circuits some tests in benefit of successful compilation.

TODO: jwk parameter parsing TODO: tests

ghost · 2020-11-04T11:32:27Z

Hi @johanfylling 👋 Good find! That indeed looks like a mismatch between implementation and spec. We'll have a look at you pull request soon and will get back to you with some feedback.

ghost · 2020-11-10T07:08:20Z

Hi @johanfylling!

Sorry for the delay. Your implementation looks pretty solid. Our next steps on this pull request would be to get the tests fully up and running again. Also I think we could speed up the release of this if we'd keep around the old String parameter for compatibility reasons. I'm thinking let the old jwk parameter be a String and add a new computed one that transforms the String to a JWK (just as you implemented it already).

Since that way we wouldn't introduce any breaking API changes, we could release this much faster as a minor version bump and wouldn't have to wait for the next major release. Moreover, it would make it much easier for you to fix the tests as the old tests should just run ok again and you would only have to add one or two test cases that cover the new parsed JWK parameter.

Let me know what you think!

…alization keeping the old behaviour of the 'jwk' parameter.

johanfylling · 2020-11-10T09:40:09Z

I have updated the pull request to now have two parameters that both read/write to the 'jwk' header param.
The 'jwk' parameter now is a String optional, and behaves just as before. The new 'jwkTyped' parameter assumes the 'jwk' header parameter to be a dictionary, and if used as a setter, a dictionary 'jwk' will be produced for the serialization.
Is this an acceptable solution? If so, I can spend some time to write tests to ensure this parameter behaves as advertised.

ghost

Yes this would definitely be an acceptable solution. The duplication is a bit unfortunate but this is our fault and we'll clean that up once we get a chance to redo the header implementation.

If you have time to add some more test that hit both the JWE and JWS implementation - that would be greatly appreciated.

ghost · 2020-11-10T09:57:43Z

JOSESwift/Sources/JWEHeader.swift

@@ -143,8 +143,8 @@ extension JWEHeader: CommonHeaderParameterSpace {
            return URL(string: parameter)
        }
    }
-
-    /// The JSON Web key corresponding to the key used to encrypt the JWE.
+


Suggested change

ghost · 2020-11-10T09:58:45Z

JOSESwift/Sources/JWEHeader.swift

@@ -154,6 +154,36 @@ extension JWEHeader: CommonHeaderParameterSpace {
        }
    }

+    /// The JSON Web key corresponding to the key used to encrypt the JWE, as a JWK..


Suggested change

/// The JSON Web key corresponding to the key used to encrypt the JWE, as a JWK..

/// The JSON Web key corresponding to the key used to encrypt the JWE, as a JWK.

ghost · 2020-11-10T10:00:16Z

JOSESwift/Sources/JWEHeader.swift

+            guard let keyTypeString = jwkParameters[JWKParameter.keyType.rawValue],
+                  let keyType = JWKKeyType(rawValue: keyTypeString) else {
+                return nil
+            }


Suggested change

guard let keyTypeString = jwkParameters[JWKParameter.keyType.rawValue],

let keyType = JWKKeyType(rawValue: keyTypeString) else {

return nil

}

guard

let keyTypeString = jwkParameters[JWKParameter.keyType.rawValue],

let keyType = JWKKeyType(rawValue: keyTypeString)

else {

return nil

}

ghost · 2020-11-10T10:04:03Z

JOSESwift/Sources/JWSHeader.swift

@@ -103,8 +103,8 @@ extension JWSHeader: CommonHeaderParameterSpace {
            return URL(string: parameter)
        }
    }
-
-    /// The JSON Web key corresponding to the key used to digitally sign the JWS.
+


Suggested change

ghost · 2020-11-10T10:05:05Z

JOSESwift/Sources/JWSHeader.swift

+            guard let keyTypeString = jwkParameters[JWKParameter.keyType.rawValue],
+                  let keyType = JWKKeyType(rawValue: keyTypeString) else {
+                return nil
+            }


Suggested change

guard let keyTypeString = jwkParameters[JWKParameter.keyType.rawValue],

let keyType = JWKKeyType(rawValue: keyTypeString) else {

return nil

}

guard

let keyTypeString = jwkParameters[JWKParameter.keyType.rawValue],

let keyType = JWKKeyType(rawValue: keyTypeString)

else {

return nil

}

johanfylling · 2020-11-10T15:33:01Z

I've added tests for JWE- and JWS headers. The two failing tests were already failing on the tag (2.2.1) I branched off from.

ghost · 2020-11-11T09:43:27Z

Marking this PR ready for review now.

ghost · 2020-11-11T09:47:07Z

I'll let you know once this is released @johanfylling. Thanks for the contribution!

johanfylling · 2020-11-11T11:18:36Z

Thank you very much for your input @daniel-mohemian and for accepting our proposal!

ghost · 2020-11-12T09:33:00Z

Hey @johanfylling 👋 Your contribution is now live as 2.3.0.

johanfylling · 2020-11-13T01:54:57Z

Awesome! Thank you very much for a speedy release.

johanfylling added 2 commits October 30, 2020 10:15

Making the jwk JOSEHeader parameter type be JWK instead of String

9753e2c

TODO: jwk parameter parsing TODO: tests

Updating jwk JWS/JWE header parameter getter

a023170

ghost mentioned this pull request Nov 4, 2020

JWS-/JWE 'jwk' header parameter should be a JSON dictionary #241

Closed

Adding 'jwkTyped' header parameter that overrides 'jwk' param in seri…

1704239

…alization keeping the old behaviour of the 'jwk' parameter.

ghost reviewed Nov 10, 2020

View reviewed changes

johanfylling added 3 commits November 10, 2020 11:26

Cleanup

dc82d31

Adding JWE-/JWS header tests

20db3cd

Updating JWEHeaderTests

80c11d8

ghost marked this pull request as ready for review November 11, 2020 09:43

Merge branch 'master' into dictionary_jwk_header_params

be479f8

ghost approved these changes Nov 11, 2020

View reviewed changes

ghost changed the title ~~Dictionary jwk header params~~ Add parsed JWK header parameter Nov 11, 2020

ghost merged commit 4425ff5 into airsidemobile:master Nov 11, 2020

ghost mentioned this pull request Nov 11, 2020

Prepare 2.3.0 #242

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parsed JWK header parameter #240

Add parsed JWK header parameter #240

johanfylling commented Oct 30, 2020

ghost commented Nov 4, 2020

ghost commented Nov 10, 2020

johanfylling commented Nov 10, 2020

ghost left a comment

ghost Nov 10, 2020

ghost Nov 10, 2020

ghost Nov 10, 2020

ghost Nov 10, 2020

ghost Nov 10, 2020

johanfylling commented Nov 10, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

johanfylling commented Nov 11, 2020

ghost commented Nov 12, 2020

johanfylling commented Nov 13, 2020

	/// The JSON Web key corresponding to the key used to encrypt the JWE, as a JWK..
	/// The JSON Web key corresponding to the key used to encrypt the JWE, as a JWK.

Add parsed JWK header parameter #240

Add parsed JWK header parameter #240

Conversation

johanfylling commented Oct 30, 2020

ghost commented Nov 4, 2020

ghost commented Nov 10, 2020

johanfylling commented Nov 10, 2020

ghost left a comment

Choose a reason for hiding this comment

ghost Nov 10, 2020

Choose a reason for hiding this comment

ghost Nov 10, 2020

Choose a reason for hiding this comment

ghost Nov 10, 2020

Choose a reason for hiding this comment

ghost Nov 10, 2020

Choose a reason for hiding this comment

ghost Nov 10, 2020

Choose a reason for hiding this comment

johanfylling commented Nov 10, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

johanfylling commented Nov 11, 2020

ghost commented Nov 12, 2020

johanfylling commented Nov 13, 2020