-
Notifications
You must be signed in to change notification settings - Fork 138
Dependabot/cargo/jsonwebtoken 10.2.0 manual #1054
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
Dependabot/cargo/jsonwebtoken 10.2.0 manual #1054
Conversation
2dd2785 to
8dfb94f
Compare
| kbs-types = "0.14.0" | ||
| kms = { git = "https://github.com/confidential-containers/guest-components.git", rev = "7be23a1", default-features = false } | ||
| jsonwebtoken = { version = "10", default-features = false } | ||
| jsonwebtoken = { version = "10", default-features = false, features = ["aws_lc_rs"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this link against the aws libcrypto c library? would rust-crypto be an option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rust-crypto is an option too. not sure why they made this choice a breaking change instead of just adding adding aws-lc crypto as an additional feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the result of me compiling grpc-as using gnu (cargo build -p attestation-service --bin grpc-as) and the result of ldd
linux-vdso.so.1 (0x00007ffc8406b000)
libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007fdb7d1ed000)
libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fdb7d1c8000)
libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fdb7d1a5000)
libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fdb7b0b1000)
libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fdb7d19f000)
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fdb7aebf000)
/lib64/ld-linux-x86-64.so.2 (0x00007fdb7d21d000)
also it can be built with musl (cargo build -p attestation-service --bin grpc-as --target x86_64-unknown-linux-musl)
thus it is statically linked.
Is this acceptable?
Currently, jsonwebtoken is imported by this crate and also by its dependency crate rust-ear with aws_lc_rs. If needed to change to rust-crypto, I need to first update the jsonwebtoken dependency on the rust-ear upstream library, switching from aws_lc_rs to rust-crypto and then go back to this PR.
rust-crypto is an option too. not sure why they made this choice a breaking change instead of just adding adding aws-lc crypto as an additional feature
The upstream crate seems decouple the crypto suites into either aws_lc_rs or rust-crypto for developers to choose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are competing requirements, sometimes you require static linking, sometimes static linking is forbidden. I think the former is mostly the case for constrained environments like an initrd in a TEE. for services it's acceptable to depend on a library I guess. If rust-ear already has made the decision to choose aws_lc_rs, we can just go with that, since JWK and ear are tightly coupled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must mis-deliver my idea. The dynamic linked information i showed reflects no aws libc is linked dynamically. Also it supports static link as I showed later. so it does not break the ststic link requirement we always head to.
91383cd to
c3832e8
Compare
kbs/src/token/jwk.rs
Outdated
| #[case( | ||
| "{\"keys\":[{\"kty\":\"oct\",\"alg\":\"COCO42\",\"kid\":\"coco123\",\"k\":\"foobar\"}]}", | ||
| true | ||
| false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test intentionally writes an invalid JWK and we want to check an error is returned. With this change, the test case becomes useless and the test coverage for invalid algos gets lost. If we accept it here, we probably need a check for UNKNOWN_ALGORITHM elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good. Let me do a little change here.
c3832e8 to
2c1f1ef
Compare
Bumps [jsonwebtoken](https://github.com/Keats/jsonwebtoken) from 9.3.1 to 10.2.0. - [Changelog](https://github.com/Keats/jsonwebtoken/blob/master/CHANGELOG.md) - [Commits](Keats/jsonwebtoken@v9.3.1...v10.2.0) --- updated-dependencies: - dependency-name: jsonwebtoken dependency-version: 10.2.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
New version (10) of jsonwebtoken requires crypto features to be enabled to make the building successful. Signed-off-by: Xynnn007 <[email protected]>
The new version of ear also updates jsonwebtoken to version 10, this will fix the not-same-version error when building. Signed-off-by: Xynnn007 <[email protected]>
2c1f1ef to
6f21ddb
Compare
cmake is used for aws_lc_sys crate, which is a trans-dependency for jsonwebtoken. Signed-off-by: Xynnn007 <[email protected]>
New version of jsonwebkey adds a new enum for algorithm 'UNKNOWN_ALGORITHM', thus the algorithm error will not be raised we change the unit test logic to assert the algorithm type. Signed-off-by: Xynnn007 <[email protected]>
libclang is used to build KBS (CoCo-AS-gRPC) version, or the error ``` > [builder 6/7] RUN cd kbs && make AS_FEATURE=coco-as-grpc ALIYUN=false ARCH=s390x NEBULA_CA_PLUGIN=false && make ARCH=s390x install-kbs: 126.0 cargo:warning=See: https://github.com/rust-lang/rust-bindgen?tab=readme-ov-file#environment-variables 126.0 cargo:warning=###### 126.0 126.0 --- stderr 126.0 126.0 thread 'main' panicked at /usr/local/cargo/registry/src/index.crates.io-7f555b6b8ccf4919/bindgen-0.72.1/lib.rs:616:27: 126.0 Unable to find libclang: "couldn't find any valid shared libraries matching: ['libclang.so', 'libclang-*.so', 'libclang.so.*', 'libclang-*.so.*'], set the `LIBCLANG_PATH` environment variable to a path where one of these files can be found (invalid: [])" 126.0 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace 126.0 warning: build failed, waiting for other jobs to finish... 147.8 make: *** [Makefile:79: background-check-kbs] Error 101 Signed-off-by: Xynnn007 <[email protected]>
Signed-off-by: Xynnn007 <[email protected]>
6f21ddb to
4d3435c
Compare
mythi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Xynnn007 thanks for the updates! LGTM
|
There are some CI errors according to the links which is not related to this PR, thus it be acceptable to get this merged. Link fix PR #1080 |
da5722c
into
confidential-containers:main
Manual version for #1050