Re: [Wireshark-dev] Multiple-line parsing of packets dissected over HTTP

[Jonathan Nieder]

Hi,

Pascal Quantin wrote:
> Le mar. 19 janv. 2021 à 17:45, Joey Salazar via Wireshark-dev <
> wireshark-dev@wireshark.org> a écrit :

>> In commit 33af2649 [1] we can keep dissecting the contents of the req,
>> adv, and res packets by setting
>>  while (plen > 0) { }
>> either in `dissect_git_pdu()` or in `dissect_one_pkt_line()`, but for now
>> in `dissect_git_pdu()` it'd be a bit messy, so wanted to ask for your
>> feedback for getting `dissect_one_pkt_line()` to work properly first.
>>
>> As you can see in pcap 169 [2], it correctly parses the length of the
>> first line as 0x0014 (20 bytes) until `0x0a`, then it's supposed to get the
>> length of the next line by the first 4 hex bytes in that line, but instead
>> of reading the length as 0x0018 (24 bytes) it's reading it as 0x0010 (16
>> bytes), and anyways, this particular line's length actually is 59 bytes.

Interesting.  Let me summarize your question: getting the information
in one place here, the relevant code at line 114 looks like

| +  while (plen > 0) {
| +proto_tree_add_uint(git_tree, hf_git_packet_len, tvb, offset, 4, plen);
| +offset += 4;
| +plen -= 4;
| +
| +proto_tree_add_item(git_tree, hf_git_packet_data, tvb, offset, plen, 
ENC_NA);
| +offset += plen;
| +// To-do: add lines for parsing of terminator packet 
| +  }

The relevant part of the pcap is shown in an image; transcribing
imperfectly, I see

| 0014command=ls-refs\n
| 0018agent=git/2.29.0.rc2
| 0016object-format=sha1
| 0001
[etc]

where \n denotes a newline byte and there are no newlines between
these pkt-lines.

That first pkt-line has 4 bytes for the length, followed by 12 bytes
for "command=ls-refs\n", including newline.  So why does this parse as

packet-length: 0x0014
packet data: "command=ls-refs\n"
packet-length: 0x0010
packet data: "agent=[etc]"

?

[...]
> So what is the code leading to this dissection? It does not seem to be
> https://gitlab.com/joeysal/wireshark/-/commit/33af2649927cb5660d4aeb64b9a9e9a58a1823aa
> as dissect_one_pkt_line() seem to read only one line (BTW using a while
> loop in this commit is useless as you are incrementing offset by plen, and
> the code you shared considers that plen includes the 4 bytes of the packet
> length field while your screenshot does not assume that).

This reply is a bit dense, but it contains the hints to move forward:

First, there's the matter of the contract of the dissect_one_pkt_line()
function.  The name suggests it would dissect a *single* pkt-line, but
it has this loop in it.  What does it actually do?  And what do we
want it to do?

That second question is easy to answer: this code will be much easier
to read if dissect_one_pktline dissects a single pkt-line!  For
example, if we imagine a contract like

/** Dissects a single pkt-line.
 *
 * @param[in] tvb Buffer containing a pkt-line.
 * @param offset Offset at which to start reading.
 * @param[out] tree Tree to attach the dissected pkt-line to.
 * @return Number of bytes dissected, or -1 on error.
 */
static int dissect_one_pkt_line(tvbuff_t *tvb, int offset, proto_tree 
*tree)

then we could call this in a loop, like:

int offset = 0;

while (offset < total length)
offset += dissect_one_pkt_line(tvb, offset, tree);

Obtaining the total length and including some error handling left as
an exercise to the reader.

As for the first question: what does the current code do?  The loop at
l114 doesn't modify plen except by subtracting 4 from it.  So instead
of reading the pkt-length from the next pkt-line, it assumes it is 4
bytes less.  0x14 - 4 is 0x10, hence the 0x10 as pkt length
assumption.

Thanks and hope that helps,
Jonathan
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

[Wireshark-dev] Assigned reviewers

[Jonathan Nieder]

Hi wiresharks,

Context: 
https://gitlab.com/wireshark/wireshark/-/merge_requests/1313#note_478706594

In Gerrit times, a person could add someone as a reviewer to a change
to request review, the reviewer could remove themselves if they were
unavailable, and so on.  What is the equivalent in the GitLab world?
More concretely:

- when a change is ready to review, how do I say so?
- if a review seems to be stalled, what's the best place to poke?
- if I would like to review a change, how should I signal interest?
- what happens when a change has been approved and it is time to merge
  it?  Where can I read about the bot that does that?

I checked docbook/wsdg_src/WSDG_chapter_sources.adoc[1] as a first
guess of where to find these answers and didn't get a clear sense of
things.  I'll be happy to contribute a summary of what I learn there.

Thanks for your kind help in reviews while we've been guessing. :)

Thanks,
Jonathan

[1] https://www.wireshark.org/docs/wsdg_html_chunked/ChSrcContribute.html
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] [Outreachy] Internship blog 2020

[Jonathan Nieder]

Hi Joey,

Joey Salazar wrote:

> Very happy to be joining for this winter's internship! A short blog
> entry on the beginning of this journey here: https://jsal.home.blog/
>
> A new entry every 2 weeks, check it out!
>
> Thank you Outreachy, Git, and Wireshark for the opportunity, happy
> to be working together!

Welcome!  I'm looking forward to working together (starting with an
initial wireshark patch to get the lay of the land, then fleshing out
the plan for the project, then executing on it in earnest).

A question for wireshark developers: are there preferences for what a
high quality dissector looks like?  [1] talks about portability and
robustness but doesn't address other aspects of convention such as how
long functions should be (like [2] does).  Is there a rule of thumb
like "when in doubt, do things the way 
does?"

Excited,
Jonathan

[1] https://gitlab.com/wireshark/wireshark/-/raw/master/doc/README.developer
[2] https://www.kernel.org/doc/html/v5.9/process/coding-style.html
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] [Outreachy] Introduction

[Jonathan Nieder]

Hi Joey,

Joey S wrote:

> Hi everyone,
>
> I'm Joey Salazar, an Outreachy 2020 applicant certified in CCNA and
> Linux+, with some experience in C for both private and open source
> repositories (BIND at gitlab.isc.org/Joey), community code reviews,
> and automated tests in bash. I'd like to contribute to the "Add Git
> protocol support to Wireshark" project and improve my skills, yet
> remain open to a different project if that'd be preferable.

Welcome!

> I have installed and built git, followed
> git.github.io/General-Microproject-Information and checked the
> sample email thread [1], as well as the tutorial
> git-scm.com/docs/MyFirstContribution and created the psuh command
> files here [2].
>
> After following the git.github.io/Outreachy-21-Microprojects page
> I'd like to work on the 'Use test_path_is_* functions in test
> scripts', and given that Charvi Mendiratta might be working on tests
> t7101,t7102 and t7201 as per this ml thread [3], I'd like to check
> with the group if working on tests t7006 and t7300 would be ok.

I'd recommend just doing a single file.  t7006 is a good one.

> In parallel, I'm following
> gitlab.com/wireshark/wireshark/-/wikis/Development as suggested
> through the #git-devel IRC channel

Yes, building wireshark and making your first modification to it would
be a good next step.

One possible first modification would be to teach
epan/dissectors/packet-git.c about sideband.  See "Packfile Data" in
git's Documentation/technical/pack-protocol.txt for how sideband
works.

Alternatively you can run wireshark and see if anything you see
bothers you and make a first contribution that improves on that. :)

Happy developing.

Thanks,
Jonathan

> [1] 
> public-inbox.org/git/1386590745-4412-1-git-send-email-t.gumme...@gmail.com/T/#u
> [2] github.com/j-sal/git/tree/psuh
> [3] 
> public-inbox.org/git/cap8ufd1m2zxum1pxmjkw2mxa9xzvuokbfa62jlp7jx6_dcy...@mail.gmail.com
> [4] git-scm.com/book/en/v2/Git-Internals-Transfer-Protocols
[5] https://www.wireshark.org/lists/wireshark-dev/202010/msg00042.html
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Introduction - An Outreachy 2020 Applicant

[Jonathan Nieder]

Hi Amanda,

Amanda Shafack wrote:

> Also, regarding my initial starter contribution, I chose the
> "micro-project" titled "Avoid pipes in git related commands in test
> scripts" (source https://bit.ly/3iZQcT2).

Thanks, sounds good.

> I plan to complete the micro-project and then move on to more research
> on Git's HTTP protocol and other resources you've pointed out.
>
> Let me know your thoughts on this, thanks.

I think that a good next step after the Git micro-project is to get to
know wireshark --- this would involve building wireshark, finding
something to change, and then changing it.

https://gitlab.com/wireshark/wireshark/-/wikis/Development/ describes
how to download and build wireshark.  Then for something to change, I
have a few different ideas:

a. tighten the error handling in epan/dissectors/packet-git.c
   (for example, what happens when there are not exactly 4 hexdigits
   at the beginning of a pkt-line?).  Git's
   Documentation/technical/protocol-common.txt describes the pkt-line
   format and Documentation/technical/pack-protocol.txt describes the
   Git transport dissected by packet-git.

b. add tests for the Git dissector.  test/README.test and the page it
   links to describe how wireshark's tests work

c. try to parse out the service name and protocol version in
   epan/dissectors/packet-git.c.  Git's
   Documentation/technical/protocol-v2.txt describes where we can find
   that information

That would get us more comfortable with the wireshark codebase and
would help prepare for fleshing out a plan for the internship.

Thoughts?

Sincerely,
Jonathan
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Introduction - An Outreachy 2020 Applicant

[Jonathan Nieder]

Richard Sharpe wrote:
> Amanda Shafack wrote:

>> Good day.
>>
>> I'm Amanda Shafack, an Outreachy 2020 applicant who wishes to
>> contribute to the "Add Git protocol support to Wireshark" project.
>>
>> I have some experience coding in C and I hope to enhance
>> my skill set by contributing to this project.
>>
>> In addition, I'm fascinated by network protocols and it's really
>> exciting to get my hands around these concepts in a real-world
>> project.
>>
>> I'm currently going through the project description and contribution
>> guidelines.
>
> Welcome. There are many helpful people on the list.
>
> It would be useful if you can point us to a protocol description
> document but that can wait until help is needed. It may be that all
> the online resources are sufficient, but if not, do not hesitate to
> use this list to ask questions.

There's an overview of Git's HTTP protocol in the Pro Git book:

  https://git-scm.com/book/en/v2/Git-Internals-Transfer-Protocols

The official protocol spec in git.git 
(https://git.kernel.org/pub/scm/git/git.git)
is split between a few files:

  Documentation/technical/protocol-common.txt:
conventions for protocol docs

  Documentation/technical/pack-protocol.txt:
overview of Git protocol

  Documentation/technical/protocol-capabilities.txt:
optional capabilities

  Documentation/technical/protocol-v2:
protocol v2, the new default (more about this is at

https://opensource.googleblog.com/2018/05/introducing-git-protocol-version-2.html)

  Documentation/technical/http-protocol.txt:
how this works with http

  Documentation/technical/pack-format.txt:
packed representation of Git objects (used both on disk and over
the wire)

Amanda, if you have any questions, please don't hesitate to ask
(#git-devel on IRC is the best place for that, and email works as
well).  I am there around 15:00-24:00 UTC most days.  In addition to
working on your initial starter contributions, we can start to put
together a plan for the project.

Richard, to set expectations: the internship period for accepted
interns starts in December: https://www.outreachy.org/apply/project-selection/.
Until then, potential interns are often not free full time, so we
focus on smaller contributions that help get their feet wet in the
codebase.

Excited,
Jonathan
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Introduction - An Outreachy 2020 Applicant

[Jonathan Nieder]

Jonathan Nieder wrote:

> +wireshark-dev@wireshark.org
> Hi Amanda,
>
> Amanda Shafack wrote:
>
>> I am Amanda Shafack, an Outreachy 2020 applicant who wishes to
>> contribute to the "Add Git protocol support to Wireshark" project.
>>
>> In addition, I have some experience coding in C and I hope to enhance
>> my skill set by contributing to this project.
>>
>> I am currently going through the project description and contribution
>> guidelines.
>
> Welcome!
>
> Since this project would involve Git (for Git protocol) and Wireshark
> (where the dissector goes), we're comfortable working with you on
> contributions to both Git and Wireshark during the application[1]
> period.
>
> https://gitlab.com/wireshark/wireshark/-/wikis/Development/ has some
> pointers on getting started with Wireshark development.  I'm cc-ing
> the wireshark developers list in case they have suggestions for an
> approachable "first patch" idea to get used to that project's
> contribution flow.

Ahem, actually cc-ing that list this time.  Sorry for the noise.

> It's also a good idea to build and run wireshark and see if anything
> strikes your eye as something you'd be interested in seeing work
> differently.
>
> For Git we have some suggestions for microprojects at
> https://git.github.io/Outreachy-21-Microprojects/
>
> Thanks for writing, and I look forward to working with you.  These are
> two open source projects that I love and I hope you enjoy working with
> them, too. :)

Sincerely,
Jonathan

> [1] https://www.outreachy.org/apply/
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

[Wireshark-dev] Joint project with Git for outreachy

[Jonathan Nieder]

Hi wiresharkers,

Outreachy  is a program similar to the
Google Summer of Code, providing internships to work on open source
projects.

Emily (cc-ed) and I would be interested in mentoring an outreachy
intern on adding support for the Git protocol to wireshark.  We think
this would be helpful for documenting the spec better, for making
debugging easier for future Git developers, and for helping people
working on systems involving Git (e.g. CI systems) to understand the
behavior of the systems they oversee.  We think that a co-mentor
within wireshark would be helpful for making sure an intern is set up
for success (helping them find pointers, making sure their approach is
on the right track, etc).

This would be a project under the Git umbrella
.

What do you think?  Does this sound interesting to you?

Thanks,
Jonathan
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe