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.

Sign up for GitHub

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

Jump to bottom

Export identity data #77

Open
wants to merge 10 commits into
base: browser-wallet-support
Choose a base branch
from
Open

Export identity data #77

wants to merge 10 commits into from

Conversation

shjortConcordium
Copy link
Contributor

Purpose

Allow the app to export all identity data:

  • PRF-key
  • IdCredSec
  • Signature retrieval / blinding randomness
  • Attribute randomness

Changes

  • Added new paths
  • Made flows to export the data
  • Made a flow to allow all identity data to be exported without confirmation.

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

@shjortConcordium shjortConcordium requested a review from orhoj March 29, 2023 09:07
@shjortConcordium shjortConcordium changed the base branch from main to browser-wallet-support April 27, 2023 08:43
orhoj
orhoj suggested changes Apr 27, 2023
View reviewed changes
Makefile Outdated
@@ -45,7 +45,9 @@ APPVERSION=$(APPVERSION_MAJOR).$(APPVERSION_MINOR).$(APPVERSION_PATCH)
APP_LOAD_PARAMS = --appFlags 0x00 $(COMMON_LOAD_PARAMS)

# Restrict derivation paths to the Concordium specific path.
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
# Restrict derivation paths to the Concordium specific path.
# Restrict derivation paths to the Concordium specific paths.

src/exportData.h Outdated
#define _CONCORDIUM_APP_EXPORT_DATA_H_

/**
* Handles the export of private keys that are allowed to leave the device.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should state what private key material that is.

src/exportData.h Outdated
* @param p2 If set to 0x01, then the seeds are exported (Using this is deprecated). If set to 0x02, then the BLS keys
* are exported. 0x00 is not used to ensure that old clients fail when calling this functionality.
*/
void handleExportData(uint8_t *dataBuffer, uint8_t p1, uint8_t p2, volatile unsigned int *flags, bool isInitialCall);
Copy link
Contributor

Choose a reason for hiding this comment

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

handleExportData is a little too generic. I suggest something like handleExportPrivateKeyData or something that makes it clear that it contains private key material. Export data could be anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to handleExportPrivateIdentityData, because not everything are really keys. 🤔

src/exportData.h Outdated
* The export paths are restricted so that the method cannot access any account paths.
* @param p1 has to be 0x00 for export of PRF key for decryption, 0x01 for export of PRF key for recovering credentials
* and 0x02 for export of PRF key and IdCredSec.
* @param p2 If set to 0x01, then the seeds are exported (Using this is deprecated). If set to 0x02, then the BLS keys
Copy link
Contributor

Choose a reason for hiding this comment

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

This is new functionality so we should not have to support the deprecated functionality, and we don't need to reserve 0x00 either. Perhaps the documentation here is just outdated (or copied from another method).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the entire doc for that function was copied. 🤔

src/verifyAddress.c
@@ -120,9 +120,10 @@ void handleVerifyAddress(uint8_t *cdata, volatile unsigned int *flags) {
uint32_t credCounter = U4BE(cdata, 4);
getIdentityAccountDisplay(ctx->display, sizeof(ctx->display), identity, credCounter);

// TODO use P1 to support new version
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a task that covers this TODO?

Copy link
Contributor Author

@shjortConcordium shjortConcordium May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have a separate PR open for this.
#83

doc/export_data.md Outdated
@@ -0,0 +1,39 @@
# Export data

This method allows exporting various data for the identity. This data does not include not account signing keys, and note that it is not possible to export keys that are used for signatures.
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
This method allows exporting various data for the identity. This data does not include not account signing keys, and note that it is not possible to export keys that are used for signatures.
This method allows exporting various data for the identity. This data does not include account signing keys, and note that it is not possible to export keys that are used for signatures.

doc/export_data.md Outdated
2. Signature retrieval / blinding randomness - 0x04
3. Attribute randomness - 0x05

If P1 = 0x00, then the if user accepts, all data (P1 \in [2-5]) can be exported for the same P2, identity and identity provider without prompting the user. Note that the instruction should be repeated with P1 = 0x00 to finish the session.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a good idea. It seems insecure that you can keep requesting this at any point in the future. I think I need you reiterate why we need this accepted first, ask for data later approach. What is the problem with the method just returning the required data when asked for everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reworked it.

@shjortConcordium shjortConcordium requested a review from orhoj May 3, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orhoj orhoj Awaiting requested review from orhoj

Assignees

@orhoj orhoj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shjortConcordium @orhoj