Update dhcp relay test for option82 stripping #6330

ganglyu · 2022-09-10T02:16:52Z

Signed-off-by: Gang Lv ganglv@microsoft.com

Description of PR

Summary:
Fixes # (issue)

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Back port request

201911
202012

Approach

What is the motivation for this PR?

dhcp relay has an option82 related pr, and we need to update e2e test to verify this issue:
sonic-net/sonic-buildimage#12033

How did you do it?

Update dhcp offer packet, add option82 and another option. After dhcp relay, option82 should be stripped.

How did you verify/test it?

I used kvm to verify this test case.

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

Signed-off-by: Gang Lv ganglv@microsoft.com

yejianquan · 2022-09-10T03:10:45Z

/azp run

azure-pipelines · 2022-09-10T03:10:53Z

Azure Pipelines successfully started running 1 pipeline(s).

qiluo-msft · 2022-09-10T21:38:51Z

ansible/roles/test/files/ptftests/py3/dhcp_relay_test.py

    def create_dhcp_offer_packet(self):
-        return testutils.dhcp_offer_packet(eth_server=self.server_iface_mac,


testutils.dhcp_offer_packet

This function is also used in tests/dhcp_relay/test_dhcp_pkt_fwd.py. Do you need to change that file? #Closed

I replace this function to add option82, and I don't think we need it for test_dhcp_pkt_fwd.

qiluo-msft · 2022-09-10T21:47:29Z

ansible/roles/test/files/ptftests/py3/dhcp_relay_test.py

@@ -284,13 +356,14 @@ def create_dhcp_offer_relayed_packet(self):
                    ('server_id', self.server_ip),
                    ('lease_time', self.LEASE_TIME),
                    ('subnet_mask', self.client_subnet),
+                    ("vendor_class_id", "http://10.23.145.67:9876/abc/defghijk/lmnopqrst.bin".encode('utf-8')),


http://10.23.145.67:9876/abc/defghijk/lmnopqrst.bin

Is it a random URL?
If yes, can we use more fictional URL like [http://0.0.0.0/test/test.bin](http://0.0.0.0/test/test.bin`) ? #Closed

http://0.0.0.0/test/test.bin is not long enough to overwrite option82.

This information is priceless, and please add it as code comment or assert, in case future dev (even yourself) mistakenly break it.

The the sample is like [http://0.0.0.0/very_long_long_test/test.bin](http://0.0.0.0/very_long_long_test/test.bin`)

We do not want to a true IP in the world hit by DDOS.

qiluo-msft · 2022-09-10T21:48:20Z

ansible/roles/test/files/ptftests/py3/dhcp_relay_test.py

@@ -284,13 +356,14 @@ def create_dhcp_offer_relayed_packet(self):
                    ('server_id', self.server_ip),
                    ('lease_time', self.LEASE_TIME),
                    ('subnet_mask', self.client_subnet),
+                    ("vendor_class_id", "http://10.23.145.67:9876/abc/defghijk/lmnopqrst.bin".encode('utf-8')),
                    ('end')])

        # TODO: Need to add this to the packet creation functions in PTF code first!


TODO

Did you actually resolve the TODO issue? #Closed

qiluo-msft · 2022-09-10T21:59:21Z

@saikrishnagajula Could you help review?

Why I did it Current isc-dhcp uses below code to remove DHCP option: memmove(sp, op, op[1] + 2); sp += op[1] + 2; sp points to the option to be stripped, we can call it as option S. op points to the option after options S, we can call it as option O. DHCP option is a typical type-length-value structure, the first byte is type, the second byte is length, and remain parts are value. In this case, option O length is bigger than option S, and more than 2 bytes, after the memmove, we will get this result: Now Option S and Option O are overwritten, op[1] was the length of Option O, and it's modified after memmove. But current implementation is still using op[1] as length to update sp (sp+=op[1]+2), so we get the wrong sp. How I did it Create patch from https://github.com/isc-projects/dhcp The new impelementation use mlen to store the length of Option O before memmove, that's how it fixed the bug. size_t mlen = op[1] + 2; memmove(sp, op, mlen); sp += mlen; How to verify it I have a PR for sonic-mgmt to cover this issue: sonic-net/sonic-mgmt#6330 Signed-off-by: Gang Lv ganglv@microsoft.com

ganglyu · 2022-09-19T01:23:47Z

/azp run

azure-pipelines · 2022-09-19T01:23:56Z

Azure Pipelines successfully started running 1 pipeline(s).

ganglyu · 2022-09-21T02:45:09Z

/azp run

azure-pipelines · 2022-09-21T02:45:18Z

Azure Pipelines successfully started running 1 pipeline(s).

Why I did it Current isc-dhcp uses below code to remove DHCP option: memmove(sp, op, op[1] + 2); sp += op[1] + 2; sp points to the option to be stripped, we can call it as option S. op points to the option after options S, we can call it as option O. DHCP option is a typical type-length-value structure, the first byte is type, the second byte is length, and remain parts are value. In this case, option O length is bigger than option S, and more than 2 bytes, after the memmove, we will get this result: Now Option S and Option O are overwritten, op[1] was the length of Option O, and it's modified after memmove. But current implementation is still using op[1] as length to update sp (sp+=op[1]+2), so we get the wrong sp. How I did it Create patch from https://github.com/isc-projects/dhcp The new impelementation use mlen to store the length of Option O before memmove, that's how it fixed the bug. size_t mlen = op[1] + 2; memmove(sp, op, mlen); sp += mlen; How to verify it I have a PR for sonic-mgmt to cover this issue: sonic-net/sonic-mgmt#6330 Signed-off-by: Gang Lv ganglv@microsoft.com

ganglyu · 2022-09-24T10:50:45Z

/azp run

azure-pipelines · 2022-09-25T07:09:47Z

Azure Pipelines successfully started running 1 pipeline(s).

ganglyu · 2022-09-25T10:21:39Z

/azp run

azure-pipelines · 2022-09-25T10:21:48Z

Azure Pipelines successfully started running 1 pipeline(s).

ganglyu · 2022-09-25T11:03:20Z

/azp run

azure-pipelines · 2022-09-25T11:03:28Z

Azure Pipelines successfully started running 1 pipeline(s).

ganglyu · 2022-09-25T11:54:32Z

/azp run

azure-pipelines · 2022-09-25T11:54:41Z

Azure Pipelines successfully started running 1 pipeline(s).

ganglyu · 2022-09-25T12:42:38Z

/azp run

azure-pipelines · 2022-09-25T12:42:46Z

Azure Pipelines successfully started running 1 pipeline(s).

ganglyu · 2022-09-25T13:51:18Z

/azp run

azure-pipelines · 2022-09-25T13:51:26Z

Azure Pipelines successfully started running 1 pipeline(s).

ganglyu · 2022-09-25T22:34:52Z

/azp run

azure-pipelines · 2022-09-25T22:35:01Z

Azure Pipelines successfully started running 1 pipeline(s).

ganglyu · 2022-09-25T23:17:41Z

/azp run

azure-pipelines · 2022-09-25T23:17:49Z

Azure Pipelines successfully started running 1 pipeline(s).

ganglyu · 2022-09-25T23:54:41Z

/azp run

azure-pipelines · 2022-09-25T23:54:50Z

Azure Pipelines successfully started running 1 pipeline(s).

ganglyu · 2022-09-26T01:25:44Z

/azp run

azure-pipelines · 2022-09-26T01:25:53Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Gang Lv ganglv@microsoft.com What is the motivation for this PR? dhcp relay has an option82 related pr, and we need to update e2e test to verify this issue: sonic-net/sonic-buildimage#12033 How did you do it? Update dhcp offer packet, add option82 and another option. After dhcp relay, option82 should be stripped. How did you verify/test it? I used kvm to verify this test case.

What is the motivation for this PR? dhcp_relay_test.py has been upgraded to python3, so cherry-pick from master to 202012 does not work. So, I need to revert the commit, and create a new PR for 202012 branch. How did you do it? Revert changes for dhcp relay test How did you verify/test it? Run dhcp relay test.

Why I did it Current isc-dhcp uses below code to remove DHCP option: memmove(sp, op, op[1] + 2); sp += op[1] + 2; sp points to the option to be stripped, we can call it as option S. op points to the option after options S, we can call it as option O. DHCP option is a typical type-length-value structure, the first byte is type, the second byte is length, and remain parts are value. In this case, option O length is bigger than option S, and more than 2 bytes, after the memmove, we will get this result: Now Option S and Option O are overwritten, op[1] was the length of Option O, and it's modified after memmove. But current implementation is still using op[1] as length to update sp (sp+=op[1]+2), so we get the wrong sp. How I did it Create patch from https://github.com/isc-projects/dhcp The new impelementation use mlen to store the length of Option O before memmove, that's how it fixed the bug. size_t mlen = op[1] + 2; memmove(sp, op, mlen); sp += mlen; How to verify it I have a PR for sonic-mgmt to cover this issue: sonic-net/sonic-mgmt#6330 Signed-off-by: Gang Lv ganglv@microsoft.com

Why I did it Current isc-dhcp uses below code to remove DHCP option: memmove(sp, op, op[1] + 2); sp += op[1] + 2; sp points to the option to be stripped, we can call it as option S. op points to the option after options S, we can call it as option O. DHCP option is a typical type-length-value structure, the first byte is type, the second byte is length, and remain parts are value. In this case, option O length is bigger than option S, and more than 2 bytes, after the memmove, we will get this result: Now Option S and Option O are overwritten, op[1] was the length of Option O, and it's modified after memmove. But current implementation is still using op[1] as length to update sp (sp+=op[1]+2), so we get the wrong sp. How I did it Create patch from https://github.com/isc-projects/dhcp The new impelementation use mlen to store the length of Option O before memmove, that's how it fixed the bug. size_t mlen = op[1] + 2; memmove(sp, op, mlen); sp += mlen; How to verify it I have a PR for sonic-mgmt to cover this issue: sonic-net/sonic-mgmt#6330 Signed-off-by: Gang Lv ganglv@microsoft.com Signed-off-by: Gang Lv ganglv@microsoft.com Co-authored-by: ganglv <88995770+ganglyu@users.noreply.github.com>

Signed-off-by: Gang Lv ganglv@microsoft.com What is the motivation for this PR? dhcp relay has an option82 related pr, and we need to update e2e test to verify this issue: sonic-net/sonic-buildimage#12033 How did you do it? Update dhcp offer packet, add option82 and another option. After dhcp relay, option82 should be stripped. How did you verify/test it? I used kvm to verify this test case.

Update dhcp relay test for option82 strip

120bd17

Signed-off-by: Gang Lv ganglv@microsoft.com

ganglyu requested a review from qiluo-msft September 10, 2022 02:16

ganglyu mentioned this pull request Sep 10, 2022

Fix dhcp option buffer issue sonic-net/sonic-buildimage#12033

Merged

7 tasks

qiluo-msft reviewed Sep 10, 2022

View reviewed changes

qiluo-msft requested review from yxieca, kellyyeh and lguohan September 10, 2022 21:59

ganglyu added 2 commits September 11, 2022 08:15

Remove TODO

4ccc224

Add comment for option limitation, and use better string

bf5dab7

kellyyeh approved these changes Sep 13, 2022

View reviewed changes

qiluo-msft approved these changes Sep 13, 2022

View reviewed changes

ganglyu added Included in 202012 branch Included in 202205 branch Request for 202012 branch Request for 202205 branch and removed Included in 202012 branch Included in 202205 branch labels Sep 17, 2022

ganglyu merged commit 6523ac0 into sonic-net:master Sep 26, 2022

wangxin added the Included in 202205 branch label Sep 28, 2022

wangxin added the Included in 202012 branch label Sep 28, 2022

ganglyu mentioned this pull request Oct 28, 2022

Update dhcp relay test for option82 stripping #6623

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update dhcp relay test for option82 stripping #6330

Update dhcp relay test for option82 stripping #6330

ganglyu commented Sep 10, 2022

yejianquan commented Sep 10, 2022

azure-pipelines bot commented Sep 10, 2022

qiluo-msft Sep 10, 2022 •

edited

Loading

ganglyu Sep 10, 2022

qiluo-msft Sep 10, 2022 •

edited

Loading

ganglyu Sep 10, 2022

qiluo-msft Sep 11, 2022

qiluo-msft Sep 11, 2022

qiluo-msft Sep 10, 2022 •

edited

Loading

qiluo-msft commented Sep 10, 2022

ganglyu commented Sep 19, 2022

azure-pipelines bot commented Sep 19, 2022

ganglyu commented Sep 21, 2022

azure-pipelines bot commented Sep 21, 2022

ganglyu commented Sep 24, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 26, 2022

azure-pipelines bot commented Sep 26, 2022

		def create_dhcp_offer_packet(self):
		return testutils.dhcp_offer_packet(eth_server=self.server_iface_mac,

Update dhcp relay test for option82 stripping #6330

Update dhcp relay test for option82 stripping #6330

Conversation

ganglyu commented Sep 10, 2022

Description of PR

Type of change

Back port request

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

yejianquan commented Sep 10, 2022

azure-pipelines bot commented Sep 10, 2022

qiluo-msft Sep 10, 2022 • edited Loading

Choose a reason for hiding this comment

ganglyu Sep 10, 2022

Choose a reason for hiding this comment

qiluo-msft Sep 10, 2022 • edited Loading

Choose a reason for hiding this comment

ganglyu Sep 10, 2022

Choose a reason for hiding this comment

qiluo-msft Sep 11, 2022

Choose a reason for hiding this comment

qiluo-msft Sep 11, 2022

Choose a reason for hiding this comment

qiluo-msft Sep 10, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft commented Sep 10, 2022

ganglyu commented Sep 19, 2022

azure-pipelines bot commented Sep 19, 2022

ganglyu commented Sep 21, 2022

azure-pipelines bot commented Sep 21, 2022

ganglyu commented Sep 24, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 25, 2022

azure-pipelines bot commented Sep 25, 2022

ganglyu commented Sep 26, 2022

azure-pipelines bot commented Sep 26, 2022

qiluo-msft Sep 10, 2022 •

edited

Loading

qiluo-msft Sep 10, 2022 •

edited

Loading

qiluo-msft Sep 10, 2022 •

edited

Loading