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

[ADP-322] Add metadata encryption to HTTP API. #4555

Merged

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented Apr 19, 2024

  • Adding noninvasive api changes to constructTransaction
  • Applying changes to Api.Server
  • Extend ErrConstructTx
  • Implementing toMetadataEncryption according to https://github.com/cardano-foundation/CIPs/tree/master/CIP-0083
  • Prepare goldens in line to openSSL and CIP83 examples
  • Show error emitting works
  • Add salting to key gen and encryption
  • Add integration test showing error
  • Add integration test showing encryption

Comments

Issue Number

adp-322

@paweljakubas paweljakubas self-assigned this Apr 19, 2024
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-322/add-metadata-encryption-to-api branch 3 times, most recently from fa84e60 to 1842e52 Compare April 25, 2024 12:57
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-322/add-metadata-encryption-to-api branch 2 times, most recently from 0cb0d8b to 78798e7 Compare May 6, 2024 06:42
@jonathanknowles jonathanknowles force-pushed the paweljakubas/adp-322/add-metadata-encryption-to-api branch 2 times, most recently from fb38fbf to c2d8bb9 Compare May 7, 2024 08:29
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-322/add-metadata-encryption-to-api branch from eda0d55 to 1c48366 Compare May 10, 2024 09:46
@jonathanknowles jonathanknowles changed the title Add metadata encryption to api Add metadata encryption to HTTP API. May 14, 2024
lib/crypto-primitives/src/Cryptography/Cipher/AES256CBC.hs Outdated Show resolved Hide resolved

data ApiEncryptMetadata = ApiEncryptMetadata
{ passphrase :: ApiT (Passphrase "lenient")
, enc :: Maybe EncryptMetadataMethod
Copy link
Member

Choose a reason for hiding this comment

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

Hi @paweljakubas

For what reason do we allow this field to be Nothing?

If it is set to Nothing, then what do we expect the behaviour to be?

if txt == "basic" then
pure AES256CBC
else
fail "'basic' is expected."
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: derive this instance automatically.

See: #4588

lib/api/src/Cardano/Wallet/Api/Http/Shelley/Server.hs Outdated Show resolved Hide resolved
Cipher algorithm uses 8-byte salt, PKCS#7 padding as specified in
https://datatracker.ietf.org/doc/html/rfc5652#section-6.3 is applied.
Only metadata value under `msg` field is encrypted. If `msg` field is missing error
will be emitted.
Copy link
Member

Choose a reason for hiding this comment

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

As a user who is new to this feature, I'm finding this description section a little hard to follow!

For example, it refers to a field called msg. However, this object only has two fields: enc and passphrase.

From reading through the tests (Api.TypesSpec), I can see that msg is supposed to be the key in a TxMetaMap.

Is there any way that we can give a more complete description of how to use this feature? (Ideally, so that the user doesn't have to go hunting through CIP 83.)

@jonathanknowles jonathanknowles changed the title Add metadata encryption to HTTP API. [ADP-322] Add metadata encryption to HTTP API. May 22, 2024
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-322/add-metadata-encryption-to-api branch from fbfb5a7 to 556012e Compare June 11, 2024 14:34
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-322/add-metadata-encryption-to-api branch from 556012e to 29d71c2 Compare June 17, 2024 07:47
@jonathanknowles jonathanknowles force-pushed the paweljakubas/adp-322/add-metadata-encryption-to-api branch from dc28dc5 to 70a1c46 Compare June 20, 2024 05:52
paweljakubas added a commit that referenced this pull request Jun 24, 2024
This PR contains suggestions for PR #4555.
paweljakubas added a commit that referenced this pull request Jun 26, 2024
This PR contains suggestions for PR #4555.
Copy link
Member

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

Hi @paweljakubas

Many thanks for creating this PR and for your patience with waiting for reviews.

Given this PR adds just the encryption functionality, with decryption planned for a later PR, I think this PR is okay to merge for now.

However, I do have some proposals that I think should be addressed in subsequent PRs:

  1. Currently, the "business logic" for metadata encryption is located within the Cardano.Wallet.Api.Http.Shelley.Server module. I would propose to extract this logic out into a pure function that (a). doesn't rely on HTTP API types and (b). lives in a more appropriate location, preferably outside of the Server module, so that the Server module only needs to handle concerns that directly relate to the HTTP API. Perhaps this new location could be something like Cardano.Write.Tx.Metadata.Encryption?

  2. I would propose to replace all instances of the magic value 674 with references to the cip20MetadataKey constant (already used by toMetadataEncrypted), and similarly, to re-locate this constant to the proposed Cardano.Write.Tx.Metadata.Encryption module.

  3. When we have a pure function that performs decryption, I would propose to:

    1. add a new test module Cardano.Write.Tx.Metadata.EncryptionSpec.
    2. add a QuickCheck property test for the encryption-decryption round-trip property: decrypt . encrypt == id.
    3. add a selection of golden example-based unit tests (i.e., tests that are very quick to run that don't require us to fire up the integration test suite).
  4. I still find the description of x-encryptMetadata in the swagger file a little hard to follow. Perhaps it would be possible to find someone who is completely new to this feature, and work with them to revise this description in a way that is easier for new users to understand? (See earlier review comment: https://github.com/cardano-foundation/cardano-wallet/pull/4555/files#r1599427620)

Again, many thanks again for creating this PR!

Jonathan

specifications/api/swagger.yaml Outdated Show resolved Hide resolved
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-322/add-metadata-encryption-to-api branch from 0bb3ed6 to a83c7e1 Compare June 26, 2024 13:52
@paweljakubas paweljakubas added this pull request to the merge queue Jun 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 26, 2024
@paweljakubas paweljakubas added this pull request to the merge queue Jun 26, 2024
Merged via the queue into master with commit a2cac93 Jun 26, 2024
3 checks passed
@paweljakubas paweljakubas deleted the paweljakubas/adp-322/add-metadata-encryption-to-api branch June 26, 2024 15:32
WilliamKingNoel-Bot pushed a commit that referenced this pull request Jun 26, 2024
…ail in a few bullet points the work accomplished in this PR. Before you submit, don't forget to: CODE-OF-CONDUCT.md CONTRIBUTING.md LICENSE MAINTAINERS.md README.md cabal.project configs docker-compose.yml docs flake.lock flake.nix floskell.json fourmolu.yaml hie-direnv.yaml justfile lib nix prototypes reports scripts specifications test touch.me.CI weeder.dhall Make sure the GitHub PR fields are correct: ✓ Set a good Title for your PR. ✓ Assign yourself to the PR. ✓ Assign one or more reviewer(s). ✓ Link to a Jira issue, and/or other GitHub issues or PRs. ✓ In the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. CODE-OF-CONDUCT.md CONTRIBUTING.md LICENSE MAINTAINERS.md README.md cabal.project configs docker-compose.yml docs flake.lock flake.nix floskell.json fourmolu.yaml hie-direnv.yaml justfile lib nix prototypes reports scripts specifications test touch.me.CI weeder.dhall Don't waste reviewers' time: ✓ If it's a draft, select the Create Draft PR option. ✓ Self-review your changes to make sure nothing unexpected slipped through. CODE-OF-CONDUCT.md CONTRIBUTING.md LICENSE MAINTAINERS.md README.md cabal.project configs docker-compose.yml docs flake.lock flake.nix floskell.json fourmolu.yaml hie-direnv.yaml justfile lib nix prototypes reports scripts specifications test touch.me.CI weeder.dhall Try to make your intent clear: ✓ Write a good Description that explains what this PR is meant to do. ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding Jira ticket. ✓ Highlight what Testing you have done. ✓ Acknowledge any changes required to the Documentation. --> - [x] Adding noninvasive api changes to `constructTransaction` - [x] Applying changes to Api.Server - [x] Extend `ErrConstructTx` - [x] Implementing `toMetadataEncryption` according to https://github.com/cardano-foundation/CIPs/tree/master/CIP-0083 - [x] Prepare goldens in line to openSSL and CIP83 examples - [x] Show error emitting works - [x] Add salting to key gen and encryption - [x] Add integration test showing error - [x] Add integration test showing encryption ### Comments <!-- Additional comments, links, or screenshots to attach, if any. --> ### Issue Number adp-322 <!-- Reference the Jira/GitHub issue that this PR relates to, and which requirements it tackles. Note: Jira issues of the form ADP- will be auto-linked. --> Source commit: a2cac93
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.

None yet

2 participants