Repository navigation
[flaky-ci] Classify pre-TLS failures in trusted certificate test - #12714
Conversation
Keep Android system trust validation against google.com, but ignore only the approved DNS and routing failures that occur before SslStream creation. All TLS and certificate validation failures continue to fail the test.\n\nCo-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
simonrozsival
added
the
flaky-ci
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors, 0 warnings, 1 suggestion.
The implementation correctly limits ignores to selected connection failures raised before SslStream.AuthenticateAsClient, preserves TLS/certificate failures as test failures, and now disposes both network resources. I left one inline suggestion to pin the deliberately narrow socket-error whitelist with parameterized coverage.
CI is currently red: dotnet-android (Package Tests macOS > Tests > APKs 1) failed in build 1586953. The available check data does not expose the underlying test/task failure, so that lane still needs investigation or a successful rerun before merge.
Generated by Android PR Reviewer for #12714 · gpt56 · 92.4 AIC · ⌖ 8.88 AIC · ⊞ 25.7K
Comment /review to run again
Pin the four socket errors that may skip the external system-trust test and representative errors that must continue to fail. Co-authored-by: Copilot App <[email protected]>
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
|
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors, 1 warning, 0 suggestions.
The change correctly separates TCP connection setup from SslStream.AuthenticateAsClient() and adds focused policy coverage. However, the new socket-error whitelist still allows other failures raised before TLS begins—such as timeouts, resets, and refused connections—to fail a certificate-validation test. I left an inline comment recommending that the phase boundary, rather than selected socket codes, determine what is ignored.
CI build 1587827 is currently red: the Linux build and macOS build checks failed, while the Windows build remains in progress. The Azure timeline/log details could not be retrieved because this runner has no Azure DevOps authentication, so I could not determine whether those build failures are caused by this PR.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
azcliprod.blob.core.windows.net
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
Generated by Android PR Reviewer for #12714 · gpt56 · 76.8 AIC · ⌖ 8.95 AIC · ⊞ 25.7K
Comment /review to run again
Co-authored-by: Copilot App <[email protected]>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is scoped to test behavior, preserves TLS validation semantics, and adds regression coverage to prevent broadening the ignore allowlist unintentionally.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
tests/Mono.Android-Tests/Mono.Android-Tests/System.Net/SslTest.cs — 💡 Suggestion: google.com and 443 are duplicated across the connect path, TLS SNI/hostname, and… |
What changed in this PR
This PR reduces CI noise in System.NetTests.SslTest.VerifyTrustedCertificates by distinguishing pre-TLS external connectivity failures (DNS/routing) from actual TLS/certificate-validation failures, and only ignoring the former so the test remains a trust-store validation.
Changes:
- Split the test into an explicit TCP connect step and a separate TLS authentication step.
- Ignore a specific allowlist of
SocketErrorvalues that occur before TLS begins, emitting more diagnostic detail. - Add a parameterized regression test that pins which
SocketErrorvalues are ignored vs. still treated as failures.
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/System.Net/SslTest.cs | Refactors the trusted certificate test to ignore specific pre-TLS connectivity failures and adds allowlist regression coverage. |
Suppressed comments (1)
tests/Mono.Android-Tests/Mono.Android-Tests/System.Net/SslTest.cs:141
- 💡 Suggestion:
IsExternalConnectivityFailurecan be simplified with pattern matching (is ... or ...), which makes the allowlist easier to scan and reduces control-flow noise.
static bool IsExternalConnectivityFailure (SocketError socketError)
{
switch (socketError) {
case SocketError.HostNotFound:
case SocketError.NoData:
case SocketError.NetworkUnreachable:
case SocketError.HostUnreachable:
return true;
}
return false;
}
Android can expose ENETUNREACH and EHOSTUNREACH as raw native SocketErrorCode values. Classify those only during the pre-TLS connection while preserving all other connection and certificate failures. Co-authored-by: Copilot App <[email protected]>
Why
System.NetTests.SslTest.VerifyTrustedCertificatesvalidates Android default/system trust againstgoogle.com, but CI failures occurred before TLS certificate validation began:No route to hostin theNoAabflavor.hostname nor servname provided, or not known.EHOSTUNREACHvalue (113) instead of the normalizedSocketError.HostUnreachablevalue.These stacks ended while constructing
TcpClient, beforeSslStream.AuthenticateAsClient, so they do not indicate certificate expiry, rotation, device trust-store state, device time, or test ordering problems.Fixes #12704
What changed
The test still validates
google.comthrough Android default/system trust. The pre-TLS connection is ignored only for managedHostNotFound,NoData,NetworkUnreachable, andHostUnreachable, plus Android nativeENETUNREACHandEHOSTUNREACHvalues when they are surfaced without managed normalization. Endpoint and socket diagnostics are retained.TLS authentication remains outside that catch boundary, so timeouts, connection resets/refusals, authentication, hostname, expiry, chain, TLS alert, and all other failures continue to fail.
Parameterized regression coverage pins the four managed values and verifies that
TimedOutandConnectionResetremain failures. NativeSocketExceptioncoverage pinsENETUNREACHandEHOSTUNREACHas ignored andECONNRESETas non-ignored.Testing
On
emulator-5554:VerifyTrustedCertificatespassed.TestsFlavor=NoAab,AndroidPackageFormat=apkSSL category: 33 passed, 0 failed, 0 skipped; classifier coverage andVerifyTrustedCertificatespassed.