[PM-15934] Add agent-forwarding detection and git signing detection parsers #12371
Conversation
|
New Issues
Fixed Issues
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #12371 +/- ##
==========================================
- Coverage 34.14% 34.14% -0.01%
==========================================
Files 2937 2937
Lines 90353 90360 +7
Branches 16967 16969 +2
==========================================
Hits 30849 30849
- Misses 57049 57056 +7
Partials 2455 2455 ☔ View full report in Codecov by Sentry. |
…den/clients into km/forwarding-and-signing-parsers
quexten
requested review from
addisonbeck and
dani-garcia
and removed request for
addisonbeck
| @@ -36,19 +38,43 @@ pub struct SshAgentUIRequest { | |||
| pub cipher_id: Option<String>, | |||
| pub process_name: String, | |||
| pub is_list: bool, | |||
| pub is_sig: bool, | |||
| pub is_git: bool, | |||
There was a problem hiding this comment.
Tiny non-blocking nit, instead of returning an is_git boolean, would it make more sense to return the namespace directly and let the TypeScript UI choose which message to show then?
Would mean less changes if we ever need to add more messages.
There was a problem hiding this comment.
Agreed, passing this through now.
|
|
||
| pub(crate) fn parse_request(data: &[u8]) -> Result<SshAgentSignRequest, anyhow::Error> { | ||
| let magic_header = "SSHSIG"; | ||
| let mut data_iter = data.to_vec().into_iter(); |
There was a problem hiding this comment.
This is getting a clippy lint for me, I think you can change it to data.iter().copied(); to avoid it.
That said, using iterators like this is kind of unergonomic, for this type of byte handling I usually recommend the bytes crate: https://docs.rs/bytes/latest/bytes/ (tokio is using it extensively so it's already in our builds as a transitive dep)
pub(crate) fn parse_request(data: &[u8]) -> Result<SshAgentSignRequest, anyhow::Error> {
let mut data = Bytes::copy_from_slice(data);
let magic_header = "SSHSIG";
// This splits the six first characters into `header` and `data` advances it's internal cursor by 6
let header = data.split_to(magic_header.len());
// sshsig; based on https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.sshsig
if header == magic_header.as_bytes() {
// This automatically advances the internal cursor by 4
let _version = data.get_u32();
// read until null byte
let namespace = data
.into_iter()
.take_while(|&x| x != 0)
.collect::<Vec<u8>>();PD: Another option is the byteorder crate, which we already have as a dependency, though I haven't used it much: https://docs.rs/byteorder/latest/byteorder
There was a problem hiding this comment.
Nice, this is exactly what I was looking for!
quexten
changed the title
Co-authored-by: Daniel García <[email protected]>
| isForwarding: boolean | ||
| namespace?: string | ||
| } | ||
| export function serve(callback: (err: Error | null, arg: SshUiRequest) => any): Promise<SshAgentState> |
There was a problem hiding this comment.
Nice! Looks better as a separate struct this way, at least we don't get arg0, arg1, arg2 parameters.
There was a problem hiding this comment.
Hehe, yeah. Meant to do it sometime either-way, but in this case clippy complained.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-15934
https://bitwarden.atlassian.net/browse/PM-15956
https://bitwarden.atlassian.net/browse/PM-15964
📔 Objective
This PR adds support for detecting and showing:
In the long term this also enables:
Note for reviewing: The callback is getting kind of long with many params. I kept it for a follow-up refactor for now (after #12166 is also merged) but let me know if you consider it blocking and we can move it to a struct in this PR.
📸 Screenshots
⏰ Reminders before review
🦮 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 confirmed issue 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