pylint: Fix R1732 consider-using-with #937

stefanberger · 2022-03-22T22:28:24Z

Signed-off-by: Stefan Berger stefanb@linux.ibm.com

THS-on

I do not see why the e2e are failing on this PR. Can you rebase on the latest master?

keylime/cmd_exec.py

THS-on · 2022-03-23T07:54:08Z

keylime/keylime_agent.py

-        f = open(secdir + "/" + self.server.enc_keyname, 'w', encoding="utf-8")
-        f.write(base64.b64encode(self.server.K).decode())
-        f.close()
+        with open(secdir + "/" + self.server.enc_keyname, 'w', encoding="utf-8") as f:


Can you fix the string formatting to os.path.join?.

I know that this is not directly related to this issue, but I try to convert them if I change the line anyway.

Fixed locally.

THS-on · 2022-03-23T07:55:14Z

keylime/secure_mount.py

@@ -20,7 +20,7 @@
 def check_mounted(secdir):
    """Inspect mountinfo to detect if a directory is mounted."""
    secdir_escaped = secdir.replace(" ", r"\040")
-    for line in open("/proc/self/mountinfo", "r", encoding="utf-8"):
+    for line in open("/proc/self/mountinfo", "r", encoding="utf-8"):  #pylint: disable=consider-using-with


Can we not open the file with with and then iterate over readline()?

This seems to work fine for me with python 3.10 just running this here:

>>> with open("/proc/self/mountinfo", "r", encoding="utf-8") as f: ... for line in f: ... print(line)

On the CI/CD system we now get this here:

Error in test.test_secure_mount.TestSecureMount.test_check_mounted_found File "/usr/lib64/python3.9/unittest/case.py", line 59, in testPartExecutor yield File "/usr/lib64/python3.9/unittest/case.py", line 592, in run self._callTestMethod(testMethod) File "/usr/lib64/python3.9/unittest/case.py", line 550, in _callTestMethod method() File "/usr/lib64/python3.9/unittest/mock.py", line 1336, in patched return func(*newargs, **newkeywargs) File "/__w/keylime/keylime/test/test_secure_mount.py", line 31, in test_check_mounted_found self.assertTrue(secure_mount.check_mounted("/secdir")) File "/__w/keylime/keylime/keylime/secure_mount.py", line 23, in check_mounted with open("/proc/self/mountinfo", "r", encoding="utf-8") as f: AttributeError: __enter__

This is beacuse the open call is mocked, but the __enter__ and __exit__ functions are not: https://github.com/keylime/keylime/blob/master/test/test_secure_mount.py

What's the solution?

So odd...

>>> subprocess.Popen.__enter__ <function Popen.__enter__ at 0x6fffffdf2d30> >>> open.__enter__ Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'builtin_function_or_method' object has no attribute '__enter__' >>> os.open.__enter__ Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'builtin_function_or_method' object has no attribute '__enter__'

open(..) and os.open(..) are two different functions.

Also the __enter__ function seems to be only there when the function is called:

>>> open.__enter__ Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'builtin_function_or_method' object has no attribute '__enter__'. Did you mean: '__ne__'? >>> open("tesfile", "wb").__enter__ <built-in method __enter__ of _io.BufferedWriter object at 0x7f8266d521f0>

There seems to be a mocking object in the library for that: https://docs.python.org/3.7/library/unittest.mock.html#mock-open

Fixed in additional patch. What a struggle!

THS-on · 2022-03-23T07:59:44Z

The warning is called R1732 not W1732.

stefanberger · 2022-03-23T10:47:49Z

When testing 447ef9f there is one test-failure in testing-farm:fedora-35-x86_64:

::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::
::   -c delete
::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::

:: [ 10:34:35 ] :: [  BEGIN   ] :: Running 'lime_keylime_tenant -c delete'
Reading configuration from ['/etc/keylime.conf']
2022-03-23 10:34:36.296 - keylime.tpm - INFO - TPM2-TOOLS Version: 5.2
2022-03-23 10:34:36.302 - keylime.tenant - INFO - Setting up client TLS in /var/lib/keylime/cv_ca
2022-03-23 10:34:36.302 - keylime.tenant - WARNING - Using default UUID d432fbb3-d2f1-4a97-9ef7-75bd81c00000
2022-03-23 10:34:36.329 - keylime.tenant - INFO - Agent Info:
{"d432fbb3-d2f1-4a97-9ef7-75bd81c00000": {"operational_state": "Get Quote", "v": "5YiO9Kc1QK9h2OSdfiD0+uVbZ7imZhZmQsLcshl0QsY=", "ip": "127.0.0.1", "port": 9002, "tpm_policy": "{\"22\": [\"0000000000000000000000000000000000000001\", \"0000000000000000000000000000000000000000000000000000000000000001\", \"000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001\", \"ffffffffffffffffffffffffffffffffffffffff\", \"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff\", \"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff\"], \"15\": [\"0000000000000000000000000000000000000000\", \"0000000000000000000000000000000000000000000000000000000000000000\", \"000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\"], \"mask\": \"0x408400\"}", "vtpm_policy": "{\"23\": [\"ffffffffffffffffffffffffffffffffffffffff\", \"0000000000000000000000000000000000000000\"], \"15\": [\"0000000000000000000000000000000000000000\"], \"mask\": \"0x808000\"}", "meta_data": "{}", "allowlist_len": 6, "mb_refstate_len": 0, "accept_tpm_hash_algs": ["sha512", "sha384", "sha256", "sha1"], "accept_tpm_encryption_algs": ["ecc", "rsa"], "accept_tpm_signing_algs": ["ecschnorr", "rsassa"], "hash_alg": "sha1", "enc_alg": "rsa", "sign_alg": "rsassa", "verifier_id": "default", "verifier_ip": "127.0.0.1", "verifier_port": 8881, "severity_level": null, "last_event_id": null}}
2022-03-23 10:34:41.387 - keylime.tenant - ERROR - Timed out waiting for delete of agent d432fbb3-d2f1-4a97-9ef7-75bd81c00000 to complete at CV
:: [ 10:34:41 ] :: [   PASS   ] :: Command 'lime_keylime_tenant -c delete' (Expected 0, got 0)
:: [ 10:34:41 ] :: [   FAIL   ] :: File '/var/tmp/rlRun_LOG.AoxQaNdP' should contain 'INFO - CV completed deletion of agent d432fbb3-d2f1-4a97-9ef7-75bd81c00000' 
:: [ 10:34:41 ] :: [  BEGIN   ] :: Running 'lime_keylime_tenant -c cvstatus'
Reading configuration from ['/etc/keylime.conf']
2022-03-23 10:34:42.204 - keylime.tpm - INFO - TPM2-TOOLS Version: 5.2
2022-03-23 10:34:42.209 - keylime.tenant - INFO - Setting up client TLS in /var/lib/keylime/cv_ca
2022-03-23 10:34:42.209 - keylime.tenant - WARNING - Using default UUID d432fbb3-d2f1-4a97-9ef7-75bd81c00000
2022-03-23 10:34:42.227 - keylime.tenant - INFO - Agent Info:
{"d432fbb3-d2f1-4a97-9ef7-75bd81c00000": {"operational_state": "Get Quote", "v": "5YiO9Kc1QK9h2OSdfiD0+uVbZ7imZhZmQsLcshl0QsY=", "ip": "127.0.0.1", "port": 9002, "tpm_policy": "{\"22\": [\"0000000000000000000000000000000000000001\", \"0000000000000000000000000000000000000000000000000000000000000001\", \"000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001\", \"ffffffffffffffffffffffffffffffffffffffff\", \"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff\", \"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff\"], \"15\": [\"0000000000000000000000000000000000000000\", \"0000000000000000000000000000000000000000000000000000000000000000\", \"000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\"], \"mask\": \"0x408400\"}", "vtpm_policy": "{\"23\": [\"ffffffffffffffffffffffffffffffffffffffff\", \"0000000000000000000000000000000000000000\"], \"15\": [\"0000000000000000000000000000000000000000\"], \"mask\": \"0x808000\"}", "meta_data": "{}", "allowlist_len": 6, "mb_refstate_len": 0, "accept_tpm_hash_algs": ["sha512", "sha384", "sha256", "sha1"], "accept_tpm_encryption_algs": ["ecc", "rsa"], "accept_tpm_signing_algs": ["ecschnorr", "rsassa"], "hash_alg": "sha1", "enc_alg": "rsa", "sign_alg": "rsassa", "verifier_id": "default", "verifier_ip": "127.0.0.1", "verifier_port": 8881, "severity_level": null, "last_event_id": null}}
:: [ 10:34:42 ] :: [   PASS   ] :: Command 'lime_keylime_tenant -c cvstatus' (Expected 0, got 0)
:: [ 10:34:42 ] :: [   FAIL   ] :: File '/var/tmp/rlRun_LOG.gSGAYMcd' should contain 'Verifier at 127.0.0.1 with Port 8881 does not have agent d432fbb3-d2f1-4a97-9ef7-75bd81c00000' 
::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::
::   Duration: 7s
::   Assertions: 2 good, 2 bad
::   RESULT: FAIL (-c delete)

All other tests are passing. I prefer to ignore this test failure but something is flaky...

mpeters · 2022-03-23T16:43:17Z

/packit test

stefanberger · 2022-03-24T20:10:32Z

I wonder what that is when Fedora 34 fails and all other ones pass...

:: [ 19:42:43 ] :: [   FAIL   ] :: Command 'rlWaitForCmd 'tail /var/tmp/limeLib/agent.log | grep -q "Executing revocation action"' -m 10 -d 1 -t 10' (Expected 0, got 1)

THS-on · 2022-03-24T20:28:43Z

I wonder what that is when Fedora 34 fails and all other ones pass...

@kkaarreell seems this like a timing issue?

@stefanberger just use /packit test to rerun the tests.

stefanberger · 2022-03-24T20:41:45Z

/packit test

kkaarreell · 2022-03-24T20:55:37Z

I wonder what that is when Fedora 34 fails and all other ones pass...

@kkaarreell seems this like a timing issue?

I can increase the timeout... so far 10 seconds for revocation action initiation was enough. What should be the reasonable timeout?

THS-on · 2022-03-24T20:58:58Z

I can increase the timeout... so far 10 seconds for revocation action initiation was enough. What should be the reasonable timeout?

10 seconds seem actually reasonable, but if it flaky again in the future we need to take a closer look what is causing this.

kkaarreell · 2022-03-24T21:02:27Z

Maybe some networking glitch on the actual virtual system... That would explain the above failure in the keylime tenant.
Having logs with timestamps would help. I will try to find a way how to incorporate more logs into the test output when test have failed.

kkaarreell · 2022-03-24T21:40:35Z

I will try to find a way how to incorporate more logs into the test output when test have failed.

Hopefully this would help next time
RedHat-SP-Security/keylime-tests#70

Fix the following issue detected by pylint 2.13.0: keylime/cmd/ima_emulator_adapter.py:73:12: E4702: Iterated dict 'position' is being modified inside for loop body, iterate through a copy of it instead. (modified-iterating-dict) Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

Fix the following issue detected by pylint 2.13.0: keylime/tenant.py:1146:19: E0601: Using variable 'response' before assignment (used-before-assignment) Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

keylime-bot requested review from jetwhiz, THS-on and keylime-bot March 22, 2022 22:28

keylime-bot assigned THS-on, jetwhiz and keylime-bot Mar 22, 2022

stefanberger force-pushed the stefanberger/fix-w1732 branch 2 times, most recently from b786915 to 0be94fd Compare March 22, 2022 23:46

THS-on reviewed Mar 23, 2022

View reviewed changes

THS-on mentioned this pull request Mar 23, 2022

Code quality improvements #929

Open

39 tasks

stefanberger changed the title ~~pylint: Fix W1732 consider-using-with~~ pylint: Fix R1732 consider-using-with Mar 23, 2022

stefanberger force-pushed the stefanberger/fix-w1732 branch from 0be94fd to 447ef9f Compare March 23, 2022 10:28

stefanberger force-pushed the stefanberger/fix-w1732 branch 4 times, most recently from 4201c74 to cfa277e Compare March 23, 2022 14:33

stefanberger force-pushed the stefanberger/fix-w1732 branch from 2f92115 to 5004ea2 Compare March 23, 2022 19:37

stefanberger force-pushed the stefanberger/fix-w1732 branch 2 times, most recently from 5ea7dd4 to b7903be Compare March 25, 2022 20:25

pylint: Fix issue detected by pylint-2.13.0

de38418

Fix the following issue detected by pylint 2.13.0: keylime/tenant.py:1146:19: E0601: Using variable 'response' before assignment (used-before-assignment) Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

stefanberger force-pushed the stefanberger/fix-w1732 branch from b7903be to 98dc470 Compare March 25, 2022 20:27

stefanberger added 2 commits March 25, 2022 16:35

pylint: Fix R1732 consider-using-with

1fe02b3

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

pylint: Fix remaining code with issue R1732 consider-using-with

5adcbc9

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

stefanberger force-pushed the stefanberger/fix-w1732 branch from 98dc470 to 5adcbc9 Compare March 25, 2022 20:35

stefanberger mentioned this pull request Mar 25, 2022

Implement broader key type support for Keylime's signing mechanisms #942

Merged

mpeters approved these changes Mar 29, 2022

View reviewed changes

mpeters merged commit e7dcad7 into keylime:master Mar 29, 2022

stefanberger deleted the stefanberger/fix-w1732 branch March 29, 2022 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pylint: Fix R1732 consider-using-with #937

pylint: Fix R1732 consider-using-with #937

stefanberger commented Mar 22, 2022

THS-on left a comment

THS-on Mar 23, 2022

stefanberger Mar 23, 2022

THS-on Mar 23, 2022

stefanberger Mar 23, 2022

THS-on Mar 23, 2022

stefanberger Mar 23, 2022

stefanberger Mar 23, 2022

THS-on Mar 23, 2022

THS-on Mar 23, 2022

stefanberger Mar 23, 2022

THS-on commented Mar 23, 2022

stefanberger commented Mar 23, 2022

mpeters commented Mar 23, 2022

stefanberger commented Mar 24, 2022

THS-on commented Mar 24, 2022

stefanberger commented Mar 24, 2022

kkaarreell commented Mar 24, 2022

THS-on commented Mar 24, 2022

kkaarreell commented Mar 24, 2022

kkaarreell commented Mar 24, 2022

pylint: Fix R1732 consider-using-with #937

pylint: Fix R1732 consider-using-with #937

Conversation

stefanberger commented Mar 22, 2022

THS-on left a 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

Choose a reason for hiding this comment

THS-on commented Mar 23, 2022

stefanberger commented Mar 23, 2022

mpeters commented Mar 23, 2022

stefanberger commented Mar 24, 2022

THS-on commented Mar 24, 2022

stefanberger commented Mar 24, 2022

kkaarreell commented Mar 24, 2022

THS-on commented Mar 24, 2022

kkaarreell commented Mar 24, 2022

kkaarreell commented Mar 24, 2022