Version 4 to Version 5 Migration Deprecations by dimitribouniol · Pull Request #221 · vapor/jwt-kit

dimitribouniol · 2024-11-20T13:45:11Z

This PR adds migration deprecations for many common APIs between version 4 and version 5 that should help most users (particularly those using ECDSA keys and standard JWTs) migrate from version 4 to version 5 by following warnings. Crucially, it allows libraries that rely on the same requirements (for instance, https://github.com/apple/app-store-server-library-swift and https://github.com/m-barthelemy/AcmeSwift) to allow for a range of versions such as "4.14.0" ..< "6.0.0".

If we agree with the proposed updates here, a few more things should be done:

Cut a new minor v4 version, specifically 4.14.0, so users can opt out of the warnings should they wish to.
Add an entry to main's README explaining the process to update from v4 to v5.
Update v5 with an AsyncJWTPayload typealias back to JWTPayload that is itself deprecated, to fully support libraries that wish to be compatible with v4 and v5.
Make a similar but smaller PR to jwt for vapor to move folks away from signers and onto keys (otherwise they are still forced into an all or nothing update since they will still be stuck with JWTSigners).

Note that I didn't make compatibility APIs for every type — if someone feels they are necessary due to a library requiring them, I figured we could add more support in a future PR.

dimitribouniol · 2024-11-20T13:46:44Z

+/// JWTKeyCollection was introduced in v5 and replaces ``JWTSigners``.
+///
+/// - Note: Please migrate over to ``JWTKeyCollection`` before updating to v5, though if you plan on remaining on v4, ``JWTSigners`` can continue to be used.
+public actor JWTKeyCollection {
+    var signers = JWTSigners()


This is a lightweight implementation of the new JWTKeyCollection API falling back entirely on the now deprecated JWTSigners API.

dimitribouniol · 2024-11-20T13:48:29Z

+/// A transitionary protocol with sync and async requirements.
+///
+/// This protocol should be dropped once you are finished migrating to v5, as it'll have been renamed back to ``JWTPayload``, but with a single async requirement. In order to support both versions v4 and v5 in a library, do not implement the requirements of ``JWTPayload`` as ``JWTSigner`` is no longer available in v5.
+public protocol AsyncJWTPayload: Codable {
+    func verify<Algorithm: JWTAlgorithm>(using signer: Algorithm) throws
+
+    /// Verifies that the payload's claims are correct or throws an error.
+    func verify<Algorithm: JWTAlgorithm>(using algorithm: Algorithm) async throws
+}


Because we can't mark protocol methods as deprecated, we define a new one that will be transitionary and support both sync and asynchronous modalities, ensuring compatibility with existing code paths, but also newer signing methods no matter which would be used.

I think you can mark protocol methods as deprecated can't you?

You can mark them, but you don't get any warnings telling you they are deprecated as a conformer (which makes sense since the method itself may be used by another protocol or just on its own by whoever is conforming to the protocol). I believe you do see them as a caller though, which is not as useful in this situation.

dimitribouniol · 2024-11-20T13:49:59Z

+    @available(*, deprecated, message: "Please make sure Payload conforms to AsyncJWTPayload instead of JWTPayload before updating to v5.")
+    public func sign<Payload: JWTPayload>(
+        _ payload: Payload,
+        kid: JWKIdentifier? = nil
+    ) async throws -> String {
+        try signers.sign(payload, kid: kid)
+    }


This variant is marked as deprecated despite appearing on the new JWTKeyCollection to help push users to adopt AsyncJWTPayload wherever they use another payload type.

dimitribouniol · 2024-11-20T13:51:21Z

+/// A transitionary protocol with sync and async requirements.
+///
+/// This protocol should be dropped once you are finished migrating to v5, as it'll have been renamed back to ``JWTPayload``, but with a single async requirement. In order to support both versions v4 and v5 in a library, do not implement the requirements of ``JWTPayload`` as ``JWTSigner`` is no longer available in v5.
+public protocol AsyncJWTPayload: Codable {


As for the name, I'm open to alternatives. JWTPayloadV4 could work well since this variant of it only really exists in the v4 version of the codebase.

Nah AsyncJWTPayload is fine, it matches with the rest of Vapor

dimitribouniol · 2024-11-20T13:51:50Z

+    func payload<Payload>(as payload: Payload.Type, jsonDecoder: any JWTJSONDecoder) throws -> Payload
+        where Payload: AsyncJWTPayload
+    {
+        try jsonDecoder
+            .decode(Payload.self, from: .init(self.encodedPayload.base64URLDecodedBytes()))
+    }


This is a duplicate of the above variant to support the new protocol.

dimitribouniol · 2024-11-20T13:52:24Z

        jsonEncoder: any JWTJSONEncoder
    ) throws -> String
-        where Payload: JWTPayload
+        where Payload: Encodable


Similarly, rather than duplicate this internal method, I loosened the protocol requirements to it works for both.

dimitribouniol · 2024-11-20T13:53:43Z

+public enum ECDSA: Sendable {
+    /// ECDSA.PublicKey was introduced in v5 and replaces ``ECDSAKey``.
+    ///
+    /// - Note: Please migrate over to ``ECDSA/PublicKey`` before updating to v5, though if you plan on remaining on v4, ``ECDSAKey`` can continue to be used.
+    public struct PublicKey<Curve: ECDSACurveType> {
+        let key: ECDSAKey
+        init(key: ECDSAKey) { self.key = key }


These are wrappers on the existing keys to support the new types a user would be forced into, wile not adding much new functionality. I added bare minimum support for all ECDSA public and private keys.

dimitribouniol · 2024-11-20T13:54:35Z

+class JWTKitMigrationTests: XCTestCase {
+    func testVerifyingCryptoKey() async throws {


An end to end test to validate everything is hooked up properly. Also tested the migrations in the app-store-server-library repo along with my own codebase.

0xTim · 2024-11-20T14:02:33Z

+public typealias ES256PublicKey = ECDSA.PublicKey<P256>
+public typealias ES256PrivateKey = ECDSA.PrivateKey<P256>
+
+extension P384: ECDSACurveType, @unchecked @retroactive Sendable {


These will need versions for Swift 5 and Swift 6

I thought it wouldn’t, because this version of the library would always be compiled with the Swift 5 language mode. Once v5 is used, then none of this is included.

Ah, nvm, forgot retroactive didn’t exist pre swift 6 compiler, will update

Fixed: https://github.com/vapor/jwt-kit/compare/fba14c1e0ff9f97b57ac77d8ae0d4d1857d830f3..83afce04c6740c40431d88f0cea0d57a50df828d

Though I'm now wondering if its worth adding Sendable conformances at all, since the rest of the library didn't support it yet at all…

I don't mind not having them tbh, anyone wanting a full sendable supported library can bump to v5

…version 4 to version 5

0xTim · 2024-12-01T13:48:32Z

Added a couple of brief comments. There's also a failing test we need to investigate

0xTim

This looks good to me, I don't know if anyone wants to comment but this is probably good to merge soon

dimitribouniol · 2024-12-07T07:45:38Z

Still need to fix the tests, but let me get PRs for the other pieces in place before we merge so they can go in all at once

dimitribouniol requested review from 0xTim and gwynne as code owners November 20, 2024 13:45

dimitribouniol commented Nov 20, 2024

View reviewed changes

dimitribouniol mentioned this pull request Nov 20, 2024

Minimal Update to JWTKit 5 apple/app-store-server-library-swift#68

Merged

0xTim reviewed Nov 20, 2024

View reviewed changes

Added migratory types with deprecations to guide users updating from …

83afce0

…version 4 to version 5

dimitribouniol force-pushed the dimitri/v5-deprecations branch from fba14c1 to 83afce0 Compare November 21, 2024 10:02

0xTim approved these changes Dec 7, 2024

View reviewed changes

		class JWTKitMigrationTests: XCTestCase {
		func testVerifyingCryptoKey() async throws {

Uh oh!

Conversation

dimitribouniol commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xTim commented Dec 1, 2024

Uh oh!

0xTim left a comment

Choose a reason for hiding this comment

Uh oh!

dimitribouniol commented Dec 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dimitribouniol commented Nov 20, 2024 •

edited

Loading