-
Notifications
You must be signed in to change notification settings - Fork 551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add TPM2 RNG #4117
base: master
Are you sure you want to change the base?
Add TPM2 RNG #4117
Conversation
Co-Authored-By: René Meusel <[email protected]>
Co-Authored-By: René Meusel <[email protected]>
Co-Authored-By: René Meusel <[email protected]>
Co-Authored-By: René Meusel <[email protected]>
src/lib/prov/tpm2/tpm2_rng.cpp
Outdated
BOTAN_ASSERT_NOMSG(digest->size == requested_bytes); | ||
out.append({digest->buffer, digest->size}); | ||
|
||
Esys_Free(digest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a good idea to use scoped_cleanup
(or some variant of it) for this. Its unlikely, but if either the assert or the append above throw, the digest
won't be freed.
src/python/botan3.py
Outdated
# | ||
class TPM2Context: | ||
def __init__(self, tcti_nameconf: str = None) -> None: | ||
tcti = c_char_p(0 if not tcti_nameconf else tcti_nameconf.encode("utf-8")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other locations (for instance the constructor of RandomNumberGenerator
) use a _ctype_str()
helper to transform a string object into a (presumable) const char*
. Perhaps we should use that here as well? Let's look into it once more...
|
||
self.__obj = c_void_p(0) | ||
rc = _DLL.botan_tpm2_ctx_init(byref(self.__obj), tcti) | ||
if rc == -40: # 'Not Implemented' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance we could pull this magic number from the enum? 😅
(there are other places that would benefit from it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, though it seems like this is hard to do since we can't access the enum via CDLL. The other options of manually keeping the same enum in Python or providing functions that return respective enum values don't seem particularly worthwhile to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, there seems to be some secret sauce around the translation of parameter/result types from C to Python and back. This blog post provides a little inspiration: https://v4.chriskrycho.com/2015/ctypes-structures-and-dll-exports.html
Perhaps we could actually mirror the error-enum into botan3.py
. That's a manual duplication, but should be okay, given that this enum probably doesn't change all the time. And then we could make CDLL do the translation for us.
Any of this should certainly not go into this pull request, though.
} | ||
}(); | ||
|
||
ctx->ctx = Botan::TPM2_Context::create(std::move(tcti)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an exception is throwing here due to for instance no hardware being available at runtime, the caller will get BOTAN_FFI_ERROR_EXCEPTION_THROWN
which is not particularly helpful. Maybe catch this and then translate an error to something more specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe even just reusing BOTAN_FFI_ERROR_NOT_IMPLEMENTED
if the hardware is unavailable? IDK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I don't think reusing BOTAN_FFI_ERROR_NOT_IMPLEMENTED
is all that helpful since the function would be implemented and TPM2 usage desired in that case (as the build is configured with --with-tpm2
).
The exception that would be thrown here is TPM2_Error
with error type ErrorType::TPMError
. I added a corresponding FFI error type such that the caller will now get that error translated within ffi_guard_thunk
instead of BOTAN_FFI_ERROR_EXCEPTION_THROWN
. That way, one can distinguish between a runtime TPM error when configured with TPM and a not implemented error when TPM is not configured.
src/lib/prov/tpm2/tpm2.h
Outdated
|
||
#include <tss2/tss2_esys.h> | ||
#include <tss2/tss2_rc.h> | ||
#include <tss2/tss2_tctildr.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing that most/all uses of TPM will just have a shared_ptr<TPM2_Context>
that they invoke various functions from the library with. That being the case, if we could hide these few uses of the TPM2 types in this header, we likely could avoid pulling the TPM library includes into anything a user might include.
Thanks for the comments! I applied review suggestions except the parts that require re-structuring |
@@ -177,6 +177,10 @@ The following enum values are defined in the FFI header: | |||
calling :cpp:func:`botan_hash_destroy` on a ``botan_rng_t`` object will cause | |||
this error. | |||
|
|||
.. cpp:enumerator:: BOTAN_FFI_TPM_ERROR = -78 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. cpp:enumerator:: BOTAN_FFI_TPM_ERROR = -78 | |
.. cpp:enumerator:: BOTAN_FFI_TPM2_ERROR = -78 |
... we've been specific basically everywhere. I think we should always state TPM2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #4117 (comment) - this FFI error translates the ErrorType::TPMError
, which is also the error type for TPM2_Error
. Or should we just introduce a new TPM2 ErrorType
and keep them separated entirely?
…ntext_object() and add a helper function to safely cast to ESYS_CONTEXT*.
This PR is for #3877 regarding the RNG integration of TPM2. It already includes FFI and Python integration.
For current progress on the entire TPM2 integration beyond RNG check #3877.
There's no need to merge this for 3.5.0 (this API may still possibly be subject to changes within broader TPM2 integration of #3877).
Setup with
swtpm
To emulate the TPM2, we use swtpm. The following steps are necessary to setup
swtpm
to work with our implementation:sudo apt install tpm2-tools libtss2-dev swtpm
--with-tpm2
and buildmkdir /tmp/myvtpm
swtpm socket --tpmstate dir=/tmp/myvtpm --tpm2 --ctrl type=tcp,port=2322 --server type=tcp,port=2321 --flags not-need-init
(terminal has to stay open)tpm2_startup -c
By default, Botan's new
TPM2_Context
will use the TPM2-TSS default strategy for identifying the TCTI. To directly useswtpm
or another TCTI, pass the corresponding TCTI name configuration toBotan::TPM2_Context::create
.Python example (because that's what we need)
ToDos