Skip to content

[ci-fix] Needs review: apply KMAC key-size requirements on SymCrypt-OpenSSL (refs #129939)#130594

Closed
github-actions[bot] wants to merge 2 commits into
mainfrom
ci-fix/kmac-symcrypt-minkey-11fd5e9cfe3e6ac9
Closed

[ci-fix] Needs review: apply KMAC key-size requirements on SymCrypt-OpenSSL (refs #129939)#130594
github-actions[bot] wants to merge 2 commits into
mainfrom
ci-fix/kmac-symcrypt-minkey-11fd5e9cfe3e6ac9

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Workflow artifact: ci-fix
Artifact kind: help
Linked KBE: #129939

Root cause

On CI legs where OpenSSL is backed by SymCrypt (PlatformDetection.IsSymCryptOpenSsl, e.g. AzureLinux 3), the KMAC known-answer tests fail with an invalid key length error rather than a codegen/PGO miscompile. In src/libraries/System.Security.Cryptography/tests/KmacTestDriver.cs:

public static KeySizes? PlatformKeySizeRequirements { get; } =
    PlatformDetection.IsOpenSslSupported && !PlatformDetection.IsSymCryptOpenSsl ? new KeySizes(4, 512, 1) : null;

public static byte[] MinimalKey { get; } =
    PlatformKeySizeRequirements?.MinSize is int min ? new byte[min] : Array.Empty<byte>();

The && !PlatformDetection.IsSymCryptOpenSsl exclusion assumes SymCrypt-OpenSSL imposes no minimum KMAC key length. It does enforce one, so PlatformKeySizeRequirements is null there, MinimalKey collapses to Array.Empty<byte>(), and every positive test that keys with MinimalKey is rejected for being too short.

This diagnosis was confirmed by @AndyAyersMS on the KBE: this is a real test bug on SymCrypt-OpenSSL boxes, not a PGO codegen problem.

Candidate change (needs review)

Drop the !IsSymCryptOpenSsl exclusion so SymCrypt-OpenSSL carries the same KeySizes(4, 512, 1) requirement, giving a non-empty MinimalKey:

public static KeySizes? PlatformKeySizeRequirements { get; } =
    PlatformDetection.IsOpenSslSupported ? new KeySizes(4, 512, 1) : null;

This is a test-only, non-muting change (no [ActiveIssue], skip, or exclusion).

What is unverified

  • Not build-validated / not run. A full System.Security.Cryptography test build plus execution on a SymCrypt-OpenSSL box exceeds this environment's budget, and SymCrypt-OpenSSL boxes are not available here.
  • Max key size (512) unconfirmed for SymCrypt-OpenSSL. This value now also drives the negative CryptographicException_Allocated_KeySize / CryptographicException_OneShot_KeySize tests (they assert a MaxSize + 1 key throws). If SymCrypt-OpenSSL does not reject keys above 512, those negative tests could fail and the max bound needs adjusting. The minimum (4) is the part directly responsible for the reported failure.

Help wanted

Could a Security-area owner confirm the SymCrypt-OpenSSL KMAC min/max key-length bounds and validate on an affected leg?

Suggested reviewers / area contacts

  • @bartonjs, @vcsjonesarea-System.Security
  • Area team: @dotnet/area-system-security
  • Thanks to @AndyAyersMS for the root-cause diagnosis on the KBE.

Note

This pull request was generated by an AI agent (GitHub Copilot) and should be reviewed by a human before merging.

Generated by CI Outer-Loop Failure Fixer · 341.1 AIC · ⊞ 18K ·

…129939)

SymCrypt-OpenSSL enforces a minimum KMAC key length, but the test driver
excluded it from PlatformKeySizeRequirements, leaving MinimalKey empty and
causing 'invalid key length' failures in the KMAC known-answer tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Threat detection produced a warning for this pull request output.

These changes need to be scrutinized before merge and only merged after a careful manual review.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security
See info in area-owners.md if you want to be subscribed.

@vcsjones

Copy link
Copy Markdown
Member

This is not the right fix and we need to understand what is going on before proceeding.

The issue is assigned to me as I intend to address this after investigating the AZL configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant