chore: Use internal secret for JWT key #686
Conversation
adwk67
moved this from Development: In Progress
to Development: Waiting for Review
in Stackable Engineering
maltesander
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
| fn get_random_base64(byte_size: usize) -> String { | ||
| let mut buf: Vec<u8> = vec![0; byte_size]; | ||
| openssl::rand::rand_bytes(&mut buf).unwrap(); | ||
| openssl::base64::encode_block(&buf) | ||
| } |
There was a problem hiding this comment.
Do we really need openssl for that?
There was a problem hiding this comment.
This is consistent with what we have done for Trino.
There was a problem hiding this comment.
I'm with Malte here. Is there no easier way to generate random bytes without using openssl? Especially if we only need openssl for this? (I haven't checked)
There was a problem hiding this comment.
Nevermind. It's also openssl
Alternative (also from Trino): https://github.com/stackabletech/trino-operator/blob/d6943fcca7fc6f08ad08a4fbe20eb555db948455/rust/operator-binary/src/controller.rs#L1502
There was a problem hiding this comment.
The current PR state:
let serial_number = rand::random::<u64>();
let bytes = serial_number.to_le_bytes();
general_purpose::STANDARD.encode(bytes)I'm all in not using openssl!
I think a likely reason we went with openssl is cryptographics. One could unintentional hurt the security by using non-crypto rand function.
So I'm fine with rand, but we should only use the OsRng generator and document why and that we think this is sufficient. See https://github.com/rust-random/rand/blob/master/SECURITY.md#specific-generators
Also, can we increase the 64 bit to 256 bit or would we run into problems?
If we end up with a nice "secure" function we could also throw that in op-rs so we improve this in a central place.
Description
Fixes #639
This change is non-breaking as the secret is created and used only "internally". The user does not have to create a secret themselves. The sample applies for the
connections.secretKeythat is part of the credentials secret used for DB connection information: this has been removed from all code (incl. examples, tests) and replaced with an internal secret. If the field is still provided it will be ignored.Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker