feat(utils): add key generation and token parsing utilities; update d…#21
feat(utils): add key generation and token parsing utilities; update d…#21Christiantyemele wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Just for more security measures, you can ensure the generated private key file is stored securely by verifying that KEY_STORAGE points to a secure location, such as a dedicated directory with limited access or explicitly set file permissions (fs::set_permissions) to restrict access.
i see what you mean here but this too is not enough security, i will prefer not to think the fully of security measure now, but this should be a future ticket
|
Why is the |
…//github.com/ADORSYS-GIS/Status-List-Server into 18-create-function-for-jwt-or-cwt-generation
Hermann-Core
left a comment
Hermann-Core
left a comment
There was a problem hiding this comment.
Each PR should focus on a single independent feature or bug fix, rather than combining multiple changes in one, as seen here. This could have been split into two separate features.
Additionally, the title does not accurately reflect what is being implemented.
Can you review the comments I left below and address them?
| hyper = "1.6" | ||
| hyper = "1.6.0" | ||
| jsonwebtoken = "9.3" | ||
| mongodb = "3.2" |
There was a problem hiding this comment.
what is the mongodb dep doing here? Are you using mongoDB?
| @@ -0,0 +1,29 @@ | |||
| use std::{env, fs, path::Path}; | |||
|
|
|||
| use rsa::{pkcs1::EncodeRsaPrivateKey, pkcs8::DecodePrivateKey, rand_core::OsRng, RsaPrivateKey}; | |||
There was a problem hiding this comment.
Wouldn't it be better to use EC keys instead of RSA keys? They are shorter while providing the same level of security as RSA.
| Ok(private_key) | ||
| } else { | ||
| tracing::info!("Generating new RSA key pair..."); | ||
| let private_key = RsaPrivateKey::new(&mut OsRng, 2048) |
There was a problem hiding this comment.
can you define a constant for the key size? so it will be easy to change it in the future if we want.
There was a problem hiding this comment.
Storing sensitive information, such as cryptographic keys, directly in the filesystem in plaintext is insecure. I recommend using the existing backend storage (the database) to store the keys and implementing a secure key management mechanism.
|
|
||
| use crate::model::{StatusListToken, StatusType}; | ||
|
|
||
| pub fn parse_token( |
There was a problem hiding this comment.
You're encoding the token into the specified format, not parsing it. Consider choosing a more appropriate name for both the function and the module to reflect this.
| ) -> Result<String, Error> { | ||
| match status_type { | ||
| StatusType::JWT => { | ||
| let header = Header::default(); |
There was a problem hiding this comment.
I think the Status List token header is somewhat specialized, particularly the typ, alg, and possibly the x5c headers thus should not be populated with default values. Could you review it again?
| StatusType::CWT => { | ||
| // for feature implmentation | ||
| Ok(String::new()) | ||
| // Serialize the token (claims) to CBOR bytes | ||
| // let claims_bytes = serde_cbor::to_vec(&token).map_err(|e| { | ||
| // tracing::error!("CBOR serialization failed: {}", e); | ||
| // std::io::Error::new(std::io::ErrorKind::Other, "failed to serialize CBOR claims") | ||
| // })?; | ||
|
|
||
| // // Return the CWT as a base64 or hex string if needed | ||
| // Ok(Base64Encoder::encode(&claims_bytes)) | ||
| } |
There was a problem hiding this comment.
Wouldn't it be better to complete the implementation before linking the PR? You've added dependencies for this feature that are not yet in use. I believe the codebase should remain clean and free of unused dependencies after each PR.
| impl FromStr for StatusType { | ||
| type Err = String; | ||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| match s.to_uppercase().as_str() { | ||
| "JWT" => Ok(Self::JWT), | ||
| "CWT" => Ok(Self::CWT), | ||
| _ => Err("Unknown status type".to_string()), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused implementation.
|
@Christiantyemele, you should take a look at @Hermann-Core's reviews. They make a lot of sense, especially the ones concerning the keygen. |
|
i am converting this PR to draft now cause
|
4 participants
…ependencies
closes adorsys/didcomm-mediator-rs#348