image-rs re-introduce ring version that is incompatible with s390x?

[stevenhorsman]

I'm not completely sure what the situation is, so apologies if this isn't clear.

During testing of kata-containers/kata-containers#8670, we ran the kata-agent build with a new commit from image-rs and they failed with:

13:13:34 error: failed to run custom build command for `ring v0.16.20`
13:13:34 
13:13:34 Caused by:
13:13:34   process didn't exit successfully: `/kata-containers/src/agent/target/release/build/ring-884cb6961366a545/build-script-build` (exit status: 101)
13:13:34   --- stderr
13:13:34   thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /root/.cargo/registry/src/index.crates.io-d11c229612889eed/ring-0.16.20/build.rs:358:10

I know ring didn't use to support s390x. That got added in October in briansmith/ring#1297, but 0.16.20 is ~3 years old, so it won't be in there. I know that Ding did some work to ensure that ring wasn't needed to unblock s390x builds of image-rs, has that been undone recently?

It seems to be commit 4ddac38 that introduced this issue as a151bca worked, so we just used that one in the end, but once we re-integrate image-rs with kata-containers we'll hit this issue.

For some of the other guest-components projects we have Makefiles to make it easier to build for multiple architectures and added workflows:

guest-components/.github/workflows/cdh_basic.yml

Lines 59 to 61 in 5d4bb95

	- name: s390x build
	run:
	make ARCH=s390x

that test them, so maybe we need to consider this for image-rs so we can find out about issues before them get integrated with kata-containers?

[mythi]

Ding did some work to ensure that ring wasn't needed to unblock s390x builds of image-rs, has that been undone recently?

Looks like the latest sigstore pulls "tough" which depends on this old ring... :/

[mythi]

sigstore-rs has a dependabot PR open that brings in updated ring. We should just get them to upgrade and cut a new release?

[Xynnn007]

Let me fix the dep in upstream sigstore-rs.

[Xynnn007]

pending pr sigstore/sigstore-rs#320

[Xynnn007]

Before we get the upstream fixed, do you think it is urgent that we need to fix by changing the current sigstore-rs rev to my personal repo? @stevenhorsman

[stevenhorsman]

Before we get the upstream fixed, do you think it is urgent that we need to fix by changing the current sigstore-rs rev to my personal repo? @stevenhorsman

Hey Ding, I'm not completely caught up yet, but I don't think it's super urgent right now. I mostly raised it to try and get a fix once we are ready to merge the image-rs working into kata-agent in main, which I hope is very soon, but at this point probably not worth switching to a fork. Thanks!

[ChengyuZhu6]

@stevenhorsman @Xynnn007 I have encountered this problem in guest-pull PR on s390x and ppc64le:
https://github.com/kata-containers/kata-containers/actions/runs/8047127851/job/21975784913?pr=8484
https://github.com/kata-containers/kata-containers/actions/runs/8047127851/job/21975803238?pr=8484
https://github.com/kata-containers/kata-containers/actions/runs/8047127851/job/21975808824?pr=8484
https://github.com/kata-containers/kata-containers/actions/runs/8047127851/job/21975789375?pr=8484

How far have we come in addressing this issue?

[Xynnn007]

How far have we come in addressing this issue?

Oh. This is a long stack.

1. image-rs depends on sigstore-rs.
2. sigstore-rs depends on tough 0.14.
3. tough 0.14.0 depends on ring 0.16. But tough 0.16 depends on ring 0.17.
4. ring 0.16 does not support s390x and ppc64le but ring 0.17 supports.

I made a PR in sigstore-rs to update tough to 0.16 but CI fails, which I also made a PR to fix in tough awslabs/tough#737.

What we should do now is

1. wait for tough community to publish a new release including the fix. (I have pinged them in the slack channel half a week ago)
2. Update the dep: update tough version to v0.16.0 sigstore/sigstore-rs#320 and get it merge
3. Update the sigstore version in guest-components/image-rs

[mythi]

Oh. This is a long stack.

1. `image-rs` depends on `sigstore-rs`.

2. `sigstore-rs` depends on `tough 0.14`.

@Xynnn007 Correct me if I'm wrong but I believe 2. got added because cosign client could not be built without tuf feature anymore. I submitted an issue about it separately: sigstore/sigstore-rs#318 So perhaps we don't need all of those dependencies but try to make tuf optional? It seems there's some work ongoing for that already

[Xynnn007]

@Xynnn007 Correct me if I'm wrong but I believe 2. got added because cosign client could not be built without tuf feature anymore. I submitted an issue about it separately: sigstore/sigstore-rs#318 So perhaps we don't need all of those dependencies but try to make tuf optional? It seems there's some work ongoing for that already

Right. Both will resolve this but the thread you mentioned seems faster as we do not require one more community. I have reviewed the code of that. Either of them is merged could handle the problem of this issue.

[stevenhorsman]

FYI I've created draft PR #491 to add testing of s390x and powerpc64le so when this is fixed we can try and stop any regressions quicker

[mkulke]

https://www.memorysafety.org/blog/rustls-with-aws-crypto-back-end-and-fips/

we might want to use AWS' backend via feature flag if that compiles on S390x

[Xynnn007]

we might want to use AWS' backend via feature flag if that compiles on S390x

It's a good idea, but the fully replacement of ring by aws-lc-rs requires indirect dependency packages that depend on ring need to be corrected as well. : |

[mkulke]

It's a good idea, but the fully replacement of ring by aws-lc-rs requires indirect dependency packages that depend on ring need to be corrected as well. : |

ahh, it's a transitive dep. yes, this won't help.

[mythi]

we might want to use AWS' backend via feature flag if that compiles on S390x

It's a good idea, but the fully replacement of ring by aws-lc-rs requires indirect dependency packages that depend on ring need to be corrected as well. : |

I believe the topic worth discussing is can we default to one stack on all platforms/architectures going forward to reduce dependencies and simplify the code base/features.

[Xynnn007]

I believe the topic worth discussing is can we default to one stack on all platforms/architectures going forward to reduce dependencies and simplify the code base/features.

We once bring openssl and ring the two different stacks for different platforms. The updations upon ring make it clearer we might not need to, which means we can deprecate all openssl features and deps and convert to ring -- or the opposite, as now the code stack can be built on both stack.

[mythi]

I believe the topic worth discussing is can we default to one stack on all platforms/architectures going forward to reduce dependencies and simplify the code base/features.

We once bring openssl and ring the two different stacks for different platforms. The updations upon ring make it clearer we might not need to, which means we can deprecate all openssl features and deps and convert to ring -- or the opposite, as now the code stack can be built on both stack.

+1, this was the thinking behind my comment. There could be alternatives too, e.g., what @mkulke shared above. It's probably not an urgent move to make but a potential goal we can discuss and agree upon.