Skip to content

Conversation

@zll600
Copy link

@zll600 zll600 commented Nov 26, 2025

Target branch: 5.3.x
Resolves issue #

  • It is a Bug fix
  • It is a New feature
  • Breaks BC
  • Includes Deprecations

Overview

I submit this PR aims to make two changes for the CheckMetadataStatement

Simplify the Implementation

Current CheckMetadataStatement works really well and is excellent. But I think it's a little bit complex to understand the source code.

For example, we are checking the trust path of the none and self type attestation.

if ($publicKeyCredentialOptions->attestation === null || $publicKeyCredentialOptions->attestation === PublicKeyCredentialCreationOptions::ATTESTATION_CONVEYANCE_PREFERENCE_NONE) {
$this->logger->debug('No attestation is asked.');
if ($aaguid === '00000000-0000-0000-0000-000000000000' && in_array(
$attestationStatement->type,
[AttestationStatement::TYPE_NONE, AttestationStatement::TYPE_SELF],
true
)) {
$this->logger->debug('The Attestation Statement is anonymous.');
$this->checkCertificateChain($attestationStatement, null);
return;
}
return;
}

There are some points I think are a little bit complex to understand why we call the checkCertificateChain here

  1. No metadata statement provided to verify the trust path.
  2. Both None and Self type attestations do not include a valid trust path.

Add background info to explain the rationale

Current implementation handles aaguid 00000000-0000-0000-0000-000000000000 with special ways. But no explanation for it. I add more background to explain why we implement like this.

Note

I think the current implementation is really good. I opened this PR because I have some questions when reading the source code. Feel free to close this PR.

@zll600 zll600 force-pushed the feature/clean-up-metadata-statement-check branch from cca1b23 to 9aa1a6a Compare November 26, 2025 01:54
@zll600 zll600 marked this pull request as ready for review November 26, 2025 01:54
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.

1 participant