Critical code review PR - Last sprint handover checklist of 1.4.0 release#165
Critical code review PR - Last sprint handover checklist of 1.4.0 release#165Prafulrakhade merged 0 commit intomasterfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if: ${{ inputs.release_type == 'snapshot' }} | ||
| uses: mosip/kattu/.github/workflows/maven-publish-android.yml@master | ||
| with: | ||
| SERVICE_LOCATION: 'vc-verifier/kotlin' | ||
| ANDROID_SERVICE_LOCATION: 'vcverifier' | ||
| JAVA_VERSION: 21 | ||
| LICENSE_NAME: 'MPL-2.0' | ||
| RELEASE_TYPE: ${{ inputs.release_type }} | ||
| secrets: | ||
| OSSRH_USER: ${{ secrets.OSSRH_USER }} | ||
| OSSRH_URL: ${{ secrets.OSSRH_CENTRAL_URL }} | ||
| OSSRH_SECRET: ${{ secrets.OSSRH_SECRET }} | ||
| OSSRH_TOKEN: ${{ secrets.OSSRH_TOKEN }} | ||
| GPG_SECRET: ${{ secrets.GPG_SECRET }} | ||
| SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_INJI_TEAM }} | ||
|
|
||
| publish-release: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions
vishwa-vyom
left a comment
vishwa-vyom
left a comment
There was a problem hiding this comment.
Few initial queries on scope
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/CredentialsVerifier.kt
Outdated
Show resolved
Hide resolved
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/PresentationVerifier.kt
Show resolved
Hide resolved
...lin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/publicKey/PublicKeyGetterFactory.kt
Show resolved
Hide resolved
| val disclosures = sdJwt.disclosures | ||
| val keyBindingJwt = sdJwt.bindingJwt | ||
|
|
||
| validateSDJwtStructure(credentialJwt, disclosures) |
There was a problem hiding this comment.
In the class name, Sd is followed and in the method name SD is followed, any specific reason for this inconsistency ?
There was a problem hiding this comment.
we can make it validateSdJwtStructure It's just a mistake
There was a problem hiding this comment.
|
|
||
| val payload = JSONObject(decodeBase64Json(parts[1])) | ||
| validateKeyBindingPayload(payload) | ||
| } |
There was a problem hiding this comment.
In our testing, we have tested the key binding aspect also ?
There was a problem hiding this comment.
It will be tested during OVP flow. Right now we have tested with unit tests
| val header = jwsObject.header | ||
|
|
||
| if (header.x509CertChain.isEmpty()) { | ||
| throw IllegalArgumentException("No X.509 certificate chain found in JWT header") |
There was a problem hiding this comment.
Critical: This validation is not correct, SD-JWT can have other signature securing mechanisms.
Ref: https://datatracker.ietf.org/doc/draft-ietf-oauth-sd-jwt-vc/ (Section 3.5). As per certify, we also have support for DID based once.
There was a problem hiding this comment.
There was a problem hiding this comment.
@vishwa-vyom created separate card to add support for other mechanism https://mosip.atlassian.net/browse/INJIMOB-3541
| val urlSafeBase64Certificate = certBase64.replace("\\s+".toRegex(), "") | ||
| .replace('+', '-') | ||
| .replace('/', '_') | ||
| val certificateBytes = Base64Decoder().decodeFromBase64Url(urlSafeBase64Certificate) |
There was a problem hiding this comment.
I see we are converting normal base64 to base64 url and then using the base64 decoding method. Can we not have a method which does base64 directly, which can avoid all the above replaces ?
There was a problem hiding this comment.
There was a problem hiding this comment.
@vishwa-vyom It was due to reuse existing decode base64 url encoded method.
Added new method to decode base64 encoded data 5694fe1
| val dateFormats = listOf( | ||
| private val dateFormats = listOf( | ||
| ("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"), | ||
| ("yyyy-MM-dd'T'HH:mm:ss'Z'") |
There was a problem hiding this comment.
As per the spec, is it not valid if we use the timezone specific times ?
There was a problem hiding this comment.
There was a problem hiding this comment.
| JWS_EDDSA_SIGN_ALGO_CONST to ED25519SignatureVerifierImpl(), | ||
| JWS_ES256K_SIGN_ALGO_CONST to ES256KSignatureVerifierImpl(), | ||
| JWS_ES256_SIGN_ALGO_CONST to ES256KSignatureVerifierImpl() | ||
| ) |
There was a problem hiding this comment.
This looks like a factory of verifier, any reason to keep it in util class ?
There was a problem hiding this comment.
Prafulrakhade
force-pushed
the
master
branch
from
15af01c to
2a8e353
Compare
3 participants
This PR is created just to add the review comments as part of the critical code review task of last sprint handover checklist for release of 1.4.0 version.
** THIS PR SHOULD NOT BE MERGED **