Remove signing key and conf from tests. Instead use OE_SET_ENCLAVE_SGX macro. #632
Conversation
CodeMonkeyLeet
added
the
engineering
| // In C++, by default const objects have internal linkage, causing the | ||
| // OE_EXPORT (visibility) to be ignored unless also qualified with | ||
| // extern | ||
| #define OE_EXPORT_CONST OE_EXPORT extern const |
There was a problem hiding this comment.
Maybe move this to bits/defs.h?
There was a problem hiding this comment.
Good point. Will move.
| @@ -7,4 +7,4 @@ if (UNIX) | |||
| add_subdirectory(enc) | |||
| endif() | |||
|
|
|||
| add_enclave_test(tests/echo ./host echo_host ./enc echo_enc.signed.so) | |||
There was a problem hiding this comment.
Maybe add a test called oesign that has both the .pem and .conf files (but no macro).
There was a problem hiding this comment.
And maybe a test called "configure" to test creation of an enclave that uses configuration file and signing.
There was a problem hiding this comment.
@mikbras These tests already exist:
- The new tests/bigmalloc only uses the signed form
- The tests/props test enclave creation and the interaction of properties from config file with runtime settings/build time settings.
There was a problem hiding this comment.
@mikbras as Simon remaked, oesign functionality is covered by props.
- Should we rename tests/props to tests/oesign instead? I can also add a basic oesign test if that makese more sense.
- Can you look at tests/load? It seems to be doing nothing. Should it be deleted?
There was a problem hiding this comment.
@anakrish I would not rename test/props to tests/oesign (I would reserve that name for tests explicitly testing various aspects of oesign on the command line (e.g. negative tests for input args etc.)
There was a problem hiding this comment.
@CodeMonkeyLeet, @mikbras
Fixed. Added basic tests/oesign that creates an enclave using sign.conf and private.pem, and checks conf values. We can later expand this for oesign tool negative testing.
There was a problem hiding this comment.
Anand, did you check this in yet? I don't see it in the anakrish.remove_signingkey branch.
There was a problem hiding this comment.
@CodeMonkeyLeet: props does not adequately test the scenario I am concerned about. We need a test that uses the .conf + .pem files but not the OE_SET_SGX_* macro. Else, how will we know it is picking up the correct settings from the .conf file and not the macro?
There was a problem hiding this comment.
@mikbras As previously mentioned, tests/bigmalloc uses only the signed form without the SGX macro, and it's dependent on the properties being correctly interpreted for the large allocation testing.
There was a problem hiding this comment.
Looks good, thanks for doing this! A couple of additional comments:
- I would remove tests/load entirely. It looks like an early debugging tool and it takes a cross folder dependency on tests/echo for its enclave.
- Can you also remove the *.conf and *.pem files currently in the tools/oesign folder? They really shouldn't be there (not sure why the log.txt is there either under /files)
|
@CodeMonkeyLeet Good catch. I have removed the files folder and conf from tools/oesign. |
tests/oesign/host/host.c
Outdated
| #include <openenclave/internal/tests.h> | ||
| #include <stdio.h> | ||
|
|
||
| static void _CheckProperties( |
There was a problem hiding this comment.
I would recommend not adding placeholders. It doesn't look like it's providing any additional code coverage relative to tests/props, and this is not a generalized framework for oesign testing (i.e. we hardcode the validation values based on the single sign.conf). Consider taking this out entirely until we can add a data driven framework to test and validate each of the command line arguments, and also various formatting quirks of the sign.conf files themselves.
|
@mikbras @CodeMonkeyLeet Can we come to a conclusion re whether we need to add tests/oesign as part of this PR or not? |
|
Failures only in Kabylake since that system is down. Merging so that I can move on to next task. |
|
This addresses #465 |
…X macro. (#632) * Remove sign.conf and private.pem from tests. * Fix test. * Address PR feedback. * Remove unused feedback. * Fix missing braces warning. * Remove oseign tests based on PR feedback.
1.tests/load: This does not seem to be doing anything. No add_test is made.
2.tests/props: Retained so that property merging scenario is tested.
3. tests/debug-mode: Retained since it is testing debug property.