gh-136728: Refactor build.yml CI config and multissltests.py #136729

WillChilds-Klein · 2025-07-16T20:05:29Z

Notes

Please see #136728. Changes to build.yml are directly adapted from @hugovk's proof-of-concept here.

Testing

this PR's CI
PR CI on my fork.

Issue: Refactor build.yml CI ssltests config and multissltests.py #136728

@hugovk

Suggested by @hugovk [1]. [1]: hugovk@a3f2ba9

This reverts commit 1fcb49f.

picnixz

If possible, I want multissltests to be refactored separately.

picnixz · 2025-08-02T10:37:24Z

.github/workflows/build.yml

+          - { os: ubuntu-24.04, ssl: openssl, ssl_ver: 3.0.16 }
+          - { os: ubuntu-24.04, ssl: openssl, ssl_ver: 3.1.8 }
+          - { os: ubuntu-24.04, ssl: openssl, ssl_ver: 3.2.4 }
+          - { os: ubuntu-24.04, ssl: openssl, ssl_ver: 3.3.3 }
+          - { os: ubuntu-24.04, ssl: openssl, ssl_ver: 3.4.1 }
+          - { os: ubuntu-24.04, ssl: awslc,  ssl_ver: 1.55.0 }


You can remove the os from the include and have:

matrix: os: [ubuntu-24.04] include: - { ssl: openssl, ssl_ver: 3.0.16 } - ...

It would be possible to merge two matrices but we'll need a pre-step and I don't think it's more maintainable.

Could we do ssl_lib: openssl | awslc? It would be easier to follow later on than just matrix.ssl everywhere.

I'm sorry but what would ssl_lib: openssl | awslc in this case? does it run on both and if it doesn't work for one, it picks the other? (also, how would we use it below?)

@picnixz I'm not sure I follow, the suggestion is just to rename matrix.ssl to matrix.ssl_lib. openssl | awslc are the valid values for either name.

Oh! I thought you suggested something like this:

matrix: os: [ubuntu-24.04] include: - { ssl_lib: openssl | awslc, ssl_ver: 3.0.16 }

and I didn't know what it would means. Yes, we can have matrix.ssl_lib

picnixz · 2025-08-02T10:39:45Z

.github/workflows/build.yml

-        echo "LD_LIBRARY_PATH=${GITHUB_WORKSPACE}/multissl/aws-lc/${AWSLC_VER}/lib" >> "$GITHUB_ENV"
-    - name: 'Restore AWS-LC build'
-      id: cache-aws-lc
+        echo "SSL_DIR=${GITHUB_WORKSPACE}/multissl/${{ matrix.ssl }}/${SSL_VER}" >> "$GITHUB_ENV"


Can't we just use ${{ env.ssldir }}? I don't understand why we're defining it here and in the env section

zizmor says using ${{ env.ssldir }} can result in template injection:

help[template-injection]: code injection via template expansion --> ./.github/workflows/build.yml:324:134 | 323 | run: | | --- help: this run block 324 | CMD=(./configure CFLAGS="-fdiagnostics-format=json" --config-cache --enable-slower-safety --with-pydebug --with-openssl="${{ env.ssldir }... | ---------- help: may expand into attacker-controllable code

Isn't env a procteced namespace that cannot be accessed from outside? but if zizmor flags it as possibly vulnerable, let's be verbose.

Let's ask zizmor author @woodruffw.

Ok, so zizmor kwnows that it's not attackable but recommends against it for shell expansion purposes: https://github.com/woodruffw/gha-hazmat/blob/e4094ceabb0068a2a5b81ecc8e1ee919ba52a050/.github/workflows/template-injection.yml#L77.

However! we can do another trick https://github.com/woodruffw/gha-hazmat/blob/e4094ceabb0068a2a5b81ecc8e1ee919ba52a050/.github/workflows/template-injection.yml#L67-L72. I guess we could directly use the environment variable name then without needing to make GH variable expansion.

Yeah, I'd recommend using the environment variable directly instead of intermediating it through a template (e.g. $FOO instead of ${{ env.FOO }}). That should be equivalent in terms of intended behavior without the injection risk 🙂

The reason zizmor flags these is because they can be exploitable in some subtle situations: most of the time env.* is static and not attacker-controllable, but sometimes it comes from GITHUB_ENV or another dynamic source that zizmor can't follow.

Some more docs here too (the first tip in that section mentions why env.* is not preferred versus direct expansion): https://docs.zizmor.sh/audits/#template-injection

picnixz · 2025-08-02T10:40:46Z

.github/workflows/build.yml

-          --base-directory "$MULTISSL_DIR" \
-          --awslc ${{ matrix.awslc_ver }} \
-          --system Linux
+        python3 Tools/ssl/multissltests.py --steps=library --base-directory "$MULTISSL_DIR" --system Linux --ssl ${{ matrix.ssl }} --ssl-versions ${{ matrix.ssl_ver }}


Can you make it multiline as before? please

Note we can use run: > syntax to avoid the trailing backslashes

Yes, it would be better. We don't care about literal new lines here.

picnixz · 2025-08-02T10:44:39Z

.github/workflows/build.yml

+        if [ "${{ matrix.ssl }}" = "openssl" ]; then
+          "${CMD[@]}"
+        else
+          "${CMD[@]}" --with-builtin-hashlib-hashes=blake2 --with-ssl-default-suites=openssl


Can we populate a variable called "CONFIGURE_FLAGS" instead and use a switch/case so that we exactly match the SSL libname. It will become easier in the future if we add BoringSSL or LibreSSL tests.

At this point I'd almost suggest using a small Python script to calculate flags. We should avoid complex shell logic in workflow files as much as possible, I don't want to read a bash switch-case in YAML! An if-statement is simpler to understand here for me, though the ${CMD[@]} syntax is arcane.

Oh yeah, a small Python script is also fine; here what matters to me is to be able to easily add a new library without having 3km long of options. As for ${CMD[@]}, it expands the array's content, so you end up with ./configure [...]. Usually, it's more common to expand the options and hardcode the command rather than expand the entire array containing the command as well.

.github/workflows/build.yml

picnixz · 2025-08-02T10:48:59Z

.github/workflows/build.yml

@@ -567,7 +507,7 @@ jobs:
        key: ${{ matrix.os }}-multissl-openssl-${{ env.OPENSSL_VER }}
    - name: Install OpenSSL
      if: steps.cache-openssl.outputs.cache-hit != 'true'


With steps.cache-openssl here, we won't be hitting the cache-ssl entry right? maybe it's better to store a cache hit with steps.cache-${{ matrix.ssl }} if it's possible?

picnixz · 2025-08-02T10:50:05Z

Tools/ssl/multissltests.py

+    @property
+    @abstractmethod
+    def library(self=None):
+        pass


Please revert this. As we can't have abstract class methods, I prefer having real class variables where we would raise if they are not set.

Yep, better to raise NotImplementedError. That's treated equivalently to an abstract method.

I think it's just better to have abstract class variables here. The reason is that I don't think we would ever need a non-static computation for most of those abstract properties. If they must be abstract class properties that are represented by dynamic values that need a bit more work to compute (but only needed to be computed once), just a @classmethod together with NotImplementedError.

picnixz · 2025-08-02T10:53:38Z

Tools/ssl/multissltests.py

+    )
+
+    versions = []
+    ssl_libs = AbstractBuilder.__subclasses__()


Please, don't use this. It's harder to maintain if we do it like this. Just hardcode the classes.

picnixz · 2025-08-02T10:53:59Z

Tools/ssl/multissltests.py

+        assert len(libs) == 1
+        cls = libs.pop()
+        if args.ssl_versions:
+            versions += [(cls, v) for v in args.ssl_versions]


Use extend instead of += [...]

picnixz · 2025-08-02T10:54:25Z

Tools/ssl/multissltests.py

+        format="*** %(levelname)s %(message)s"
+    )
+
+    versions = []


You could have a dict of classes to a list of versions instead of storing a tuple of (class, version)

hugovk · 2025-07-21T17:01:49Z

Tools/ssl/multissltests.py

-        "AWS-LC versions, defaults to '{}' if no crypto library versions are given."
-    ).format(AWSLC_RECENT_VERSIONS)
+    default=None,
+    help="SSL lib version(s), default depends on value passed to --ssl",


Suggested change

help="SSL lib version(s), default depends on value passed to --ssl",

help="SSL lib versions, default depends on libs passed to --ssl",

hugovk · 2025-07-21T17:03:51Z

Tools/ssl/multissltests.py

@@ -78,6 +58,7 @@
        "versions."
    ),
 )
+parser.color = True


This is now the default, so we can skip this:

Suggested change

parser.color = True

(Also, it wouldn't be effective if we were to use an older Python version)

hugovk · 2025-08-05T09:31:44Z

.github/workflows/build.yml

-        echo "LD_LIBRARY_PATH=${GITHUB_WORKSPACE}/multissl/aws-lc/${AWSLC_VER}/lib" >> "$GITHUB_ENV"
-    - name: 'Restore AWS-LC build'
-      id: cache-aws-lc
+        echo "SSL_DIR=${GITHUB_WORKSPACE}/multissl/${{ matrix.ssl }}/${SSL_VER}" >> "$GITHUB_ENV"


zizmor says using ${{ env.ssldir }} can result in template injection:

help[template-injection]: code injection via template expansion --> ./.github/workflows/build.yml:324:134 | 323 | run: | | --- help: this run block 324 | CMD=(./configure CFLAGS="-fdiagnostics-format=json" --config-cache --enable-slower-safety --with-pydebug --with-openssl="${{ env.ssldir }... | ---------- help: may expand into attacker-controllable code

.github/workflows/build.yml

AA-Turner · 2025-08-05T13:12:51Z

.github/workflows/build.yml

-  build-ubuntu-ssltests-openssl:
-    name: 'Ubuntu SSL tests with OpenSSL'
+  build-ubuntu-ssltests:
+    name: 'Ubuntu SSL tests'
    runs-on: ${{ matrix.os }}


All jobs run on the same OS, this is simpler:

Suggested change

runs-on: ${{ matrix.os }}

runs-on: ubuntu-24.04

We use matrix.os in the cache key so better not hardcode it here?

AA-Turner · 2025-08-05T13:21:23Z

Tools/ssl/multissltests.py

+Please keep this script compatible with all currently-maintained Python
+versions.


Do we care about Python 3.9 at this point? Frequently for in-tree tools we use python_for_regen as the base version, or the previous stable release (e.g. 3.12 at present).

Yes, we care because of security branches I'd say. Also, there are still distros with Pyhton 3.9 as its default I think (I mean, my default Python in OpenSUSE was Python 3.6!). But generally, it's to limit to "simple" Python when possible.

True, but e.g. for backports to 3.9 we would run against the version of Tools/ssl/multissltests.py in the 3.9 branch? We should add a comment explaining why we care about all non EOL versions, though.

True, but e.g. for backports to 3.9 we would run against the version of Tools/ssl/multissltests.py in the 3.9 branch?

That's... a good question. I would have said "yes", but now I'm not entirely sure. Maybe not?

Ah actually, we do:

def run_python_tests(self, tests, network=True): if not tests: cmd = [ sys.executable, os.path.join(PYTHONROOT, 'Lib/test/ssltests.py'), '-j0' ] elif sys.version_info < (3, 3): cmd = [sys.executable, '-m', 'test.regrtest'] else: cmd = [sys.executable, '-m', 'test', '-j0'] if network: cmd.extend(['-u', 'network', '-u', 'urlfetch']) cmd.extend(['-w', '-r']) cmd.extend(tests) self._subprocess_call(cmd, stdout=None)

So for executing the tests, we're really re-using the python interpreter that was used to generate the scripts, so we need to run the tests against the correct interpreter version.

@WillChilds-Klein could you update run_python_tests() in this PR to remove the 3.3 block & use long options to regrtest? -w -> --rerun & -r -> --randomize

AA-Turner · 2025-08-05T13:21:35Z

Tools/ssl/multissltests.py

+from abc import abstractmethod
+from abc import ABCMeta


Suggested change

from abc import abstractmethod

from abc import ABCMeta

from abc import ABCMeta, abstractmethod

AA-Turner · 2025-08-05T13:22:06Z

Tools/ssl/multissltests.py

-        "OpenSSL and LibreSSL versions are given."
-    ).format(LIBRESSL_RECENT_VERSIONS, LIBRESSL_OLD_VERSIONS)
+    '--ssl',
+    choices=['openssl', 'awslc', 'libressl'],


Suggested change

choices=['openssl', 'awslc', 'libressl'],

choices=('openssl', 'awslc', 'libressl'),

AA-Turner · 2025-08-05T13:22:46Z

Tools/ssl/multissltests.py

-        "LibreSSL versions, defaults to '{}' (ancient: '{}') if no "
-        "OpenSSL and LibreSSL versions are given."
-    ).format(LIBRESSL_RECENT_VERSIONS, LIBRESSL_OLD_VERSIONS)
+    '--ssl',


Again I'd suggest --ssl-library or etc

WillChilds-Klein added 8 commits July 14, 2025 21:19

Adapt @hugovk's proof-of-concept CI definition refactor

a104f8a

Suggested by @hugovk [1]. [1]: hugovk@a3f2ba9

Preliminary refactor of multissltests.py, TODO migrate to classes

3fcbe0d

Fix aws-lc/awslc lib name discrepancy

5d8ec9a

Migrate AbstractBuilder to abc

991c6b2

Fix imports

7b51499

Complete ABC refactor

6638127

Colorize parser

4e0a8ca

Adjust compatibility comment

1a90e0c

bedevere-app bot mentioned this pull request Jul 16, 2025

Refactor build.yml CI ssltests config and multissltests.py #136728

Open

zware added the skip news label Jul 16, 2025

StanFromIreland added the skip news label Jul 16, 2025

blurb-it bot and others added 4 commits July 16, 2025 20:08

📜🤖 Added by blurb_it.

1fcb49f

Revert "📜🤖 Added by blurb_it."

dd969fb

This reverts commit 1fcb49f.

Merge branch 'main' into refactor-ssl-tests

1edce1c

Include old versions when using default versions

3343120

WillChilds-Klein marked this pull request as ready for review July 16, 2025 20:39

WillChilds-Klein requested review from gpshead, picnixz, ezio-melotti, hugovk and AA-Turner as code owners July 16, 2025 20:39

bedevere-app bot added the awaiting review label Jul 16, 2025

picnixz reviewed Aug 2, 2025

View reviewed changes

hugovk reviewed Aug 5, 2025

View reviewed changes

AA-Turner reviewed Aug 5, 2025

View reviewed changes

	help="SSL lib version(s), default depends on value passed to --ssl",
	help="SSL lib versions, default depends on libs passed to --ssl",

		Please keep this script compatible with all currently-maintained Python
		versions.

	from abc import abstractmethod
	from abc import ABCMeta
	from abc import ABCMeta, abstractmethod

	choices=['openssl', 'awslc', 'libressl'],
	choices=('openssl', 'awslc', 'libressl'),

Uh oh!

gh-136728: Refactor build.yml CI config and multissltests.py #136729

Are you sure you want to change the base?

gh-136728: Refactor build.yml CI config and multissltests.py #136729

Conversation

WillChilds-Klein commented Jul 16, 2025 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

Testing

Uh oh!

picnixz left a comment

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!

picnixz Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

picnixz Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

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!

picnixz Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

WillChilds-Klein commented Jul 16, 2025 •

edited by bedevere-app bot

Loading

picnixz Aug 5, 2025 •

edited

Loading

picnixz Aug 5, 2025 •

edited

Loading

picnixz Aug 5, 2025 •

edited

Loading

picnixz Aug 5, 2025 •

edited

Loading

picnixz Aug 5, 2025 •

edited

Loading