Windows builds in CI failing because of cipher ordering

[yschimke]

OK to relax in the test? Specifically it looks like a difference in ordering of the TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 and TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 ciphers relative to AES GCM (TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384)

https://github.com/google/conscrypt/runs/1526147431

    java.lang.AssertionError: Unexpected element(s). Expected <[TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256, TLS_ECDHE_PSK_WITH_CHACHA20_POLY1305_SHA256, TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA, TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA, TLS_PSK_WITH_AES_128_CBC_SHA, TLS_PSK_WITH_AES_256_CBC_SHA, TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, TLS_RSA_WITH_AES_128_GCM_SHA256, TLS_RSA_WITH_AES_256_GCM_SHA384, TLS_RSA_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_256_CBC_SHA, TLS_EMPTY_RENEGOTIATION_INFO_SCSV]>, actual <[TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256, TLS_ECDHE_PSK_WITH_CHACHA20_POLY1305_SHA256, TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA, TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA, TLS_PSK_WITH_AES_128_CBC_SHA, TLS_PSK_WITH_AES_256_CBC_SHA, TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, TLS_RSA_WITH_AES_128_GCM_SHA256, TLS_RSA_WITH_AES_256_GCM_SHA384, TLS_RSA_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_256_CBC_SHA, TLS_EMPTY_RENEGOTIATION_INFO_SCSV]>

[yschimke]

This appears to be a difference in expected ordering due to AES support. Is CpuFeatures detection missing on Windows?

public static final List<String> CIPHER_SUITES_DEFAULT = CpuFeatures.isAESHardwareAccelerated()
        ? CIPHER_SUITES_AES_HARDWARE
        : CIPHER_SUITES_SOFTWARE;

[prbprbprb]

Yeah, I had a quick look at that last week and it's weird that it only affects this test but I didn't get time to dig further.

Generally these sort of ordering changes occur when there are mismatched expectations around hardware acceleration, but that should affect the preceding test too (test_SSLContext_pskOnlyConfiguration_defaultProviderOnly()). On the other hand, it's their provider so I guess it's not up to us to enforce any re-ordering they do of cyphersuites.

On the minus side, the assumption that the provider won't re-order things is buried in assertEnabledCipherSuites(). Rather than build two variants of that, it might be worth skipping this check on Windows for now.

[yschimke]

Arguably we could just assume true on Windows in this day and age https://en.wikipedia.org/wiki/AES_instruction_set

No good answer for Windows without extra native code https://security.stackexchange.com/questions/4590/tool-or-process-to-check-for-aes-ni-support-on-processor

[yschimke]

It won't affect the PSK tests as those are not conditional

    // NOTE: This list needs to be kept in sync with Javadoc of javax.net.ssl.SSLSocket and
    // javax.net.ssl.SSLEngine.
    public static final List<String> CIPHER_SUITES_DEFAULT_PSK = Arrays.asList(
            "TLS_ECDHE_PSK_WITH_CHACHA20_POLY1305_SHA256",
            "TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA",
            "TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA",
            "TLS_PSK_WITH_AES_128_CBC_SHA",
            "TLS_PSK_WITH_AES_256_CBC_SHA");

[prbprbprb]

I'm clearly missing something here :) Both test_SSLContext_pskOnlyConfiguration_defaultProviderOnly and test_SSLContext_x509AndPskConfiguration_defaultProviderOnly have CIPHER_SUITES_TLS13 in their expected results and that's the one that gets ordered differently depending on AES support.

TBF if the issue is indeed mis-detected acceleration then I should really make the equivalent change to https://r.android.com/c/platform/libcore/+/1224843 in Conscrypt which kind of fell off my radar. I.e. in tests defer to NativeCrypto.EVP_has_aes_hardware() when determining the expected order of suites.

[yschimke]

No was me, I thought this ordering came from Conscrypt and missed that it was the underlying JVM. I'm also guessing you could pretty much drop the conditional from the tests if you know all test environments are running with AES instruction sets. Seems fairly likely since (I'm guessing) this has been common place since 2011 on Desktop and Server CPUs?

[prbprbprb]

32bit Cuttlefish was (is?) non-AES-accelerated, which was a thorn for a while but I thought we caught all of the affected places. But I think we may be able to drop support for that now and stick to 64bit.

[yschimke]

For now, I think it does unblock having working PR builds to use for artifacts. I hope https://github.com/google/conscrypt/runs/1622559125 is an unrelated failure.

[prbprbprb]

I hope https://github.com/google/conscrypt/runs/1622559125 is an unrelated failure.

That looks like an infrastructure flake to me