SSWG-0029: JWSETKit

The review of SSWG-0029: JWSETKit begins now and runs for two weeks, until February 12th, 2024.


Reviews are an important part of the Swift Server Work Group incubation process . All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager (via email) or direct message in the Swift forums.

The package is being proposed at the Sandbox maturity level. Please note the Maturity Justification inside the proposal for details and indicate which you find more appropriate as part of your review.

What goes into a review of a proposal?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, become listed on the Swift Server Ecosystem index page .

When reviewing a proposal, here are some questions to consider:

  • What is your evaluation of the proposal and its APIs/implementation?
  • Is the problem being addressed relevant for the wider Swift on Server ecosystem?
  • Does this proposal fit well with the feel and direction of the Swift Server ecosystem?
  • If you have used other libraries in Swift or other languages, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Konrad 'ktoso' Malawski
Review Manager

3 Likes

Not sure if it's correct place or should start an issue on Github, but will write first here.

Was going through the code yesterday as library is indeed needed, and noticed it's using AnyCodable. Could be my personal tastes, but think it should be more strict here, cause there are lines in JSONWebValueStorage which breaks type contract for me, like:

    private var storage: [String: AnyCodable]

    switch value {
        case let value as any JSONWebFieldEncodable:
            storage[key] = .init(value.jsonWebValue)
        case let value as any Decodable:
            storage[key] = .init(value)
        default:
            remove(key: key)
            assertionFailure("Unknown storage type")
        }

AnyCodable is both Encodable and Decodable, why passing just Encodable or Decodable should work then? :thinking:
Removing AnyCodable, passing generic type and being strict on types feels like a better choice in Swift ecosystem, as JSONWebValueStorage is public and exposed for others to use.

1 Like

SSWG reviews are primarily about the criteria mentioned on Swift.org - SSWG Incubation Process as "requirements".

Having that said, technical best practices are part of that.
Any feedback / reviews that can help access and improve the library are welcome process wise.

1 Like

Thanks @jaleel for you feedback.

Please check changes in this branch GitHub - amosavian/JWSETKit at generic-codable.

Will have a look in upcoming days and overall more deeply in codebase. But thanks for quick response!

1 Like

In general, I am a +1 on incubating a JWT package since JWX is commonly used in server applications (leaving aside that they are often the wrong tool for the job). This package is missing a few minimum requirements though:

  • From what I can see it hasn't adopted the Swift code of conduct
  • Longevity: The package appears to be maintained by just @amosavian
  • Dependencies: The package is using CryptoSwift which contains unvetted cryptographic implementations; hence, we avoid using this in any SSWG incubating package.

Until we resolve the above issues I personally think we shouldn't go forward with incubating this package. After the issues have been resolved we can send it for another round of review.

2 Likes

Thanks @FranzBusch,

  • Code of conduct has been added to project.
  • I'm open to add other maintainers or transfer project to SSWG.
  • CryptoSwift is used to support PBKDF2 and RSA-PKCS1.5 encryption:
    • We can use a implementation of PBKDF inside the repo as oracle-nio did.
    • I can remove RSA-PKCS1.5 support on Linux as _CryptoExtras module does not support this padding..

Edit: Please check GitHub - amosavian/JWSETKit at remove-cryptoswift for changes in order to remove CryptoSwift

1 Like

I feel this is an issue - having different feature sets for Linux and macOS is likely to cause confusion for users and goes against the ethos of the SSWG of having consistent functionality on all platforms

1 Like

Hi @0xTim

There are few algorithms which are not supported in Apple's Crypto/CryptoExtras:

  • PBKDF2
  • RSA1_5 (RSA-PKCS1.5)
  • RSA-OAEP-384
  • RSA-OAEP-512

I found no other Swift library than CryptoSwift which supports Linux and implements these algorithms. These algorithms are supported by CommonCrypto and BoringSSL and can be added to Crypto with no significant effort.

1 Like

Sure but with the current state of things - having different features on different platforms is probably a no go, more than any implementation details IMO

1 Like

I concur with Tim - missing features across platforms feels like a non-starter. JWTKit v5 achieves feature parity via SwiftCrypto, so the baseline seems reasonable.

:tophat: Chiming in with review manager hat on:

Sorry for the long wait here @amosavian. We're voting now and will reach a conclusion shortly.

It should be clear though that the criteria for Sandbox are specifically those listed here: Swift.org - SSWG Incubation Process and there is no part about completeness or feature-set judgement.

The feedback of the team is valuable nevertheless though, we have concerns about the feature set, but it is not a requirement of the Sandbox level so it might be accepted nevertheless.

We did a round of discussions in the group with regards to this proposal; and general SSWG rules, so allow me to summarize them here.


:white_check_mark: The SSWG has decided to accept this proposal into Sandbox maturity level!

Specifically, this decision is guided by the project fulfilling all requirements of the Sandbox level, and has an active active stream of development.

:exclamation: At the same time however, we are concerned about the feature parity between platforms, and going forward we are going to start a discussion and may change SSWG maturity level requirements to reflect that packages should have platform parity.

Also, SSWG endorsed projects should prefer the "vetted" SSWG endorsed SwiftCrypto package for their cryptography needs when applicable.

Since we did not have rules about such parity, or requirements regarding cryptography at the time of this review. We review it using the current rules. If and when we were to adopt a new rule in the maturity levels, we will re-review packages again.

Therefore , we would strongly suggest getting this package up to feature parity, either by feature removal (as proposed in !feat: Remove CryptoSwift (PKCS1.5 encryption support in Linux) ยท amosavian/JWSETKit@b8256c8 ยท GitHub), or by figuring out how to implement the missing logic using SwiftCrypto. @lukasa has been open to accepting new algorithms into the "_CryptoExtras" package that accompanies SwiftCrypto; so aiming to get an implementation there and use it would be the best outcome here.

Another related discussion re algorithms recently happened over here: [Pitch] OracleNIO: Oracle DB Driver built on SwiftNIO -- specifically discussing PBKDF2 which could be added to _CryptoExtras.

We'll add the project to the list of SSWG endorsed projects shortly.

1 Like

To clarify to this: PBKDF2 can be added subject to the caveats discussed in the PR linked above. RSA OAEP with the two extended SHA functions should be addable in the current API.

RSA encryption with PKCS1v1.5 padding is a potential sticking point. This is a dangerous primitive that is frequently misused. I think we need to seriously ask ourselves whether this is a necessary part of our ecosystem, or whether we can try to hold a line on avoiding even having it.

2 Likes