DecodingKey is difficult to re-use

tjwilson90 commented 4 years ago

impl<'a> DecodingKey<'a> {
    pub fn from_rsa_pem(key: &'a [u8]) -> Result<Self> {
        let pem_key = PemEncodedKey::new(key)?;
        let content = pem_key.as_rsa_key()?;
        Ok(DecodingKey {
            family: AlgorithmFamily::Rsa,
            kind: DecodingKeyKind::SecretOrDer(Cow::Owned(content.to_vec())),
        })
    }
}

This constructs a DecodingKey that owns all of its state, but the signature suggests that it borrows the input key. As a result, the only way to construct a DecodingKey<'static> for lazy_static or once_cell is to leak the input key (or unsafely transmute the lifetime of the input key and hope that DecodingKey never changes its implementation to actually.borrow the input).

Keats commented 4 years ago

The intent was indeed to use it in a lazy_static way, something like (pseudo-code)

lazy_static! {
  let decoding_key = {
      let bytes = include_bytes!("public.pem");
      DecodingKey::from_rsa_pem(bytes).unwrap()
  }
}

What would you change?

tjwilson90 commented 4 years ago

That works, but only if I embed my public key in my program (which would require me to recompile and redeploy if I change my key, and would make it impossible for a user of my pre-compiled program to generate and use their own keys).

Since the resulting DecodingKey doesn't need to maintain a borrow on the key, the key should have a distinct lifetime, i.e., the type of key should be &[u8], not &'a [u8].

code-elf commented 4 years ago

Just ran into this as well. It'd be way more versatile if DecodingKey's methods just borrowed or consumed the key, depending on internal implementation, rather than requiring an external guarantee about the lifetime.

Keats commented 4 years ago

I've pushed a stop-gap change in 7.1.0, more changes can be done later if necessary as breaking change

dakom commented 4 years ago

Right now my setup is something like this:

lazy_static! {
    pub static ref JWT_ENCODING_KEY:EncodingKey = {
        let secret = std::env::var("JWT_SECRET").expect("must have JWT_SECRET set");
        EncodingKey::from_secret(secret.as_ref())
    };

    pub static ref JWT_DECODING_KEY:DecodingKey<'static> = {
        let secret = std::env::var("JWT_SECRET").expect("must have JWT_SECRET set");
        DecodingKey::from_secret(secret.as_ref())
    };

It fails to compile of course.

EncodingKey is fine, but DecodingKey fails since secret is dropped right away.

imho DecodingKey should really just own its data. There's really only two scenarios iirc:

Totally new keys. Anyway it has to be re-created, so holding references doesn't help
Total re-use, in which case a reference to the DecodingKey is what's needed, and internal references of the DecodingKey don't help.
Partial re-use. Here is where it would make sense for the DecodingKey to hold references, so, say, part of its data could be swapped out. I don't think that applies here?

Am I missing something?

Keats commented 4 years ago

https://github.com/Keats/jsonwebtoken/pull/128 should make it easier but that's a breaking change.

dakom commented 4 years ago

How about adding this to that breaking change as well?

impl DecodingKey<'static> {
    pub fn from_secret_clone(secret: &[u8]) -> Self {
        DecodingKey {
            family: AlgorithmFamily::Hmac,
            kind: DecodingKeyKind::SecretOrDer(Cow::Owned(secret.to_vec())),
        }
    }
}

Keats commented 4 years ago

Can we work on a PR with breaking changes for the future 8.0? You can go wild

cc @neoeinstein

As well as any other issues for EncodingKey or really anything else.

Keats commented 3 years ago

Anyone up to do a PR on https://github.com/Keats/jsonwebtoken/pull/160 ?

Keats commented 3 years ago

Can people try https://github.com/Keats/jsonwebtoken/pull/160 to see if it solves all the issues they had?

Keats commented 3 years ago

It's also on crates.io as v8 beta 0