CC | dependency error on kata-agent for s390x

[BbolroC]

Description of problem

Since #5542 was merged on CCv0, CI jobs (e.g. http://jenkins.katacontainers.io/job/kata-containers-CCv0-ubuntu-20.04-s390x-PR/229/consoleText) for s390x has been failing due to the following build error on agent:

Compiling ed25519 v1.5.2
error: failed to run custom build command for `ring v0.16.20`

Caused by:
  process didn't exit successfully: `/kata-containers/src/agent/target/release/build/ring-117466d433d50a09/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /root/.cargo/registry/src/github.com-eae4ba8cbf2ce1c7/ring-0.16.20/build.rs:358:10
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...
make: *** [Makefile:113: target/s390x-unknown-linux-gnu/release/kata-agent] Error 101

On the image-rs side, confidential-containers/guest-components#47 made a difference for s390x, which introduced sigstore-rs → openidconnect → ring dependency chain (https://github.com/sigstore/sigstore-rs/blob/main/Cargo.toml#L25).

A conversation for the s390x support on ring started with a PR(briansmith/ring#1297) last year, but not yet merged. We need to fix this issue on image-rs or sigstore-rs like what has been made for kata-ctl (https://github.com/kata-containers/kata-containers/blob/main/src/tools/kata-ctl/Cargo.toml#L23) while we are waiting for having the PR for ring merged.

[target.'cfg(target_arch = "s390x")'.dependencies]
reqwest = { version = "0.11", default-features = false, features = ["json", "blocking", "native-tls"] }

[target.'cfg(not(target_arch = "s390x"))'.dependencies]
reqwest = { version = "0.11", default-features = false, features = ["json", "blocking", "rustls-tls"] }

cc: @stevenhorsman, @arronwy, @hbrueckner

[hbrueckner]

@BbolroC Thanks for opening this issue!

Also cc'ing @uweigand for ring dependency awareness.

[arronwy]

@Xynnn007

[BbolroC]

UPDATE

Investigation and test results

1. The code changes below made for kata-ctl cannot solve the problem:

[target.'cfg(target_arch = "s390x")'.dependencies]
reqwest = { version = "0.11", default-features = false, features = ["json", "blocking", "native-tls"] }

[target.'cfg(not(target_arch = "s390x"))'.dependencies]
reqwest = { version = "0.11", default-features = false, features = ["json", "blocking", "rustls-tls"] }

The reqwest has features where we could selectively configure either of them for the crate (even none of them).

• rustls-tls refers to hyper-rustls -> rustls -> ring
• native-tls does not have any chain to ring.

2. The problem is different for the sigstore case (Cargo.toml at the commit referred by image-rs: https://github.com/sigstore/sigstore-rs/blob/v0.3.3/Cargo.toml) , native-tls is configured by default. but the problem comes from openidconnect, where ring is hard-coded as dependency (https://github.com/ramosbugs/openidconnect-rs/blob/main/Cargo.toml#L41)

Conclusion

1. Let sigstore not use openidconnect - not good
2. Let image-rs not use sigstore - not good
3. Get the PR for ring merged - ideal, but no guarantee for when

I look forward to feedback from @uweigand. Thanks @Xynnn007 and @huoqifeng for the discussion

[stevenhorsman]

@fidencio mentioned that td-shim have replaced ring with sha2 https://github.com/confidential-containers/td-shim/pull/428/files, so I wonder if that would be helpful, but would involved getting openidconnect to accept the change I guess. If ring isn't being very slow to accept PRs then they might be interested though.

[BbolroC]

@stevenhorsman Thanks for the idea. I've gone through how ring is used in openidconnect. The search result of the package is around 70 hits over 2 files. which does not look simple to make a PR in a short period of time. (fyi: I am not a crypto expert enough to handle a PR for this. We should find someone else to do this.) Plus, unlike the PR for ring (titled s390x support), it might be really hard to convince an author of openidconnect (https://github.com/ramosbugs) to switch to sha2 just for the issue we are facing.

cc: @hbrueckner

[stevenhorsman]

Plus, unlike the PR for ring (titled s390x support), it might be really hard to convince an author of openidconnect (https://github.com/ramosbugs) to switch to sha2 just for the issue we are facing.

Yep - that makes sense and not unexpected, we just thought we'd throw in the idea. 🤞 we can get the ring PR merged soon.

[Xynnn007]

BTW - This PR is working add RustCrypto support, which can get rid of ring. ramosbugs/openidconnect-rs#58

[hbrueckner]

Hi @BbolroC @stevenhorsman ,

@stevenhorsman Thanks for the idea. I've gone through how ring is used in openidconnect. The search result of the package is around 70 hits over 2 files. which does not look simple to make a PR in a short period of time. (fyi: I am not a crypto

Also looked into ring usage in openidconnect and agree with @BbolroC ... they seem to entirely rely on ring for cryptographic operations.

[hbrueckner]

• This PR is working add RustCrypto support,

Thanks for sharing. And looking at the commit, they indeed replace a couple of crypto operations with the rustCrypto ones (e.g. similar to the mentioned sha2 above.)

[liudalibj]

Looks like @briansmith is working on briansmith/ring#1555, when the issue be finished, maybe openidconnect can run on s390x platform successfully with new released ring version.

[mattarnoatibm]

While we wait for the fix in the ring create to be progressed and completed we're going to attempt a workaround conditionally compiling image-rs without the sigstore-rs crate and without support for cosign signing when its compiled for s390x: confidential-containers/guest-components#80.

[BbolroC]

@stevenhorsman I was wondering if this issue could be closed.

[stevenhorsman]

@stevenhorsman I was wondering if this issue could be closed.

I don't think we should close it, but maybe it needs updating, or possibly moving to image-rs? AFAIK we didn't get ring working on s390x, so cosign support doesn't work (which we'd like, hence keeping this open), we are just avoiding the problem by not building the co-sign feature on s390x.

[BbolroC]

True. Then I will make an update on what is going for this issue this week. Thanks for your opinion. ❤️

[Xynnn007]

FYI, recentlyopenidconnect replaced ring with Rust Cryptos. I'm going to test in the sigstore crate and replace that with the new version, which may help s390x to support cosign.

[stevenhorsman]

@Xynnn007 - thanks for the update and for testing it out. I thought Gerry mentioned that the latest sigstore clashed with another dependency, but hopefully that is resolved/resolvable now?