-
Notifications
You must be signed in to change notification settings - Fork 11
feat: add verified content to v3 #121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
||
| return returnedObject | ||
| } catch (err) { | ||
| return null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: in crypto.ts,
} catch (err) {
// Should only throw if MissingPublicKeyError.
// This library should be able to be used to encrypt and decrypt content
// if the content does not contain verified fields.
if (err instanceof MissingPublicKeyError) {
throw err
}
return null
}
The missing public key error is thrown to the client - should we perhaps be doing the same thing for cryptov3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to keep it consistent between the versions. I'll throw the error from decryptFromSubmissionKey and to decrypt as well.
Tbh We don't handle these errors in our code, where we only check for null and return another error else. But that's for later when we update app implementation
|
|
||
| // Act | ||
| const ciphertext = crypto.encrypt(plaintext, publicKey) | ||
| const verifiedText = cryptoV1.encrypt(plainVerifiedText, ciphertext.submissionPublicKey, signingSecretKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Could we perhaps leave a comment about the rationale behind why cryptoV1 is used for decrypting verifiedContent here?
It might be a little confusing as to why cryptoV1 is being used in the cryptoV3 test file for new engs.
Perhaps something explaining the below comment (but probably more concise).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question - confirm if this is accurate:
For storage mode forms, the verifiedContent is added outside of the main encryptedContent.
For MRF, this verifiedContent is not yet implemented.
Thinking about its implementation and extensibility to MRF singpass for multiple steps. (crucial since changes to SDK usually can be added but hard to be taken away).
For singpass with MRF on the FormSG app side, my understanding is that since it is only for 1st step - we can use cryptoV1 to encrypt with the current step's submission public key.
For multiple step MRFs, we can decrypt with the prev step's submission public key and then re-encrypt for each subsequent step with the latest submission public key.
However, if we use cryptoV1 for verified content and use the submission steps's latest submission key - would admins still be able to decrypt the verified content with the form private key using decrypt in cryptoV3?
- yes, since can pass in {encryptedSubmissionSecretKey, ...verifiedContent} as the DecryptParams and the same
encryptMessageis used in v1 and v3 - allowing the decrypt in v3 to also decrypt ciphertexts using v1.
Hence, this change is safe to make. But there likely will need to be explicit logic to do this:
For multiple step MRFs, we can decrypt with the prev step's submission public key and then re-encrypt for each subsequent step with the latest submission public key.
in the application code.
.github/workflows/ci.yml
Outdated
| NODE_OPTIONS: '--max-old-space-size=8192' | ||
| - name: Submit test coverage to Coveralls | ||
| uses: coverallsapp/[email protected] | ||
| - name: Submit test coverage to Coveralls-next |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: curious - why was this updated due to the previous coveralls failing?
could it be due to v1.1.2 which is outdated - our formsg app is using v2. perhaps using v2 to match the main formsg app would be easier to maintain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Standardization sounds good! Reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a good look at it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last thing before we merge this change!
| } | ||
|
|
||
| /** | ||
| * Note on verifiedContent decryption for cryptoV3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Note on verifiedContent decryption for cryptoV3: | |
| * Note on verifiedContent decryption for cryptoV3: | |
| * Although decryption is supported, verifiedContent encryption is not supported | |
| * in cryptoV3 encrypt. | |
| * This is to keep the encryption of verifiedContent and encryptedContent similar to storage mode - where | |
| * verifiedContent and encryptedContent are defined and encrypted separately. |
| "@types/node": "^18.18.9", | ||
| "@typescript-eslint/eslint-plugin": "^4.25.0", | ||
| "auto-changelog": "^2.4.0", | ||
| "coveralls": "^3.1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Thanks for identifying and removing this! 🎉 Always happy to remove stuff -> less things to maintain!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Problem
MRF decryption doesn't support decryption of verifiedContent. This is needed to support Singpass integrations with MRF as Ndi data is stored in
verifiedContent, separate fromencryptedContentNdi data mainly refers to
and varies depending on the Singpass form auth type used.
This ndi data is needed for example to know if the submitter is authorised to submit a form - eg, nric whitelisting.
As part of MRF singpass support, some changes are made:
Why?
There are some pros to using form public key for encryption, as this means that subsequent steps cannot decrypt and see the submitter id content stored in verified content (1).
However, to support multi step singpass for MRF, this submitter id content is needed - hence form public key cannot be used.
Hence, submissionPublicKey is used at the expense of (1), where subsequent steps (and submitters) may be able to decrypt the verified content
This is done as the previous storage mode is implemented the same way.
First, the form responses are encrypted with the form public key (encryptedContent).
Then, the verified content are encrypted with the form public key (verifiedContent).
These 2 items are separately generated and encrypted are then added to a common object and saved into the same database document.
For MRF:
a) First, the form responses will be encrypted with the submissionPublicKey (per step) using v3's encrypt. Generating a new submissionPublicKey.
b) Then, the verified content will be separately encrypted with the form submissionPublicKey. For each step, the verifiedContent will be decrypted using the previous submissionPublicKey and then new info for that step will be appended before being re-encrypting using the new submissionPublicKey.
While it is possible to combine step b) and a) together, by expanding v3's encrypt to support encryption of verifiedContent as well. For now, a decision has been made to keep it separate to follow the implementation of v1 more closely.
Closes FRM-2156
Solution
verifiedContentto input types &verifiedto output type for cryptoV3 functionsverifiedContentis present, decryptverifiedContentwith submissionSecretKey and throw errors if unsuccessfulverifiedContentnot present, skip decryption and return onlyencryptedContent(backwards compatible)Before & After Screenshots
Screenshot showing accurate decryption of a MRF submission data (see console & note successful decrypted verifiedContent
Dependency Updates
The
coveralls 3.1.1package was causing a security vulnerability. It was preventing a transient packageform-datafrom being upgraded (was2.3.3), whereby this older version was using an unsafeMath.random()to choose boundaries, which can be predictable by an attacker given enough sequences of outputs.Updated sdk to use
coveralls-nextwhich is a fork ofcoveralls(no longer maintained).formsh-sdkis bumped tov4.0.4.