Refactor async code synchronization #499

ansasaki · 2023-01-13T15:40:24Z

Use messages for synchronization instead of mutexes and condvar. Add
separate workers for keys, payload, and revocation execution, as well as
a worker to handle the revocations coming from ZeroMQ channel.

The workers hold the state locally instead of storing all data in server
context (QuoteData).

The keys worker holds the received U and K keys and is responsible for
combining them to obtain K. It also provides the K key when requested
by the /keys/verify endpoint to prove that the resulting K is correct.

When the U and V keys combination is successful (the HMAC verification
is successful), the keys worker wakes the payloads worker sending the
obtained K key and the encrypted payload.

The payloads worker is responsible for decrypting and running the
payload sent by the keys worker.

After decrypting and running the payload, the payloads worker will send
messages to the revocation worker and ZMQ worker to start listening for
revocations.

This also implements graceful shutdown on Ctrl + C (SIGINT) signal.
A dedicated task is spawned to receive the signal and stop the other workers.

Unit tests were implemented for the changes.

Fixes: #363, #447

keylime-agent/src/common.rs

keylime-agent/src/payloads.rs

keylime-agent/src/common.rs

keylime-agent/src/keys_handler.rs

ansasaki · 2023-01-19T09:24:30Z

It seems the change I introduced in e3ebc06 makes tarpaulin to hang (see xd009642/tarpaulin#1056).

I'll revert it for now.

ansasaki · 2023-01-26T09:48:46Z

@ueno @aplanas @lkatalin Could you please review this?

aplanas · 2023-01-30T13:52:53Z

It seems the change I introduced in e3ebc06 makes tarpaulin to hang (see xd009642/tarpaulin#1056).

I'll revert it for now.

I still see the error?

Jan 30 13:45:40.153 ERROR cargo_tarpaulin: Failed to get test coverage! Error: Failed to run tests: Error: Timed out waiting for test response

ansasaki · 2023-01-30T14:02:38Z

It seems the change I introduced in e3ebc06 makes tarpaulin to hang (see xd009642/tarpaulin#1056).
I'll revert it for now.

I still see the error?
Jan 30 13:45:40.153 ERROR cargo_tarpaulin: Failed to get test coverage! Error: Failed to run tests: Error: Timed out waiting for test response

It seems this is back (sigh). It is difficult to debug this as I can't reproduce locally... I will investigate

keylime-agent/src/common.rs

keylime-agent/src/revocation.rs

ansasaki · 2023-01-31T11:38:31Z

@aplanas I did not find the root cause that makes tarpaulin to hang. It definitely is something related with the instrumentation for test coverage, as it works fine when running the tests without it. There are multiple related issues open upstream on tarpaulin repo.

The only thing I could do was to update the image used for testing to a newer version (from Fedora 34 to Fedora 37).

If the issue makes the tests to fail frequently, let's disable the tests that runs external binaries or move the coverage measurement to a separate job which is not required to pass.

aplanas

For what I see LGTM

ansasaki · 2023-02-01T16:17:46Z

@ueno @aplanas @lkatalin Could you please review/approve this?

aplanas · 2023-02-02T13:01:29Z

@ueno @aplanas @lkatalin Could you please review/approve this?

AFAICT LGTM, yes

ansasaki · 2023-02-02T13:08:59Z

@THS-on Could you please review/approve this?

THS-on

General LGTM, but have not run it locally.
I have not worked with much Rust sync code, so I cannot really comment on that part.

keylime-agent/src/common.rs

keylime-agent/src/main.rs

ansasaki · 2023-02-08T09:35:00Z

@ashcrow @mbestavros @lukehinds @lkatalin @puiterwijk @mpeters @ueno

Could someone please review/approve this?

I believe we need to review/expand the reviewers who can approve changes to get merged. I'm trying to get this merged for weeks, but cannot get the required approval.

Also I would like to make changes to the repository (e.g. creating a template for issues, creating a template for PR checklist) but I do not have the required permission. Could someone with the right permissions please contact me?

Separate the ZeroMQ service from the worker task. Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>

ashcrow · 2023-02-08T17:21:57Z

I'm happy to approve given I see 3 approvals already and all outstanding comments have been resolved. My only caveat is that the Fedora tests failed. If this is a known issue then I'll add my approval, if not, let's get that fixed first 🙏

@lkatalin Is there a discussion already on going about approvers by chance?

Add the payloads module which contains all payload-related functions. Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>

Use messages for synchronization instead of mutexes and condvar. Add separate workers for keys and payload execution, holding the state locally instead of storing all data in server context (QuoteData). The keys worker holds the received U and K keys and is responsible for combining them to obtain K. It also provides the K key when requested by the /keys/verify endpoint to prove that the resulting K is correct. When the U and V keys combination is successful (the HMAC verification is successful), the keys worker wakes the payloads worker sending the obtained K key and the encrypted payload. The payloads worker is responsible for decrypting and running the payload sent by the keys worker. Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>

Use a separate task to handle the SIGINT (Ctrl + C) signal and wait for all the tasks to finish. Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>

Modify the ZeroMQ listener to poll the socket for events before trying to read instead of blocking. Create a revocation worker which is reponsible for running the revocation payloads. The payloads worker will send a message to the revocation worker after decrypting the payload which may contain the revocation certificate. The payloads worker will also send a message to the ZeroMQ worker to start listening for revocation notifications. All revocation messages received through both ZeroMQ and Rest API are sent to the revocation worker which will process them one by one. This is to avoid running revocation scripts concurrently, which may conflict. Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>

Cleanup U and V keys processing and add tests for processing with bogus keys in the vectors of received keys. Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>

ansasaki · 2023-02-08T18:02:32Z

I'm happy to approve given I see 3 approvals already and all outstanding comments have been resolved. My only caveat is that the Fedora tests failed. If this is a known issue then I'll add my approval, if not, let's get that fixed first pray

@ashcrow Done!

ashcrow

Review based on previous reviewers.

keylime-bot requested review from lukehinds, ashcrow and THS-on January 13, 2023 15:40

keylime-bot assigned THS-on, lukehinds and ashcrow Jan 13, 2023

ueno reviewed Jan 15, 2023

View reviewed changes

keylime-agent/src/common.rs Outdated Show resolved Hide resolved

keylime-agent/src/common.rs Outdated Show resolved Hide resolved

keylime-agent/src/payloads.rs Outdated Show resolved Hide resolved

keylime-agent/src/payloads.rs Outdated Show resolved Hide resolved

aplanas reviewed Jan 16, 2023

View reviewed changes

ansasaki force-pushed the async_refactor branch 3 times, most recently from c7ba7ef to 25df334 Compare January 18, 2023 13:10

ansasaki changed the title ~~WIP: Refactor async code synchronization~~ Refactor async code synchronization Jan 18, 2023

ansasaki force-pushed the async_refactor branch from 25df334 to 496c1a8 Compare January 18, 2023 18:24

ansasaki force-pushed the async_refactor branch from 496c1a8 to 504b763 Compare January 19, 2023 09:28

ansasaki mentioned this pull request Jan 19, 2023

Prevent HMAC mismatch error from disabling rust agent. #500

Closed

ansasaki force-pushed the async_refactor branch 2 times, most recently from a5f1ba9 to d515ce2 Compare January 20, 2023 10:00

ansasaki force-pushed the async_refactor branch from d515ce2 to 4ac3b8c Compare January 30, 2023 13:32

aplanas approved these changes Jan 30, 2023

View reviewed changes

keylime-agent/src/common.rs Outdated Show resolved Hide resolved

keylime-agent/src/revocation.rs Outdated Show resolved Hide resolved

ansasaki force-pushed the async_refactor branch 4 times, most recently from cda679d to c9f2aa4 Compare January 31, 2023 10:58

ansasaki force-pushed the async_refactor branch 2 times, most recently from 954cd7d to 7f8b26b Compare February 1, 2023 11:59

aplanas approved these changes Feb 1, 2023

View reviewed changes

sergio-correia approved these changes Feb 2, 2023

View reviewed changes

THS-on approved these changes Feb 2, 2023

View reviewed changes

keylime-agent/src/common.rs Show resolved Hide resolved

keylime-agent/src/main.rs Show resolved Hide resolved

keylime-bot unassigned THS-on Feb 2, 2023

ansasaki added bug Something isn't working enhancement labels Feb 8, 2023

main: Run ZeroMQ service in a separate task

b333a4d

Separate the ZeroMQ service from the worker task. Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>

ansasaki force-pushed the async_refactor branch 2 times, most recently from 1f7da3d to 035b378 Compare February 8, 2023 17:11

ansasaki added 5 commits February 8, 2023 18:23

main: Move payload related functions to payloads module

1f30b4f

Add the payloads module which contains all payload-related functions. Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>

keylime-agent: Gracefully shutdown on SIGINT

f06e972

Use a separate task to handle the SIGINT (Ctrl + C) signal and wait for all the tasks to finish. Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>

Store associated U keys, auth tags, and payloads together

6b37e82

Cleanup U and V keys processing and add tests for processing with bogus keys in the vectors of received keys. Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>

ansasaki force-pushed the async_refactor branch from 035b378 to 6b37e82 Compare February 8, 2023 17:25

ashcrow approved these changes Feb 8, 2023

View reviewed changes

ashcrow merged commit 4104f5b into keylime:master Feb 8, 2023

ansasaki deleted the async_refactor branch February 9, 2023 08:53

ansasaki mentioned this pull request Feb 21, 2023

Agent do not reply with JSON for some endpoints #447

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor async code synchronization #499

Refactor async code synchronization #499

ansasaki commented Jan 13, 2023 •

edited

Loading

ansasaki commented Jan 19, 2023

ansasaki commented Jan 26, 2023

aplanas commented Jan 30, 2023

ansasaki commented Jan 30, 2023

ansasaki commented Jan 31, 2023

aplanas left a comment

ansasaki commented Feb 1, 2023 •

edited

Loading

aplanas commented Feb 2, 2023

ansasaki commented Feb 2, 2023

THS-on left a comment

ansasaki commented Feb 8, 2023

ashcrow commented Feb 8, 2023

ansasaki commented Feb 8, 2023 •

edited

Loading

ashcrow left a comment

Refactor async code synchronization #499

Refactor async code synchronization #499

Conversation

ansasaki commented Jan 13, 2023 • edited Loading

ansasaki commented Jan 19, 2023

ansasaki commented Jan 26, 2023

aplanas commented Jan 30, 2023

ansasaki commented Jan 30, 2023

ansasaki commented Jan 31, 2023

aplanas left a comment

Choose a reason for hiding this comment

ansasaki commented Feb 1, 2023 • edited Loading

aplanas commented Feb 2, 2023

ansasaki commented Feb 2, 2023

THS-on left a comment

Choose a reason for hiding this comment

ansasaki commented Feb 8, 2023

ashcrow commented Feb 8, 2023

ansasaki commented Feb 8, 2023 • edited Loading

ashcrow left a comment

Choose a reason for hiding this comment

ansasaki commented Jan 13, 2023 •

edited

Loading

ansasaki commented Feb 1, 2023 •

edited

Loading

ansasaki commented Feb 8, 2023 •

edited

Loading