Skip to content

CCP protocol implementation #1858

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 26, 2019
Merged

CCP protocol implementation #1858

merged 3 commits into from
Feb 26, 2019

Conversation

polybassa
Copy link
Contributor

No description provided.

Copy link

@ebroecker ebroecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good at first glance

@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

Merging #1858 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1858      +/-   ##
==========================================
- Coverage   85.77%   85.75%   -0.03%     
==========================================
  Files         185      185              
  Lines       42473    42473              
==========================================
- Hits        36430    36421       -9     
- Misses       6043     6052       +9
Impacted Files Coverage Δ
scapy/pton_ntop.py 89.04% <0%> (-8.22%) ⬇️
scapy/layers/tls/crypto/pkcs1.py 78.67% <0%> (-1.48%) ⬇️
scapy/automaton.py 81.81% <0%> (-0.56%) ⬇️
scapy/layers/tls/handshake_sslv2.py 91.32% <0%> (-0.38%) ⬇️
scapy/arch/windows/__init__.py 71.67% <0%> (-0.32%) ⬇️
scapy/layers/inet6.py 88.28% <0%> (+0.05%) ⬆️
scapy/layers/ntp.py 91.77% <0%> (+0.26%) ⬆️
scapy/sendrecv.py 85.88% <0%> (+0.68%) ⬆️

@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

Merging #1858 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1858      +/-   ##
==========================================
- Coverage   85.76%   85.75%   -0.02%     
==========================================
  Files         185      185              
  Lines       42473    42473              
==========================================
- Hits        36429    36421       -8     
- Misses       6044     6052       +8
Impacted Files Coverage Δ
scapy/contrib/cansocket_python_can.py 91.8% <ø> (ø) ⬆️
scapy/sendrecv.py 84.86% <0%> (-0.52%) ⬇️
scapy/layers/tls/handshake_sslv2.py 91.32% <0%> (-0.38%) ⬇️
scapy/layers/inet6.py 88.05% <0%> (-0.24%) ⬇️

@gpotter2
Copy link
Member

gpotter2 commented Feb 19, 2019

Hey @polybassa, some of the ISOTP tests are a bit unstable. Do you think you could have a look ? Thanks !
https://travis-ci.org/secdev/scapy/jobs/495430130

Edit: My bad.. I restarted the test which destroyed the logs... sorry will come back to you when it fails again

We have a retry_test function in scapy.tools.UTscapy in last resort, if the failure isn’t related to your test (what we use when pinging servers / doing dns), but I don’t think you should be using it for local tests

@polybassa
Copy link
Contributor Author

polybassa commented Feb 19, 2019 via email

@polybassa
Copy link
Contributor Author

polybassa commented Feb 19, 2019 via email

@gpotter2
Copy link
Member

Here’s a failure: https://travis-ci.org/secdev/scapy/jobs/495745915

And here’s a backup of the logs: https://pastebin.com/Fz1fxT0T

@polybassa
Copy link
Contributor Author

Hi @gpotter2. I've created PR #1863. Hope this makes the test more stable.

@gpotter2 gpotter2 mentioned this pull request Feb 22, 2019
27 tasks
more work on ccp

finish implementation of ccp

more unit tests

more unit tests

fix minor bug to support the basecls parameter in pythoncan-sockets

add ccp documentation
@polybassa
Copy link
Contributor Author

This PR is complete, from my side.

Copy link
Member

@gpotter2 gpotter2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good !

PS: Note that we’re currently having a global test failure due to a python-can update. Any new test will fail, they’re working on it.

data = bytes(self.load)
self.remove_payload()
self.add_payload(new_pl_cls(data))
self.payload_cls = new_pl_cls
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you document a bit what this is doing ?

I don’t think one would expect answers() to edit the packets, it is commonly only a test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@polybassa
Copy link
Contributor Author

I saw this bug with python-can. Is there something I can do for you? Would it be a solution, to use a fixed version of python-can?

@gpotter2
Copy link
Member

gpotter2 commented Feb 25, 2019

Python-can just released a new version. The issue is now fixed :-)

This PR fails the spelling check:

scapy/contrib/automotive/ccp.py:562: dependant  ==> dependent

@gpotter2 gpotter2 merged commit 94fb470 into secdev:master Feb 26, 2019
@gpotter2
Copy link
Member

Thanks for your work !

@polybassa
Copy link
Contributor Author

polybassa commented Feb 26, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants