Skip to content
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

Cryptodoc update for TPM 2.0 Wrapper #250

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Cryptodoc update for TPM 2.0 Wrapper #250

wants to merge 3 commits into from

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Oct 25, 2024

Closes #248

atreiber94

This comment was marked as outdated.

Copy link
Collaborator

@atreiber94 atreiber94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just found 2 nits, fine with me to merge

Comment on lines 66 to 69
- ``Esys_EvictControl(ctx, key.transient_handle(), sessions, out(handle))``
- ``Esys_TR_SetAuth(ctx, key.transient_handle(), auth_value)``
- ``key.persistent_handle() = Esys_TR_GetTpmHandle(ctx, key.transient_handle())``
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put in two topics into one suggestions here earlier: Technically it should be m_esys_ctx here instead of ctx

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? It is m_impl->m_ctx which is the same as for the other ESAPI invocations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this has been abstracted to m_esys_ctx in this documentation (there's no way to see what ctxis based on the Input description of the method documented here.
In the other ESAPI invocations, ctx has been listed as an input (and for the docu I'm fine with not distinguishing between the wrapped and original context)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. I'll just add the ctx as an input value. Technically, this would be the this pointer here, I believe that's why I left it out of the inputs. But that might not be obvious at all.

I think, we should definitely abstract from the context's inner handles here.

docs/cryptodoc/src/10_tpm.rst Outdated Show resolved Hide resolved
Co-Authored-By: Amos Treiber <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide Documentation of TPM2 in the Documents
2 participants