Skip to content
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

CORE-18635 Avro schemas for session encryption operations #1400

Merged
merged 10 commits into from
Dec 19, 2023

Conversation

YashNabar
Copy link
Contributor

@YashNabar YashNabar commented Dec 13, 2023

APIs for an implementation of encryption and decryption.

Runtime pull request in corda/corda-runtime-os#5258

@corda-jenkins-ci02
Copy link
Contributor

Scanning for breaking API changes introduced by this PR

Scan Succeeded

@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Dec 13, 2023

Jenkins build for PR 1400 build 7

Build Successful:
Jar artifact version produced by this PR: 5.2.0.18-alpha-1702919807911

@yift-r3 yift-r3 marked this pull request as ready for review December 15, 2023 11:16
@yift-r3 yift-r3 requested a review from a team as a code owner December 15, 2023 11:16
@yift-r3 yift-r3 requested review from a team December 15, 2023 11:16
# Conflicts:
#	gradle.properties
Copy link
Contributor

@anien anien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor suggestion, as I found it easier to read if the order is consistent.

Comment on lines 4 to 5
"doc": "Error result while performing encryption or decryption operation.",
"namespace": "net.corda.data.crypto.wire.ops.encryption.response",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"doc": "Error result while performing encryption or decryption operation.",
"namespace": "net.corda.data.crypto.wire.ops.encryption.response",
"namespace": "net.corda.data.crypto.wire.ops.encryption.response",
"doc": "Error result while performing encryption or decryption operation.",

Comment on lines 14 to 15
"doc": "The symmetric key alias.",
"type": ["null", "string"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"doc": "The symmetric key alias.",
"type": ["null", "string"]
"type": ["null", "string"]
"doc": "The symmetric key alias.",

Comment on lines 14 to 15
"doc": "The symmetric key alias.",
"type": ["null", "string"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"doc": "The symmetric key alias.",
"type": ["null", "string"]
"type": ["null", "string"]
"doc": "The symmetric key alias.",

dimosr
dimosr previously approved these changes Dec 18, 2023
anien
anien previously approved these changes Dec 18, 2023
Copy link
Contributor

@anien anien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

"doc": "The data to decrypt."
},
{
"name": "context",
Copy link
Contributor

@kyriathar kyriathar Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding this context was was decided at design level.

However, I have seen before a case where we have been using it while we shouldn't and we had to go in there and revert all of that, to use some other field instead back then (IRRC we used to pass the signature spec in context. This piece of work can be found here).

I would say if you are not planning on using it remove it as it will cause at the very least confusion on the API, like what is good for. It can always be added later if found it's needed which I doubt It will be needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to remove it.

@dickon WDYT?

"doc": "Request to decrypt the given byte array",
"fields": [
{
"name": "category",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: so this means we could be passing in any category. So for example LEDGER (ledger keys are meant to be used for signing) to decrypt secrets. Should we be restricting this field with an enum of valid categories?

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link
Contributor

@kyriathar kyriathar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK (crypto)

@yift-r3 yift-r3 merged commit 01782d0 into release/os/5.2 Dec 19, 2023
6 checks passed
@yift-r3 yift-r3 deleted the yash/CORE-18635 branch December 19, 2023 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants