[PM-10941] Add ssh import #82
Conversation
|
No New Or Fixed Issues Found |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #82 +/- ##
==========================================
+ Coverage 64.41% 64.75% +0.33%
==========================================
Files 188 190 +2
Lines 13819 13979 +160
==========================================
+ Hits 8902 9052 +150
- Misses 4917 4927 +10 ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Alright, this is a bit of a long one 😅. Generally function wise it seems fine, but we have a few code cleanups we can do along the way to reduce code duplication, rely on slightly less lower level primitives in certain crates and better convey what's happening.
I've left a bunch of suggestions and issues along the way which should be fairly non controversial. But there are also a few thoughts which are more discussion topics.
crates/bitwarden-ssh/src/import.rs
Outdated
| let encrypted_private_key_info = EncryptedPrivateKeyInfo::from_der(der.as_bytes()) | ||
| .map_err(|_| SshKeyImportError::ParsingError)?; | ||
| encrypted_private_key_info | ||
| .decrypt(password.as_bytes()) | ||
| .map_err(|_| SshKeyImportError::WrongPassword)? |
There was a problem hiding this comment.
thought: SecretDoc defines a helper, from_pkcs8_encrypted_pem which could be used. EncryptedPrivateKeyInfo is somewhat lower level in my opinion?
You would need to map the error differently though since you get the generic pkcs8 error but it should expose a DecryptFailed case.
We can go a step further and lift the SecretDocument::from_pem into the else statement since we would be decoding it from scratch and not need the der representation.
There was a problem hiding this comment.
let decrypted_der = if let Some(password) = password {
SecretDocument::from_pkcs8_encrypted_pem(&encoded_key, password.as_bytes()).map_err(
|err| match err {
pkcs8::Error::EncryptedPrivateKey(pkcs5::Error::DecryptFailed) => {
SshKeyImportError::WrongPassword
}
_ => SshKeyImportError::ParsingError,
},
)?
} else {
SecretDocument::from_pkcs8_pem(&encoded_key).map_err(|_| SshKeyImportError::ParsingError)?
};Like this?
There was a problem hiding this comment.
Yes, that seems nice. We could probably even rename it to der now that we don't have a regular der.
There was a problem hiding this comment.
SecretDocument::from_pkcs8_encrypted_pem and SecretDocument::from_pkcs8_pem have the same result type. That allows you to move the error handling out of the nested code block.
Something like:
let der = if let Some(password) = password {
SecretDocument::from_pkcs8_encrypted_pem(&encoded_key, password.as_bytes())
} else {
SecretDocument::from_pkcs8_pem(&encoded_key)
};
let der = match.map_err(|err| match err {
pkcs8::Error::EncryptedPrivateKey(pkcs5::Error::DecryptFailed) =>
SshKeyImportError::WrongPassword,
_ => SshKeyImportError::ParsingError
})?;It works as a one-liner if you want it to, but that feels a bit more cluttered. Splitting out the error handling from the control flow makes the purpose of each block clear and makes the error propagation stand out a bit more.
Rebinding the variable (let der) might seem a bit odd, but it's a common idiom when you're changing a type but not the underlying data. In this case, the first der is a Result<T> and it's unwrapped to a T, which fits the idiom exactly.
| pub enum SshKeyImportError { | ||
| #[error("Failed to parse key")] | ||
| ParsingError, | ||
| #[error("Password required")] | ||
| PasswordRequired, | ||
| #[error("Wrong password")] | ||
| WrongPassword, | ||
| #[error("Unsupported key type")] | ||
| UnsupportedKeyType, | ||
| } |
There was a problem hiding this comment.
❓ @Hinton - Is there a path forward for i18n in the SDK? These seem like the kind of errors we'd like to forward to the user, which would mean communicating the i18n key we'd like the UI to use for localization along with the error.
There was a problem hiding this comment.
No we leave that to the presentation layer for now. Using i18n in lower level services have always been sort of a crutch.
For WASM we support https://sdk-api-docs.bitwarden.com/bitwarden_error_macro/attr.bitwarden_error.html which exposes basic, flat and full. For non user facing errors it's safe to use basic or flat and log it. For user facing errors we generally encourage you to use flat or full, and properly handle the error in the presentation layer.
In this case if we want to show error messages to the user we should use flat or full and capture the different variants in the JS world and apply localization there.
Note: The story on mobile is slightly different due to some uniffi limitations. We're working on providing the same capabilities there.
There was a problem hiding this comment.
Thanks! I'm going to leave this item open for now, but it's not blocking the review.
| } | ||
| } | ||
|
|
||
| fn import_pkcs8_key( |
There was a problem hiding this comment.
🤔 There's a lot of repetitive map_err calls in this method, which leads me to believe that there's a better way to represent this logic.
For example, extracting the parsing out from the processing of the key would let the parser signal a wide number of error conditions. The importer can then map those parsing failures once as a failed parse.
| let private_key: ed25519::KeypairBytes = private_key_info | ||
| .try_into() | ||
| .map_err(|_| SshKeyImportError::ParsingError)?; | ||
| ssh_key::private::PrivateKey::from(Ed25519Keypair::from(&private_key.secret_key.into())) |
There was a problem hiding this comment.
🤔 Three type conversions on a single line leads me to believe that there should be a more direct way to convert key formats. It looks like all of these types come from a library? If so, consider wrapping the logic in a newtype to implement an external trait on an external type.
crates/bitwarden-ssh/src/import.rs
Outdated
| let encrypted_private_key_info = EncryptedPrivateKeyInfo::from_der(der.as_bytes()) | ||
| .map_err(|_| SshKeyImportError::ParsingError)?; | ||
| encrypted_private_key_info | ||
| .decrypt(password.as_bytes()) | ||
| .map_err(|_| SshKeyImportError::WrongPassword)? |
crates/bitwarden-ssh/src/import.rs
Outdated
| if private_key.is_encrypted() { | ||
| if let Some(password) = password { | ||
| private_key | ||
| .decrypt(password.as_bytes()) | ||
| .map_err(|_| SshKeyImportError::WrongPassword)? | ||
| .try_into() | ||
| .map_err(|_| SshKeyImportError::ParsingError) | ||
| } else { | ||
| Err(SshKeyImportError::PasswordRequired) | ||
| } | ||
| } else { | ||
| private_key | ||
| .try_into() | ||
| .map_err(|_| SshKeyImportError::ParsingError) | ||
| } |
There was a problem hiding this comment.
suggestion: We can improve this code slightly.
- The if condition for password can be rewritten using
ok_orwhich converts anOptiontoResult. - We can remove some code duplication by assigning a temporary variable and lifting out some code from the if block.
| if private_key.is_encrypted() { | |
| if let Some(password) = password { | |
| private_key | |
| .decrypt(password.as_bytes()) | |
| .map_err(|_| SshKeyImportError::WrongPassword)? | |
| .try_into() | |
| .map_err(|_| SshKeyImportError::ParsingError) | |
| } else { | |
| Err(SshKeyImportError::PasswordRequired) | |
| } | |
| } else { | |
| private_key | |
| .try_into() | |
| .map_err(|_| SshKeyImportError::ParsingError) | |
| } | |
| let private_key = if private_key.is_encrypted() { | |
| let password = password.ok_or(SshKeyImportError::PasswordRequired)?; | |
| private_key | |
| .decrypt(password.as_bytes()) | |
| .map_err(|_| SshKeyImportError::WrongPassword)? | |
| } else { | |
| private_key | |
| } | |
| private_key | |
| .try_into() | |
| .map_err(|_| SshKeyImportError::ParsingError) |
There was a problem hiding this comment.
We might want to provide some documentation here as well, since this will be available in TS.
Co-authored-by: Oscar Hinton <[email protected]>
Co-authored-by: Oscar Hinton <[email protected]>
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-10941
📔 Objective
Adds ssh import to the sdk so we can validate, decrypt, and convert keys on import on all clients, and unblock importing 1Password keys from pux exports.
This much cleans up the implementation of the
desktop_nativemodule, replacing a lot of mapping and matching code.Further, this moves generator out of
bitwarden-ssh'slib.rs.Note: importer will subsequently be removed from
desktop_native⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes