Skip to content

Allow SCP when only short APDUs are supported by smart card connection #177

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

Merged
merged 5 commits into from
May 27, 2025

Conversation

AdamVe
Copy link
Member

@AdamVe AdamVe commented May 22, 2025

This pull request introduces support for handling both short and extended APDUs (Application Protocol Data Units) in the smart card protocol, along with associated updates to the testing framework. Key changes include modifications to the ScpProcessor and SmartCardProtocol classes to enable this functionality, as well as new tests to validate APDU size handling.

Core functionality updates:

  • ScpProcessor: Added a usingShortApdus flag and an ExtendedApduProcessor to handle short APDUs when extended APDUs are not supported. Adjusted the sendApdu method to conditionally use the appropriate processor based on APDU size. [1] [2]
  • SmartCardProtocol: Updated initScp to support a new shortApdus parameter, enabling the selection of short or extended APDUs dynamically. Adjusted logic to determine maximum APDU size based on device capabilities and user input. [1] [2] [3] [4]

Testing enhancements:

  • Added a new testApduSizes method in Scp11DeviceTests to validate handling of different APDU sizes and formats (short and extended). This includes generating payloads of varying sizes and verifying round-trip communication with the smart card.
  • Integrated the testApduSizes method into both Android and desktop test suites (Scp11Tests). [1] [2]

Utility and callback additions:

  • Introduced a DeviceCallback interface in TestState and a withDevice method in SecurityDomainTestState to facilitate device-specific test operations. [1] [2]

Minor updates:

  • Updated copyright headers across multiple files to include the year 2025. [1] [2] [3] [4] [5]

These changes enhance the flexibility of the smart card protocol implementation while ensuring robust testing for various APDU handling scenarios.

@AdamVe AdamVe requested a review from Copilot May 22, 2025 13:10
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds optional short APDU support alongside existing extended APDU handling in the smart card protocol and updates tests to cover varying APDU sizes.

  • Introduces a shortApdus flag throughout SmartCardProtocol and ScpProcessor to allow forcing short APDU mode.
  • Adds withDevice helper in test state and a new testApduSizes test to validate both short and extended APDU behavior.
  • Updates integration tests on desktop and Android to include the APDU size coverage.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
SecurityDomainTestState.java Added withDevice(DeviceCallback) helper for device‐level operations.
Scp11DeviceTests.java New testApduSizes method looping over payload sizes and formats.
TestState.java Introduced DeviceCallback interface for generic device callbacks.
testing-desktop/.../Scp11Tests.java Hooked up testApduSizes in desktop integration suite.
testing-android/.../Scp11Tests.java Hooked up testApduSizes in Android integration suite.
SmartCardProtocol.java Added shortApdus overload and conditional extended APDU support.
ScpProcessor.java Split APDU formatting logic into short vs. extended processor paths.
Comments suppressed due to low confidence (3)

core/src/main/java/com/yubico/yubikit/core/smartcard/ScpProcessor.java:63

  • [nitpick] This nested ternary for choosing between extendedProcessor and processor is hard to read. Splitting into a clear if/else block or extracting into a well-named helper method would improve readability and reduce cognitive load.
usingShortApdus && data.length + 8 > SHORT_APDU_MAX_CHUNK ? extendedProcessor.formatApdu(...) : processor.formatApdu(...);

testing/src/main/java/com/yubico/yubikit/testing/TestState.java:104

  • [nitpick] The name DeviceCallback is very generic. Renaming it to something more specific like YubiKeyDeviceCallback or SecurityDomainDeviceCallback could make its intent clearer.
public interface DeviceCallback {

testing/src/main/java/com/yubico/yubikit/testing/sd/Scp11DeviceTests.java:106

  • The RandomUtils class is used here but no import is shown; verify that the correct RandomUtils is imported (e.g. org.apache.commons.lang3.RandomUtils or your utility package) to avoid compile errors.
byte[] payload = RandomUtils.getRandomBytes(payloadSize);

@AdamVe AdamVe merged commit 70719ec into main May 27, 2025
16 checks passed
@AdamVe AdamVe deleted the adamve/short_apdu_scp_support branch May 27, 2025 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants