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

Feat/ldg 508 nano app implement getpublickey method #9

Merged
merged 20 commits into from
Dec 2, 2024

Conversation

keiff3r
Copy link

@keiff3r keiff3r commented Nov 29, 2024

No description provided.

- Add Ed25519 public key derivation functionality
- Update public key context structure to handle 32-byte keys
- Add UI flows for public key confirmation
- Add tests for get_public_key command
- Add initial support for signed public keys (WIP)

Note: Signed public key functionality is implemented but not working correctly yet.
Tests show mismatch between expected and actual signatures.

BREAKING CHANGE: Public key format changed from 65 to 32 bytes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

keiff3r and others added 15 commits November 29, 2024 21:38
- Update helper_send_response_pubkey to sign raw public key instead of hash
- Remove unused m_hash field from pubkey context structure
- Fix signature verification by using correct input data

This fixes the issue where signed public keys were not verifying correctly
due to signing the wrong input data.
Add a new screen to display the BIP32 derivation path when showing public keys
in the BAGL UI. This improves user verification by showing the full derivation
path before the public key.

- Add MAX_SERIALIZED_BIP32_PATH_LENGTH constant
- Update derivation_path_type to handle governance paths
- Add new UI step for BIP32 path display
- Format and show derivation path in pubkey display flow
- Replace simple address review with detailed NBGL review layout
- Add BIP32 path display alongside public key
- Update test snapshots for new UI layout

The change improves the user experience by showing both the BIP32 path and
public key in a clear, structured format using the NBGL review light interface.
* refactor(tests): update pubkey command tests to use new navigation helper
* feat(tests): add navigate_until_text_and_compare utility function
* feat(tests): add instructions_builder helper for screen navigation

The changes improve test maintainability by abstracting common navigation
patterns into reusable utilities that handle device-specific behavior
(Stax/Flex vs others) consistently.
* feat: add new function to handle signed public key response unpacking
* fix: correct return type annotation for get_public_key_response
* style: add consistent spacing between functions
* chore: enable formatOnSave in VSCode settings

Adds utility function to unpack signed public key responses and fixes
type hints for existing public key response unpacker. Improves code
formatting consistency across the test suite.
* chore: comment out boilerplate fuzzing test parser
* chore: comment out boilerplate transaction test files
* docs: add TODOs to update APDU and transaction documentation
* style: improve markdown table formatting

Temporarily disables outdated test files and marks documentation for
updates to match the current application implementation. Part of the
transition away from the legacy BOLOK chain example.
* chore: disable unused CMake configuration for unit tests
* chore: comment out test executables and library definitions
* chore: remove obsolete test linking configuration

Comments out boilerplate unit test configuration as part of the transition
away from CMake-based testing.
- Add constants for legacy path indices and governance subtree value
- Implement dynamic title generation for public key display based on:
  - Whether the key is signed
  - Whether it's a governance key
  - The governance key level (Root, Level 1, Level 2)
- Add validation for path types and return SW_INVALID_PATH on invalid paths
- Generate new golden snapshots

This change improves UX by providing clearer context about the type of
public key being displayed to users.
@keiff3r
Copy link
Author

keiff3r commented Nov 30, 2024

The code for this functionality works properly. The only feature missing is governance path support for the new derivation path, more specifically some data about:

  • which path index is the 'purpose' and
  • which is the 'governance subtree'


#include <stdbool.h> // bool
#include <string.h> // memset
// #ifdef HAVE_NBGL

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Comment on lines +22 to +23
// #include <stdbool.h> // bool
// #include <string.h> // memset

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Comment on lines +25 to +30
// #include "os.h"
// #include "glyphs.h"
// #include "nbgl_use_case.h"
// #include "io.h"
// #include "bip32.h"
// #include "format.h"

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Comment on lines +32 to +39
// #include "display.h"
// #include "constants.h"
// #include "../globals.h"
// #include "../sw.h"
// #include "../address.h"
// #include "action/validate.h"
// #include "../transaction/types.h"
// #include "../menu.h"

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
nbgl_useCaseReviewStatus(STATUS_TYPE_ADDRESS_REJECTED, ui_menu_main);
}
}
// static char g_address[43];

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Comment on lines +43 to +51
// static void review_choice(bool confirm) {
// // Answer, display a status page and go back to main
// validate_pubkey(confirm);
// if (confirm) {
// nbgl_useCaseReviewStatus(STATUS_TYPE_ADDRESS_VERIFIED, ui_menu_main);
// } else {
// nbgl_useCaseReviewStatus(STATUS_TYPE_ADDRESS_REJECTED, ui_menu_main);
// }
// }

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Comment on lines +53 to +57
// int ui_display_address() {
// if (G_context.req_type != CONFIRM_ADDRESS || G_context.state != STATE_NONE) {
// G_context.state = STATE_NONE;
// return io_send_sw(SW_BAD_STATE);
// }

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Comment on lines +59 to +63
// memset(g_address, 0, sizeof(g_address));
// uint8_t address[ADDRESS_LEN] = {0};
// if (!address_from_pubkey(G_context.pk_info.raw_public_key, address, sizeof(address))) {
// return io_send_sw(SW_DISPLAY_ADDRESS_FAIL);
// }

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Comment on lines +65 to +67
// if (format_hex(address, sizeof(address), g_address, sizeof(g_address)) == -1) {
// return io_send_sw(SW_DISPLAY_ADDRESS_FAIL);
// }

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
// return 0;
// }

// #endif

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
keiff3r and others added 3 commits December 2, 2024 11:42
* Remove governance title display logic for new derivation path
* Delete unused NEW_PATH constants
* Add tests for new derivation path public key retrieval
* Clean up path type checks in derivation_path_type function
* Generate associated golden snapshots
@keiff3r keiff3r requested a review from a team December 2, 2024 10:57
GuilaneDen
GuilaneDen previously approved these changes Dec 2, 2024
src/helper/util.c Outdated Show resolved Hide resolved
@keiff3r keiff3r marked this pull request as ready for review December 2, 2024 11:40
@keiff3r keiff3r merged commit a2594d8 into main Dec 2, 2024
42 of 43 checks passed
@keiff3r keiff3r deleted the feat/LDG-508--nano-app-implement-getpublickey-method branch December 4, 2024 10:58
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.

2 participants