chore: greater protection for release mode #72

rjzak · 2022-10-10T19:47:28Z

Self-signed certificate not permitted in release mode
Generating a certificate is not permitted in release mode

References #49

Signed-off-by: Richard Zak richard@profian.com

rvolosatovs

I don't think that solution to "having too much code" can be adding more code.

Why don't we just remove the testing code from our critical production service instead?

If we need this functionality for unit tests, then let's define clear boundaries in our internal API such that testing would be possible without modifying the implementation and be fully self-contained within the test module?

In any case, tests utilizing testing code don't test anything, those paths will never be taken in production setting, since those pieces of code will never be compiled in release mode. Note, that a mistake here could be very costly, while also being pretty easy to make. Tests must rely on "real" implementations we run in production as much as possible.

rvolosatovs · 2022-10-10T21:56:36Z

src/ext/mod.rs

@@ -28,5 +28,5 @@ pub trait ExtVerifier {
    /// certificate. Returning `Ok(false)` will allow the certification request
    /// to continue, but this particular extension will not be included
    /// in the resulting certificate.
-    fn verify(&self, cri: &CertReqInfo<'_>, ext: &Extension<'_>, dbg: bool) -> Result<bool>;
+    fn verify(&self, cri: &CertReqInfo<'_>, ext: &Extension<'_>, dbg: &bool) -> Result<bool>;


bool is Copy and small enough that & does not bring any benefits

I did that to address complaints about use of a moved value.

strange, it just compiles fine without it

I think my IDE was having problems. I reverted back to bool and it's fine.

rvolosatovs · 2022-10-10T21:58:13Z

src/main.rs

@@ -80,6 +85,7 @@ struct Args {
    #[clap(short, long, env = "ROCKET_ADDRESS", default_value = "::")]
    addr: IpAddr,

+    #[cfg(debug_assertions)]
    #[clap(short, long, env = "RENDER_EXTERNAL_HOSTNAME")]


Especially looking at the env var name, this parameter does not seem relevant to the Steward at it's present form

It's used to generate keys, which we wouldn't do in a production environment.

Additionally, some tests were disabled for release mode (since Nix seems to test in release mode) due to this discovered bug #73

rjzak · 2022-10-10T22:56:40Z

I don't think that solution to "having too much code" can be adding more code.

I don't think I added that much code. I disabled code when compiling for release.

Why don't we just remove the testing code from our critical production service instead?

We need to have some testing (debug mode) to ensure things work in the future, since not everyone has SGX or SNP machines. The very reason for the nil and kvm backends in Enarx.

If we need this functionality for unit tests, then let's define clear boundaries in our internal API such that testing would be possible without modifying the implementation and be fully self-contained within the test module?

Sure, I can move the logic for generate() to the test side. But I still think having some debug functionally as a stand-alone executable beyond unit tests is helpful. The only part would be to ensure not using self-signed certs when compiled for release.

In any case, tests utilizing testing code don't test anything, those paths will never be taken in production setting, since those pieces of code will never be compiled in release mode. Note, that a mistake here could be very costly, while also being pretty easy to make. Tests must rely on "real" implementations we run in production as much as possible.

Again, the tests ensure the debug parts work, when utilized for actual testing. Better attestation reports and public keys can be serialized to ensure better testing #73 to fully exercise the release code.

rvolosatovs

As discussed a few times already, deferring review to @npmccallum
I personally think that we should just remove KVM attestation and use real, captured SGX and SEV reports in tests.
If we really want to have KVM attestation available, then that should be guarded by insecure feature (as is already done) and --insecure runtime flag. Still, I am not happy by how much the implementation is changed to account for this. I find that the prevalent cfg attributes make the code harder to reason about and maintain. What's worse, it introduces plethora of new code paths, that actual, real, production service will never take and results in "real" code that should be tested to not get tested.

If we really want to have KVM attestation available, then I think we need to restructure the code in such a way that this feature would not be as intrusive and such that the unit tests would still test "real" code paths and not code paths, which would never be taken in production.

rvolosatovs · 2022-10-19T13:45:12Z

src/ext/kvm.rs

-            return Err(anyhow!("steward not in debug mode"));
-        }
+        #[cfg(not(feature = "insecure"))]
+        return Err(anyhow!("steward not in debug mode"));


Suggested change

return Err(anyhow!("steward not in debug mode"));

bail!("steward not in insecure mode")

Captured SEV & SGX reports added yesterday.

Signed-off-by: Richard Zak <richard@profian.com>

rjzak requested review from a team and bstrie as code owners October 10, 2022 19:47

rjzak requested a review from rvolosatovs October 10, 2022 19:48

rvolosatovs requested changes Oct 10, 2022

View reviewed changes

rjzak force-pushed the debug_vs_release_checks branch from 7778951 to 2be891a Compare October 10, 2022 22:29

rjzak requested a review from haraldh as a code owner October 10, 2022 22:29

rjzak force-pushed the debug_vs_release_checks branch 3 times, most recently from cc1a40b to 64d7434 Compare October 11, 2022 16:45

rvolosatovs reviewed Oct 19, 2022

View reviewed changes

rjzak force-pushed the debug_vs_release_checks branch from ea1ce69 to 33c9a9e Compare October 26, 2022 15:40

rjzak force-pushed the debug_vs_release_checks branch 2 times, most recently from c4a191a to 1954681 Compare November 7, 2022 20:56

rjzak mentioned this pull request Dec 4, 2022

Refactor Unit Tests #134

Open

chore: greater protection for release mode

c2fcc09

Signed-off-by: Richard Zak <richard@profian.com>

rjzak force-pushed the debug_vs_release_checks branch from 1954681 to 0a3cb7d Compare December 7, 2022 23:11

rjzak added 3 commits December 7, 2022 18:15

chore: insecure mode feature flag

675a026

Signed-off-by: Richard Zak <richard@profian.com>

Adding canned CSRs, tests

ad5b1ea

Signed-off-by: Richard Zak <richard@profian.com>

feat: add csr from signed Enarx

ab44039

Signed-off-by: Richard Zak <richard@profian.com>

rjzak force-pushed the debug_vs_release_checks branch from 0a3cb7d to ab44039 Compare December 7, 2022 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: greater protection for release mode #72

chore: greater protection for release mode #72

rjzak commented Oct 10, 2022

rvolosatovs left a comment •

edited

rvolosatovs Oct 10, 2022

rjzak Oct 10, 2022

haraldh Oct 11, 2022

rjzak Oct 11, 2022

rvolosatovs Oct 10, 2022

rjzak Oct 10, 2022

rjzak Oct 10, 2022

rjzak commented Oct 10, 2022

rvolosatovs left a comment

rvolosatovs Oct 19, 2022

rjzak Oct 19, 2022

	return Err(anyhow!("steward not in debug mode"));
	bail!("steward not in insecure mode")

chore: greater protection for release mode #72

Are you sure you want to change the base?

chore: greater protection for release mode #72

Conversation

rjzak commented Oct 10, 2022

rvolosatovs left a comment • edited

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

rjzak commented Oct 10, 2022

rvolosatovs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvolosatovs left a comment •

edited