vaultwarden
vaultwarden copied to clipboard
dani-garcia
Improve JWT RSA key initialization and avoid saving public key
Nice change.
P.S.: Just to clarify, this means I can delete the rsa_key.pub.pem file from my vw data directory, correct?
Nice change.
P.S.: Just to clarify, this means I can delete the
rsa_key.pub.pemfile from my vw data directory, correct?
Yes it does.
@dani-garcia It looks ok too me.
The only thing i encountered is that if i generate a RSA key manually like openssl genrsa -out rsa_key.pem 3072 that works, but if i go heigher, like 5120, it will load, but it will also fail generating a JWT since the key size is too large.
Maybe we want to put a limit on the size or at least check the size? Or maybe even allow a different key type? ECDSA for example? That is smaller, though not sure if that the generated JWT's are also smaller though.
Maybe we want to put a limit on the size or at least check the size? Or maybe even allow a different key type? ECDSA for example? That is smaller, though not sure if that the generated JWT's are also smaller though.
The proposed change was made under the assumption that we only wanted 2048-bit RSA keys; and consequently, I should have added a check in the else block that the key size is 2048.
If we want to allow different key sizes and types, then I think that should be configurable. Personally I prefer non-NIST elliptic curves, but NIST curves have more support (e.g., IANA only recommends NIST elliptic curves for TLS—sans the non-NIST Edwards curves (e.g., X25519)—not necessarily on "security" grounds but also how popular they are (e.g., NIST P-521 is not recommended)). Additionally we may not want to have a bunch of granular config options.
For that reason I think a config option that is a tuple of type and size would be nice with the two types being ECC and RSA and the sizes being 256 and 384 for ECC and 2048 and 4096 for RSA, and whose default value is (ECC, 256) where ECC means specifically NIST curves (e.g., NIST P-256 aka secp256r1 aka prime256v1). The code changes for this are pretty minimal.
If the argument for this though is purely based on an admin generating the private key outside of the application, then I think only a check that verifies the key size is 2048 should happen. The key generation is part of the application logic no different than how things are stored in the database, and I don't think that should be supported. Obviously an admin that knows what they're doing can alter or manually generate such things, but they do so under "no warranty".
Not sure why I didn't think of this before, but can't we just remove the file altogether? What's the point of storing the key? It does not take long to generate a private key especially if the decision is to switch to something like NIST P-256, and I have never had issues changing keys. One could even argue this improves security a bit since it makes the key at least semi-ephemeral—yes, this is an abuse of terminology; but the lifetime of the key is the lifetime of the running daemon which is shorter than a persistent key that requires manual deletion to generate a new one.
On my personal fork, I went ahead and removed the file and decided to use the twisted Edwards curve birationally equivalent to Curve25519; and much to my delight, the clients my wife and I use have no issues. The only clients we use though are the web vault accessed via Firefox on Arch Linux and macOS Ventura, the Firefox extension on those same OSes, the app on iOS 17, and the app on Android 11. Using the curve used by Ed25519 may cause issues for other clients though, so it probably makes sense to just use NIST P-256 with the possibility of making that configurable. For my use case though, the code looks like below:
use crate::error::Error;
use jsonwebtoken::{Algorithm, DecodingKey, EncodingKey};
use openssl::pkey::PKey;
use std::sync::OnceLock;
const JWT_ALGORITHM: Algorithm = Algorithm::EdDSA;
static ED_KEYS: OnceLock<(EncodingKey, DecodingKey)> = OnceLock::new();
#[allow(clippy::map_err_ignore)]
pub fn init_ed_keys() -> Result<(), Error> {
PKey::generate_ed25519()
.map_err(Error::from)
.and_then(|key| {
ED_KEYS
.set((
EncodingKey::from_ed_pem(key.private_key_to_pem_pkcs8()?.as_slice())?,
DecodingKey::from_ed_pem(key.public_key_to_pem()?.as_slice())?,
))
.map_err(|_| Error::from(String::from("ED_KEYS must only be initialized once")))
})
}
fn get_private_ed_key() -> &'static EncodingKey {
&ED_KEYS
.get()
.expect("ED_KEYS must be initialized in main")
.0
}
fn get_public_ed_key() -> &'static DecodingKey {
&ED_KEYS
.get()
.expect("ED_KEYS must be initialized in main")
.1
}
Fewer lines of code, no need to access the file system, the gold standard of public key algorithms/curves, a semi-ephemeral key, no sensitive data stored on the file system, smaller key sizes, and smaller signatures. A win-win-win-win-win-win-win.
In theory that makes sense, but I see a case where you'd run into an issue.
You start vw and the key is generated and used. You invite someone or add someone to an org. Then you restart the vw server. What now? The tokens are no longer valid, because they were created with a key that no longer exists. Neither on disk, nor in memory.
Or did I miss something?
In theory that makes sense, but I see a case where you'd run into an issue.
You start vw and the key is generated and used. You invite someone or add someone to an org. Then you restart the vw server. What now? The tokens are no longer valid, because they were created with a key that no longer exists. Neither on disk, nor in memory.
Or did I miss something?
Not only invites, but also refresh tokens, auth tokens etc.. are invalid after that
In theory that makes sense, but I see a case where you'd run into an issue.
You start vw and the key is generated and used. You invite someone or add someone to an org. Then you restart the vw server. What now? The tokens are no longer valid, because they were created with a key that no longer exists. Neither on disk, nor in memory.
Or did I miss something?
No, that seems like a great reason to generate a file. The code would look like below:
use crate::config::Config;
use crate::error::Error;
use jsonwebtoken::{Algorithm, DecodingKey, EncodingKey};
use openssl::pkey::{Id, PKey};
use std::fs::File;
use std::io::{Read, Write};
use std::sync::OnceLock;
const JWT_ALGORITHM: Algorithm = Algorithm::EdDSA;
static ED_KEYS: OnceLock<(EncodingKey, DecodingKey)> = OnceLock::new();
#[allow(clippy::map_err_ignore, clippy::verbose_file_reads)]
pub fn init_ed_keys() -> Result<(), Error> {
let mut file = File::options()
.create(true)
.read(true)
.write(true)
.open(CONFIG.private_ed25519_key())?;
let mut priv_pem = Vec::with_capacity(128);
let ed_key = if file.read_to_end(&mut priv_pem)? == 0 {
let ed_key = PKey::generate_ed25519()?;
priv_pem = ed_key.private_key_to_pem_pkcs8()?;
file.write_all(priv_pem.as_slice())?;
ed_key
} else {
let ed_key = PKey::private_key_from_pem(priv_pem.as_slice())?;
if ed_key.id() == Id::ED25519 {
ed_key
} else {
return Err(Error::from(format!(
"{} is not a private Ed25519 key",
CONFIG.private_ed25519_key()
)));
}
};
ED_KEYS
.set((
EncodingKey::from_ed_pem(priv_pem.as_slice())?,
DecodingKey::from_ed_pem(ed_key.public_key_to_pem()?.as_slice())?,
))
.map_err(|_| Error::from(String::from("ED_KEYS must only be initialized once")))
}
Only a win-win-win :(.
In the likely case NIST P-256 is chosen, I did find jsonwebtoken::EncodingKey does not like openssl::ec::EcKey::private_key_to_pem, so you'll have to pass the EcKey into PKey::from_ec_key—PKey::ec_gen only works for OpenSSL 3.0.0+.
May I ask what the status of this PR is? I am asking because maybe I can help with something.
We can merge this as is and leave the discussion of different ciphers for another PR.
I think migrating to ed25519 would make the most sense, as it would also cut down on the generated JWTs signature sizes (a quick test with the admin page JWT reduced it's size from 499 bytes with RSA2048 to 243 bytes with ED25519). The clients aren't checking the JWT signature so this change shouldn't cause any compatibility issues.
We might need to consider keeping the RSA decoding at least for one release to still be able to handle older invites etc, but in my opinion this shouldn't need to be configurable as long as we use a secure algorithm and key size. If any admins want to use a different key size, well they are on their own for that.
We can merge this as is and leave the discussion of different ciphers for another PR.
I think migrating to ed25519 would make the most sense, as it would also cut down on the generated JWTs signature sizes (a quick test with the admin page JWT reduced it's size from 499 bytes with RSA2048 to 243 bytes with ED25519). The clients aren't checking the JWT signature so this change shouldn't cause any compatibility issues.
We might need to consider keeping the RSA decoding at least for one release to still be able to handle older invites etc, but in my opinion this shouldn't need to be configurable as long as we use a secure algorithm and key size. If any admins want to use a different key size, well they are on their own for that.
I agree. Merging as is seems good to me. The rest would be extra for later.