kbs: token: add verifier with JSON Web Keys

[mythi]

Add a new token verifier that uses JSON Web Keys (JWK) from
the configured JWK Set sources.

JWK Sets can be provided locally using file:// URL schema
or they can be downloaded automatically via OpenID Connect configuration
https:// URLs providing a pointer via "jwks_uri".

[zvonkok]

Can you add a meaningfull description? Thanks.

[mythi]

@Xynnn007 a bit offtopic but is there a reason why CoCo tokenverifier could not use jsonwebtoken also? AFAUI, we could have a single verifier that just understands local pem/jwks and OpenID Connect provided jwks:

trusted_cert_paths = ["/path/to/foo.pem", "https://provider.openid.conf", "/path/to/bar.jwks"]

[mythi]

Can you add a meaningfull description? Thanks.

done

[Xynnn007]

@Xynnn007 a bit offtopic but is there a reason why CoCo tokenverifier could not use jsonwebtoken also? AFAUI, we could have a single verifier that just understands local pem/jwks and OpenID Connect provided jwks:

I did not remember the concrete reason for it is a long time ago -- but I think it makes sense to use a common crate for the same functions.

[mythi]

@Xynnn007 a bit offtopic but is there a reason why CoCo tokenverifier could not use jsonwebtoken also? AFAUI, we could have a single verifier that just understands local pem/jwks and OpenID Connect provided jwks:

I did not remember the concrete reason for it is a long time ago -- but I think it makes sense to use a common crate for the same functions.

I was also wondering if it let us to drop dep:openssl.

[huoqifeng]

I was also wondering if it let us to drop dep:openssl.

For some verifier like IBMSE, openssl is the only certified cryptography library.

[mythi]

I was also wondering if it let us to drop dep:openssl.

For some verifier like IBMSE, openssl is the only certified cryptography library.

OK. I was mainly referring to the CoCo tokenverifier code.

[mythi]

@jialez0 PTAL

[mythi]

rebased + force pushed. however, this change is relatively independent and does not add any new dependencies. are there any concerns or reasons to wait with this?

[Xynnn007]

Only two left comments.

[Xynnn007]

Why not first

let url = Url::parse(p).map_err(...)?;
// then
match url {
...
}

[mythi]

the current match combines all checks under it. It is an error also when some insecure scheme is used.

[Xynnn007]

Ok. A first impression is

// failed to parse
let url = Url::parse(p).map_err(...)?;

if url.scheme() == "https" {
    ...
}

if url.scheme() == "file" && Path::new(url.path()).exists() {
    ...
}

...

[mythi]

I can move the error check arm to be the first one?

[mythi]

moved

[fitzthum]

I second Ding's original suggestion here. I would just get the error check out of the way in the beginning rather than adding another set of brackets. This will also allow you to return a more specific error.

I think it tends to be more readable to use match statements more like switch statements and to avoid chaining them together unless it really fits the fundamental logic of the function.

[mythi]

I just updated this part too, thanks for the feedback!

[mythi]

Only two left comments.

what's the other one?

[Xynnn007]

@mythi it is #458 (comment)

[mythi]

@mythi it is #458 (comment)

thanks, it's easy to miss comments in resolved conversations.

[fitzthum]

A few comments on style but looks good overall.

[fitzthum]

I second Ding's original suggestion here. I would just get the error check out of the way in the beginning rather than adding another set of brackets. This will also allow you to return a more specific error.

I think it tends to be more readable to use match statements more like switch statements and to avoid chaining them together unless it really fits the fundamental logic of the function.

[fitzthum]

I wouldn't use a match here since there is really no logic in this case. You can just use map instead.

[Xynnn007]

Agreed. With the type change of trusted_certs_paths, we can simply write as

    pub async fn new(config: AttestationTokenVerifierConfig) -> Result<Self> {
        let keys = stream::iter(config.trusted_certs_paths)
            .filter_map(|p| async move {
                match get_jwks_from_file_or_url(&p).await {
                    Ok(jwkset) => Some(jwkset.keys),
                    Err(e) => {
                        warn!("error getting JWKS: {e:?}");
                        None
                    }
                }
            })
            .concat()
            .await;

        Ok(Self { trusted_certs: jwk::JwkSet { keys } })
    }

where futures_util crate is needed.

[mythi]

Right, I guess I blindly followed the CoCo approach so it ended up looking the same

[mythi]

where futures_util crate is needed.

the proposal looks nice but I wonder if this is justified for the functionality here?

[Xynnn007]

What the stream::iter does, is try to do get_jwks_from_file_or_url concurrently rather than serially if multiple paths in trusted_certs_paths are given. This makes sense to me. wdyt?

[mythi]

My use-case is with one URL only but I understand we may have several trusted_certs_path in the future where parallelism might help to improve the KBS startup time.

[Xynnn007]

Agreed. With the type change of trusted_certs_paths, we can simply write as

    pub async fn new(config: AttestationTokenVerifierConfig) -> Result<Self> {
        let keys = stream::iter(config.trusted_certs_paths)
            .filter_map(|p| async move {
                match get_jwks_from_file_or_url(&p).await {
                    Ok(jwkset) => Some(jwkset.keys),
                    Err(e) => {
                        warn!("error getting JWKS: {e:?}");
                        None
                    }
                }
            })
            .concat()
            .await;

        Ok(Self { trusted_certs: jwk::JwkSet { keys } })
    }

where futures_util crate is needed.

[fitzthum]

See my comment on Ding's thread, but generally this seems fine.

[fitzthum]

Let's merge this if you are happy with it @Xynnn007