Call revocation actions #159

lkatalin · 2021-02-10T22:31:37Z

This code runs those revocation scripts which were sent by the tenant and can be accessed from the secure mount point post-attestation.

I've decided not to check whether the revocation scripts begin with local_action, since they are all listed in action_list anyhow. But if someone has a reason that they should be marked that way, I can add this check back again.

I had a few other thoughts and questions inline marked with TODO and would love some suggestions on those points. The most important being: If these scripts don't run for whatever reason (ex. the Python version was too old), is that detected in some other part of the code? It seems like we would want a guarantee that a node can't continue to be in the cluster if it doesn't run the scripts, since it could still be in communication with the compromised node, for example.

ashcrow

Looks like great progress!! 😄

src/revocation.rs

lukehinds

This is looking good to me. It would be nice to have some tests, just trying to think how we can do that. Not sure if we need to mock or can rely on the CI containers having a py env.

lkatalin · 2021-02-17T21:34:46Z

This is looking good to me. It would be nice to have some tests, just trying to think how we can do that. Not sure if we need to mock or can rely on the CI containers having a py env.

@lukehinds This is a good point and I'm adding some unit tests. ~~They pass locally, but as you pointed out, there is a problem with not having Python in the CI container. I'm going to try to fix this in a separate PR because it is proving to be tricky.~~
Edit: not sure what was going on earlier, but it looks like it works now.

src/revocation.rs

src/main.rs

lkatalin · 2021-02-18T23:17:03Z

@ashcrow @lukehinds I will have to figure out this CI failure tomorrow (not able to replicated it locally) and might add some more unit tests, but was wondering if you have other comments on how it works currently. Thanks.

lkatalin · 2021-02-20T01:06:41Z

Tests passing again!

src/revocation.rs

Signed-off-by: Lily Sturmann <[email protected]>

ashcrow · 2021-02-22T22:27:33Z

~~Let us know once you're ready for a re-review!!~~ Never mind -- I see it now 😄

ashcrow

👍 😄

ashcrow · 2021-02-22T22:32:09Z

src/revocation.rs

+
+    let mut outputs = Vec::new();
+
+    if Path::new(&action_file).exists() {


future: It may make sense to load this once rather than when we run revocations. However, I believe this is fine and may be what we want long term depending on the expectations of the users.

lukehinds · 2021-02-23T07:02:41Z

looks good to me , @lkatalin you can self merge if you like (just in case you have further tweaks).

keylime-bot requested review from puiterwijk, ashcrow and lukehinds February 10, 2021 22:31

lkatalin force-pushed the revactions branch 2 times, most recently from 2a0879c to f7e7180 Compare February 10, 2021 22:49

keylime-bot assigned lukehinds, puiterwijk and ashcrow Feb 10, 2021

lkatalin force-pushed the revactions branch 3 times, most recently from 7b55ede to 2693f38 Compare February 12, 2021 23:23

lkatalin marked this pull request as ready for review February 12, 2021 23:33

ashcrow reviewed Feb 16, 2021

View reviewed changes

src/revocation.rs Outdated Show resolved Hide resolved

src/revocation.rs Show resolved Hide resolved

src/revocation.rs Outdated Show resolved Hide resolved

lukehinds reviewed Feb 17, 2021

View reviewed changes

lkatalin force-pushed the revactions branch 5 times, most recently from 4e55d27 to 1067596 Compare February 17, 2021 21:33

lkatalin commented Feb 17, 2021

View reviewed changes

src/revocation.rs Outdated Show resolved Hide resolved

lkatalin requested a review from ashcrow February 17, 2021 21:53

ashcrow reviewed Feb 17, 2021

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

lkatalin force-pushed the revactions branch from 1067596 to 957b6d2 Compare February 18, 2021 22:33

lkatalin requested review from lukehinds and ashcrow February 18, 2021 23:17

lkatalin force-pushed the revactions branch 3 times, most recently from d1cfb6a to 7725ccd Compare February 19, 2021 23:30

lkatalin force-pushed the revactions branch 3 times, most recently from d7ceb56 to 4b33cc8 Compare February 20, 2021 00:58

ashcrow reviewed Feb 22, 2021

View reviewed changes

src/revocation.rs Outdated Show resolved Hide resolved

lkatalin force-pushed the revactions branch 2 times, most recently from 8e0eeb9 to af04103 Compare February 22, 2021 18:47

ashcrow reviewed Feb 22, 2021

View reviewed changes

src/revocation.rs Outdated Show resolved Hide resolved

lkatalin force-pushed the revactions branch 2 times, most recently from 0ad6fe3 to 5e029f5 Compare February 22, 2021 21:16

Call revocation actions

7df05e3

Signed-off-by: Lily Sturmann <[email protected]>

lkatalin force-pushed the revactions branch from 5e029f5 to 7df05e3 Compare February 22, 2021 22:03

lkatalin requested a review from ashcrow February 22, 2021 22:17

ashcrow approved these changes Feb 22, 2021

View reviewed changes

lukehinds approved these changes Feb 23, 2021

View reviewed changes

lkatalin merged commit 57c39a5 into keylime:master Feb 23, 2021

lkatalin mentioned this pull request Feb 23, 2021

Implement Revocation Actions #96

Closed

lkatalin deleted the revactions branch July 16, 2021 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call revocation actions #159

Call revocation actions #159

lkatalin commented Feb 10, 2021 •

edited

Loading

ashcrow left a comment

lukehinds left a comment

lkatalin commented Feb 17, 2021 •

edited

Loading

lkatalin commented Feb 18, 2021

lkatalin commented Feb 20, 2021

ashcrow commented Feb 22, 2021 •

edited

Loading

ashcrow left a comment

ashcrow Feb 22, 2021

lukehinds commented Feb 23, 2021


		let mut outputs = Vec::new();

		if Path::new(&action_file).exists() {

Call revocation actions #159

Call revocation actions #159

Conversation

lkatalin commented Feb 10, 2021 • edited Loading

ashcrow left a comment

Choose a reason for hiding this comment

lukehinds left a comment

Choose a reason for hiding this comment

lkatalin commented Feb 17, 2021 • edited Loading

lkatalin commented Feb 18, 2021

lkatalin commented Feb 20, 2021

ashcrow commented Feb 22, 2021 • edited Loading

ashcrow left a comment

Choose a reason for hiding this comment

ashcrow Feb 22, 2021

Choose a reason for hiding this comment

lukehinds commented Feb 23, 2021

lkatalin commented Feb 10, 2021 •

edited

Loading

lkatalin commented Feb 17, 2021 •

edited

Loading

ashcrow commented Feb 22, 2021 •

edited

Loading