ipatests: add test for kdcproxy handling reply split to several TCP packets #4614

wladich · 2020-04-29T11:24:00Z

This is a regression test for the bug in python-kdcproxy mentioned in
https://github.com/latchset/kdcproxy/pull/44
  When the reply from AD is split into several TCP packets the kdc
  proxy software cannot handle it and returns a false error message
  indicating it cannot contact the KDC server.

This could be observed as login failures of AD user on IPA clients
when:
* IPA client was configured to use kdcproxy to communicate with AD
* kdcproxy used TCP to communicate with AD
* response from AD to kdcproxy was split into several packets

This patch also refactors and improves existing tests:
* switch to using pytest fixtures for test setup and cleanup steps to make
  them isolated and reusable
* simulate a much more restricted network environment: instead of blocking
  single 88 port we now block all outgoing traffic except few essential
  ports
* add basic tests for using kdcproxy to communicate between IPA client
  and AD DC.

tiran · 2020-04-29T11:32:38Z

ipatests/pytest_ipa/integration/fixtures.py

@@ -0,0 +1,30 @@
+#


pytest fixtures are usually stored in a conftest.py, which is auto-imported by pytest.

This approach is applicable for fixtures which are applicable for all tests. Here this fixture is specific to a particular set of tests, so IMO its better to have this in that test class's install class method.

This is ipa_ad_trust fixture. It can be used to establish trust between IPA and AD when no special options are required. So this fixture could be used in this module, in test_smb, test_sssd and any new test suite which needs to setup trust.

@wladich
Still this fixture servers the trust scenario only.
I think we can move this fixture to a shared file(e.g pytest_ipa/integreation/tasks.py) and can import/use where ever necessary.

@kaleemsiddiqu Please see comment #4614 (comment)

Personally I do not have any preference where to store this small fixture - in a separate file or in conftest or somewhere else.

I only want to say that scope of fixtures defined in conftest.py is very large and applies to almost every tests and thats when we should define it. IMO ipa_trust_ad does not fit that case. Its upto you and @tiran to decide, i just shared my opinion.

tiran · 2020-04-29T11:35:04Z

ipatests/test_integration/test_http_kdc_proxy.py

+            ['OUTPUT', '-p', 'tcp', '--dport', '80', '-j', 'ACCEPT'],
+            ['OUTPUT', '-p', 'tcp', '--dport', '443', '-j', 'ACCEPT'],
+            ['OUTPUT', '-p', 'tcp', '--sport', '22', '-j', 'ACCEPT']]
+        Firewall(self.client).prepend_passthrough_rules(fw_rules_allow)


Is it necessary to create a new firewall object for every function call?

definitely not, fixing.

tiran · 2020-04-29T11:39:32Z

ipatests/test_integration/test_http_kdc_proxy.py

+    @pytest.mark.usefixtures('restrict_network_for_client',
+                             'client_use_kdcproxy')
+    def test_ipa_user_login_on_client_with_kdcproxy(self, users):


Please use follow black's and PEP-8's formatting style:

@pytest.mark.usefixtures( 'restrict_network_for_client', 'client_use_kdcproxy' ) def test_ipa_user_login_on_client_with_kdcproxy(self, users):

Could you please point which pep-8 style rule you are referring to?
And https://www.freeipa.org/page/Python_Coding_Style does not seem to mention any other rules...

It's black code style, https://github.com/psf/black#the-black-code-style

I tried to use black utility with new code and it heavily reformatted the code, not only the mentioned lines. One of the changes is making lines longer then 79 chars which violates current code style.
But even with "-l 79" the changes are huge.
So could you please clarify, is the use of black formater now agreed and enforced for the FreeIPA project? I see it is not mentioned in https://www.freeipa.org/page/Python_Coding_Style, neither it is being checked by PR-CI.

netoarmando · 2020-04-30T20:22:30Z

@wladich, please, increase the timeout argument, I suspect that the reason jobs are picked by multiple runners is related to that.

netoarmando · 2020-04-30T20:23:02Z

@wladich, please, increase the timeout argument, I suspect that the reason jobs are picked by multiple runners is related to that.

wladich · 2020-05-05T09:59:53Z

All tests pass in rawhide and one test expectedly fails in Fedora 31

tiran · 2020-05-15T10:31:23Z

ipatests/conftest.py

+
+
+@pytest.fixture(scope='class')
+def ipa_ad_trust(mh):


This fixture only applies to integration tests and depends on the multihost mh fixture. Please move it to ipatests/pytest_ipa/integration/__init__.py.

@tiran
fixed

wladich · 2020-05-19T08:59:01Z

Tests fail at ipa-server-install: http://freeipa-org-pr-ci.s3-website.eu-central-1.amazonaws.com/jobs/d7be85da-9942-11ea-b3a3-fa163e168469

wladich · 2020-06-03T10:21:02Z

Rerunning the tests to check that they still pass after freeipa/freeipa-pr-ci@7c6f67e

This is a workaround for https://pagure.io/freeipa/issue/6523: when ipa-client-install is called with --server option, dns_lookup_realm and dns_lookup_kdc options in kreb5.conf are set to "false" preventing libkrb5 from discovering AD realm. This function changes those values to "true".

Similar to kinit_admin, this allows to check for error values returned by kinit.

The fixture enables us to setup and cleanup trust between IPA master and AD root with single line of code. It also handles configuring clients to be able to accept logins of AD users.

…ackets This is a regression test for the bug in python-kdcproxy mentioned in latchset/kdcproxy#44 When the reply from AD is split into several TCP packets the kdc proxy software cannot handle it and returns a false error message indicating it cannot contact the KDC server. This could be observed as login failures of AD user on IPA clients when: * IPA client was configured to use kdcproxy to communicate with AD * kdcproxy used TCP to communicate with AD * response from AD to kdcproxy was split into several packets This patch also refactors and improves existing tests: * switch to using pytest fixtures for test setup and cleanup steps to make them isolated and reusable * simulate a much more restricted network environment: instead of blocking single 88 port we now block all outgoing traffic except few essential ports * add basic tests for using kdcproxy to communicate between IPA client and AD DC.

The test fails with older versions of kdcproxy as the related bug was fixed in 0.4.2: latchset/kdcproxy#44

the new tests require ad AD instance

stale · 2020-08-07T19:51:16Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

wladich added ipa-4-6 Mark for backport to ipa 4.6 ipa-4-8 Mark for backport to ipa 4.8 labels Apr 29, 2020

tiran requested changes Apr 29, 2020

View reviewed changes

wladich force-pushed the kdcproxy-ad-test branch 2 times, most recently from a7f0723 to 8ab71ab Compare April 29, 2020 16:28

wladich force-pushed the kdcproxy-ad-test branch from 8ab71ab to eec72c0 Compare May 4, 2020 09:48

wladich added the re-run Trigger a new run of PR-CI label May 4, 2020

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label May 4, 2020

wladich force-pushed the kdcproxy-ad-test branch from eec72c0 to cd05fb9 Compare May 4, 2020 15:07

wladich added the needs review Pull Request is waiting for a review label May 5, 2020

tiran requested changes May 15, 2020

View reviewed changes

wladich force-pushed the kdcproxy-ad-test branch from cd05fb9 to 1580d2e Compare May 18, 2020 09:47

wladich added the re-run Trigger a new run of PR-CI label May 18, 2020

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label May 18, 2020

wladich added the re-run Trigger a new run of PR-CI label May 19, 2020

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label May 19, 2020

wladich force-pushed the kdcproxy-ad-test branch from 1580d2e to 0a46235 Compare May 19, 2020 11:59

wladich removed the needs review Pull Request is waiting for a review label May 20, 2020

wladich force-pushed the kdcproxy-ad-test branch from 0a46235 to db75d50 Compare May 20, 2020 09:58

wladich added the re-run Trigger a new run of PR-CI label May 20, 2020

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label May 20, 2020

wladich added the needs review Pull Request is waiting for a review label May 20, 2020

wladich force-pushed the kdcproxy-ad-test branch from db75d50 to b454e13 Compare June 3, 2020 10:16

wladich mentioned this pull request Jun 3, 2020

Failure in dnf install during PRCI tests freeipa/freeipa-pr-ci#363

Closed

sorlov-rh added 7 commits June 3, 2020 14:37

ipatests: return result of kinit_as_user, pass raiseonerr parameter

bc3a6d1

Similar to kinit_admin, this allows to check for error values returned by kinit.

ipatests: add a fixture for establishing IPA-AD trust

0ee2a82

The fixture enables us to setup and cleanup trust between IPA master and AD root with single line of code. It also handles configuring clients to be able to accept logins of AD users.

ipatests: add function for getting python package version on remote host

b42624a

ipatests: mark test_kdcproxy_handles_small_packets_from_ad xfail

06b2ea9

The test fails with older versions of kdcproxy as the related bug was fixed in 0.4.2: latchset/kdcproxy#44

ipatests: update prci definitions for test_http_kdc_proxy

6c63af4

the new tests require ad AD instance

temp commit

366e407

wladich force-pushed the kdcproxy-ad-test branch from b454e13 to 366e407 Compare June 3, 2020 12:56

freeipa-pr-ci added the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Jun 8, 2020

stale bot added the stale Stale PR [Bot] label Aug 7, 2020

wladich added postponed and removed needs review Pull Request is waiting for a review labels Aug 10, 2020

stale bot removed the stale Stale PR [Bot] label Aug 10, 2020

wladich marked this pull request as draft February 19, 2021 12:25

wladich closed this Mar 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipatests: add test for kdcproxy handling reply split to several TCP packets #4614

ipatests: add test for kdcproxy handling reply split to several TCP packets #4614

wladich commented Apr 29, 2020

tiran Apr 29, 2020

wladich Apr 29, 2020 •

edited

Loading

kaleemsiddiqu May 5, 2020

wladich May 7, 2020

kaleemsiddiqu May 11, 2020

wladich May 11, 2020

kaleemsiddiqu May 11, 2020

tiran Apr 29, 2020

wladich Apr 29, 2020

tiran Apr 29, 2020

wladich Apr 29, 2020

tiran Apr 29, 2020

wladich May 4, 2020 •

edited

Loading

netoarmando commented Apr 30, 2020

netoarmando commented Apr 30, 2020

wladich commented May 5, 2020

tiran May 15, 2020

wladich May 20, 2020

wladich commented May 19, 2020

wladich commented Jun 3, 2020

stale bot commented Aug 7, 2020

ipatests: add test for kdcproxy handling reply split to several TCP packets #4614

ipatests: add test for kdcproxy handling reply split to several TCP packets #4614

Conversation

wladich commented Apr 29, 2020

Choose a reason for hiding this comment

wladich Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wladich May 4, 2020 • edited Loading

Choose a reason for hiding this comment

netoarmando commented Apr 30, 2020

netoarmando commented Apr 30, 2020

wladich commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wladich commented May 19, 2020

wladich commented Jun 3, 2020

stale bot commented Aug 7, 2020

wladich Apr 29, 2020 •

edited

Loading

wladich May 4, 2020 •

edited

Loading