Use JWTKit where possible #95
Conversation
|
|
||
| if (environment == AppStoreEnvironment.xcode || environment == AppStoreEnvironment.localTesting) { | ||
| // Data is not signed by the App Store, and verification should be skipped | ||
| // Data is not signed by the App Store, and verification should be skipped. |
There was a problem hiding this comment.
Nit unnecessary addition
| // Data is not signed by the App Store, and verification should be skipped. | ||
| // The environment MUST be checked in the public method calling this | ||
| return VerificationResult.valid(decodedBody) | ||
| return VerificationResult.valid(payload) |
There was a problem hiding this comment.
Nit unnecessary trailing whitespace
| guard let x5c_header = header.x5c else { | ||
| return VerificationResult.invalid(VerificationError.INVALID_JWT_FORMAT) | ||
| } | ||
| if ChainVerifier.EXPECTED_ALGORITHM != header.alg || x5c_header.count != ChainVerifier.EXPECTED_CHAIN_LENGTH { |
There was a problem hiding this comment.
Where is the algorithm now checked?
| if online { | ||
| if case .validCertificate = verificationResult { | ||
| verifiedPublicKeyCache[CacheKey(leaf: leaf, intermediate: intermediate)] = CacheValue(expirationTime: getDate().addingTimeInterval(TimeInterval(integerLiteral: ChainVerifier.CACHE_TIME_LIMIT)), publicKey: verificationResult) | ||
| if case .validCertificate(_) = verificationResult { |
There was a problem hiding this comment.
Nit unnecessary change, this looks like just reformatting, could we please keep the reformatting a such to a minimum in this PR (if we just want a reformatting PR that would be fine)
| let intermediateCertificate = try Certificate(derEncoded: Array(intermeidate_der_encoded)) | ||
| let validationTime = !onlineVerification && decodedBody.signedDate != nil ? decodedBody.signedDate! : getDate() | ||
|
|
||
| let verificationResult = await verifyChain(leaf: leafCertificate, intermediate: intermediateCertificate, online: onlineVerification, validationTime: validationTime) |
There was a problem hiding this comment.
This seems to remove any use of verifyChain and verifyChainWithoutCaching? How is this handling caching of these certificates
| } | ||
| } | ||
|
|
||
| extension AppTransaction: JWTPayload { |
There was a problem hiding this comment.
Is it possible to extend or modify DecodedSignedData in some way to keep this more flexible?
|
@ptoffy Sorry for the slow review, left comments, main one relates to how this seems to remove cert caching |
2 participants
This PR aims to reduce the custom JWT/JWS code in the library since most of the functions are already present in JWTKit. X5C verification is replaced with JWTKit's
X5CVerifier(besides custom rules, for example the mandatory x5c certificate count).Custom JWT signing is also replaced with JWTKit.