Skip to content
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

[BUG] org.opensearch.flowframework.util.EncryptorUtilsTests is flaky #233

Closed
amitgalitz opened this issue Nov 30, 2023 · 3 comments
Closed
Labels
bug Something isn't working untriaged

Comments

@amitgalitz
Copy link
Member

What is the bug?

There are 3 flaky tests all with the same exception:

 - org.opensearch.flowframework.util.EncryptorUtilsTests.testEncryptWithDifferentMasterKey
 - org.opensearch.flowframework.util.EncryptorUtilsTests.testEncryptDecryptTemplateCredential
 - org.opensearch.flowframework.util.EncryptorUtilsTests.testEncryptDecrypt

run: https://github.com/opensearch-project/flow-framework/actions/runs/7052745868/job/19198492841

All the tests have this exception:

  2> java.lang.IllegalArgumentException: Right now only AES/GCM/NoPadding is supported
        at __randomizedtesting.SeedInfo.seed([9F46C5A959EB03EE:DAA6D892FDC9D7C8]:0)
        at com.amazonaws.encryptionsdk.jce.JceMasterKey.getInstance(JceMasterKey.java:66)
        at org.opensearch.flowframework.util.EncryptorUtils.encrypt(EncryptorUtils.java:185)
        at org.opensearch.flowframework.util.EncryptorUtilsTests.testEncryptWithDifferentMasterKey(EncryptorUtilsTests.java:114)
    

I haven't been able to reproduce this locally yet though

What is the expected behavior?

test always passes

What is your host/environment?

github runner above

Do you have any additional context?

The error is that only AES/GCM/NoPadding wrapping algorithm is supported, however this is the one that we are using.

@amitgalitz amitgalitz added bug Something isn't working untriaged labels Nov 30, 2023
@dbwiddis
Copy link
Member

dbwiddis commented Dec 1, 2023

@dbwiddis
Copy link
Member

dbwiddis commented Dec 1, 2023

This is a strange error. The exception is thrown in this code block:

public static JceMasterKey getInstance(
    final SecretKey key,
    final String provider,
    final String keyId,
    final String wrappingAlgorithm) {
  switch (wrappingAlgorithm.toUpperCase()) {
    case "AES/GCM/NOPADDING":
      return new JceMasterKey(provider, keyId, JceKeyCipher.aesGcm(key));
    default:
      throw new IllegalArgumentException("Right now only AES/GCM/NoPadding is supported");
  }
}

But the line of code throwing the exception has a constant for that argument:

JceMasterKey.getInstance(new SecretKeySpec(bytes, ALGORITHM), PROVIDER, "", WRAPPING_ALGORITHM);

where

WRAPPING_ALGORITHM = "AES/GCM/NoPadding";

I believe it's an issue in com.amazonaws.encryptionsdk.jce.JceMasterKey; where they are doing an uppercase string conversion without locale information, which would make sense for CI under a different locale.

It's 100% reproducible with

./gradlew ':test' --tests "org.opensearch.flowframework.util.EncryptorUtilsTests.testEncryptDecryptTemplateCredential" -Dtests.seed=640FAD486BA2AFF -Dtests.security.manager=false -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=tr-TR -Dtests.timezone=Etc/UCT -Druntime.java=17

Adding on some instrumentation to test locale isolates the problem:

String t1 = WRAPPING_ALGORITHM.toUpperCase();
String t2 = WRAPPING_ALGORITHM.toUpperCase(Locale.ROOT);
System.err.println("Uppercase wrapping algorithm: >" + t1 + "<");
System.err.println("Uppercase wrapping algorithm with root locale: >" + t2 + "<");
System.err.println("Equality: " + "AES/GCM/NOPADDING".equals(t1) + ", " + "AES/GCM/NOPADDING".equals(t2));

Output:

  2> Uppercase wrapping algorithm: >AES/GCM/NOPADDİNG<
  2> Uppercase wrapping algorithm with root locale: >AES/GCM/NOPADDING<
  2> Equality: false, true

It's hard to see but the "i" in padding is not the correct character, unicode C4 B0: https://unicode.scarfboy.com/?s=U%2B0130
t1 in hex: 41 45 53 2F 47 43 4D 2F 4E 4F 50 41 44 44 C4 B0 4E 47
t2 in hex: 41 45 53 2F 47 43 4D 2F 4E 4F 50 41 44 44 49 4E 47

So two things we can do:

  1. Work around the bug in our framework by uppercasing the string to start with.
  2. File a bug upstream with the lack of localization

@dbwiddis
Copy link
Member

dbwiddis commented Dec 2, 2023

Fixed by #239

@dbwiddis dbwiddis closed this as completed Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working untriaged
Projects
None yet
Development

No branches or pull requests

2 participants