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?
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 EncodableandDecodable, why passing just EncodableorDecodable should work then?
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.
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.
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.
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
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.
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
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.
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.
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.
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.
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.
Hey @lukasa, RSAES-PKCS1-v1.5 would be deprecated soon by draft-madden-jose-deprecate-none-rsa15-00. I'm not sure we should keep it as a deprecated protocol or just remove it as it is unsafe to add it to swift-crypto.
Chiming in with review manager hat on:
The SSWG has decided to accept this proposal into Sandbox maturity level!
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.