duplicit FIPSKeysImporter tests, one for el10, the other for everything else.. by andrlos · Pull Request #39 · rh-openjdk/jtreg-buffer

andrlos · 2025-10-09T01:28:29Z

SSIA, was done due to the fact that DSA is being decomissioned from fips systems starting el10
tested on ojdk17 on both el8 & 10

…ng else..

judovana · 2025-10-09T07:59:51Z

test/reproducers/1991003/FIPSKeysImporter.java

 * @test
 * @summary Test for RH1991003 - FIPS keys importer
- * @requires (jdk.version.major >= 8)
+ * @requires os.version ~= ".*el10.*"


Whhere os version comes from?

Maybe split (add) better to https://github.com/rh-openjdk/jtreg-buffer/blob/550b97dd723545023993673648bbb3209cb87333/test/VarDeps.java so we can have os.name aka el, rhel, fedora... and os.versionNUmber ala 10.... And amyeb contribntue this to usptream?

In that moment there would be possibled to have os.versionNuimber >= 10 and we would be safe for next years.... @zzambers wdyt?

the os.version is a java property.. you can try the output (as did I) via System.getProperty("os.version"), I was not able to manage any kind of splitting inside the jtreg annotations :( the syntax is quite strict and I am glad that I was able to at least narrow like this
same with fips.. other than the ugly hack you guys are using with OTOOL fips variable, I was not able to detect the fips environment on its own inside the jtreg annotations.. unless we are able to do it, I don't think we can hope to propagate this to upstream

This https://github.com/rh-openjdk/jtreg-buffer/blob/main/test/VarDeps.java is what you were supposed to enhance. And larter backportt it o upstream...

hmmm okay, take a look now @judovana

judovana · 2025-10-09T08:01:00Z

test/reproducers/1991003/FIPSKeysImporterDSA.java

+/*
+ * @test
+ * @summary Test for RH1991003 - FIPS keys importer
+ * @requires !(os.version ~= ".*el10.*")


Otherwise nice solution.. bad luck it may bite any time soon.... Eg dsa disapearing en el9, or need of el11 ....

well it should not dissapear on already existing rhels.. we shall see in the future, but my guess is that DSA will be decomissioned since el10 fips and el11 non-fips.. but older rhels will support it just fine

judovana · 2025-10-09T08:02:03Z

test/reproducers/1991003/FIPSKeysImporter.java

-        //doTestSignature("SHA1withDSA", rootCA0.getPrivateKey(JKS_ORIGIN),
-        //        rootCA0.getPublicKey(JKS_ORIGIN));
-
        TestCA rootCA1 = cas.get("root_ca_1");


I will leave on @zzambers how correct the algorithm listing is correct/enough. If you discussed with him, klet me know. TY!

I think change is correct (removing DSA on rhel10, keeping others). I don't think there is anything more to be done (at least at this point).

judovana · 2025-10-09T10:24:55Z

Btw the fips condition seems to bne missing...?

andrlos · 2025-10-13T07:28:33Z

I need to redo the VarDeps to be jdk8 compatible

judovana · 2025-10-13T07:55:42Z

test/reproducers/1991003/FIPSKeysImporter.java

 * @test
 * @summary Test for RH1991003 - FIPS keys importer
- * @requires os.version ~= ".*el10.*"
+ * @requires var.os.version.major >= 10 & var.sys.fips == "true"


Great!

Feel free to correct me, but the "el" check shouldstay, right? So you may match & os.version ~= ".*el.*" or add some var.os.version.name ?

sorry I thought that rhel/fedora were the only OSes to feature fips.. okay.. I shall reinstate it.. I think that var.os.version.name would be unnecessary since I can just use this regex..

judovana · 2025-10-13T08:19:00Z

test/reproducers/1991003/FIPSKeysImporterDSA.java

+ * @run main/othervm/timeout=30 FIPSKeysImporterDSA
+ */
+
+public final class FIPSKeysImporterDSA {


One more thing - there is quite a lot of code shared between FipsKeysImporter*.java files. Any desire to share the code?

Well honestly it was not my desire to do that, but fine :D

Nn. I do not insits at all. It brings another ttroubles. Feel free to extract only some, aor none at all

It depends on your judgement of possible complexity removal/aditional and pros and cons of the selected shared logic.

Jdk8 adjustion have priority....

trying to mitigate duplicated code in FIPSKeysImporter classes

andrlos · 2025-10-13T09:31:55Z

I just found out that this is not ready yet as is, in case it hits the el10 & fips combination it still triggers the DSA variant for some reason..

EDIT: nevermind.. I accidentaly force changed the msys variable on the line above instead of the fips one.. all good to merge

judovana · 2025-10-13T09:39:40Z

test/reproducers/1991003/FIPSKeysImporterDSA.java

+import javax.net.ssl.SSLParameters;
+import javax.net.ssl.SSLSession;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.SSLEngineResult.HandshakeStatus;


I think nearly all imports are in FipsKesIMporter.java

yep... all of them actually.. NOW it should be OK :D sorry

judovana · 2025-10-13T11:28:29Z

I wanted to squash and merge, but then I noticed that non of the two tests is visible in GH - eg: https://github.com/rh-openjdk/jtreg-buffer/actions/runs/18463464820/job/52599940744?pr=39

IS it expected?

zzambers · 2025-10-13T11:54:47Z

Looks ok to me. I think another alternative would be to pass different arg to test class on different systems (just having separate @test parts), but this also works.

zzambers · 2025-10-13T11:58:05Z

I can only comment. Seems I don't have rights in this repo to approve. :)

judovana · 2025-10-13T12:04:38Z

I can only comment. Seems I don't have rights in this repo to approve. :)

Thats what i thought Jirik will do, but he did this.. so I was not nitpicking more...

zzambers · 2025-10-13T12:08:08Z

test/VarDeps.java

+            // Could not read flag — assume non-FIPS
+        } finally {
+            if (br != null) {
+                try { br.close(); } catch (IOException ignored) {}


Btw. you could make use of try-with-resources here (jdk7+, I think), to make it a bit more elegant / less error prone.

@jandrlik pls do.

oh yeah.. I vaguely remember this from school :D sorry

andrlos · 2025-10-13T13:04:29Z

I wanted to squash and merge, but then I noticed that non of the two tests is visible in GH - eg: https://github.com/rh-openjdk/jtreg-buffer/actions/runs/18463464820/job/52599940744?pr=39

IS it expected?

with the ".el." filter it only runs on el systems -> not in GH actions

judovana · 2025-10-13T13:15:53Z

That do not seem right. * @requires (var.os.version.major < 10 | var.sys.fips == "false") & (os.version ~= ".*el.*") seems tobe wrong then. It should run on anything except el10 in fips, shouldnt it?

andrlos · 2025-10-13T13:26:08Z

yeah, it definitely should.. fix would be something like:
* @requires var.sys.fips == "true" & ( ! (os.version ~= ".*el.*") | var.os.version.major >= 10 )

translated to common speech would be "we require for it to be FIPS AND (for it to NOT be RHEL OR be at least of os.version 10)"

- redone the logic statements in require to allow non-el OSes

andrlos · 2025-10-13T14:03:45Z

@judovana OK, now!!

judovana

Awesome!

duplicit FIPSKeysImporter tests, one for el10, the other for everythi…

550b97d

…ng else..

judovana reviewed Oct 9, 2025

View reviewed changes

enhancing VarDeps with fips detection and osMajorVersionDetection

b2f8d1e

judovana reviewed Oct 13, 2025

View reviewed changes

making VarDeps changes ojdk8 compliant

ccc0a85

trying to mitigate duplicated code in FIPSKeysImporter classes

judovana reviewed Oct 13, 2025

View reviewed changes

cleaning imports

4b5c4aa

zzambers reviewed Oct 13, 2025

View reviewed changes

- try-with-resources for VarDeps filereading codeblocks

bb30ffb

- redone the logic statements in require to allow non-el OSes

judovana approved these changes Oct 13, 2025

View reviewed changes

judovana merged commit b747d1f into rh-openjdk:main Oct 13, 2025
16 checks passed

Conversation

andrlos commented Oct 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

judovana commented Oct 9, 2025

Uh oh!

andrlos commented Oct 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrlos commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

judovana commented Oct 13, 2025

Uh oh!

zzambers commented Oct 13, 2025

Uh oh!

zzambers commented Oct 13, 2025

Uh oh!

judovana commented Oct 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrlos commented Oct 13, 2025

Uh oh!

judovana commented Oct 13, 2025

Uh oh!

andrlos commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrlos commented Oct 13, 2025

Uh oh!

judovana left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andrlos commented Oct 13, 2025 •

edited

Loading

andrlos commented Oct 13, 2025 •

edited

Loading