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

Add EK Certificate Chain support #116

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ematery
Copy link

@ematery ematery commented Oct 21, 2024

No description provided.

Copy link
Member

@THS-on THS-on left a comment

Choose a reason for hiding this comment

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

Thanks for creating a proposal! This is definitely a welcome change

Comment on lines +124 to +127
* get all available NV-Index-Handles in the defined range (`0X01C00100 - 0X01C001FF`)
* read the chain from the NV-Index-Handles
* Theoretically the certificates can be in any order. This would require an option to make a sorting possible in the agent. As it does not make really sense to store the chain in an unsorted order i would postpone any implementation till this case really exists.
* Only a subset of the NV-Index-Handles may be required. This would require an option to filter/set the required handles. I would also wait for implementation till a real case exists for that.
Copy link
Member

Choose a reason for hiding this comment

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

This should be probably implemented in the rust-tss-esapi create.

Copy link
Author

Choose a reason for hiding this comment

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

The rust-tss-esapi is a rust wrapper for the native tss-esapi library, which does not include a function to get the certificate chain. Perhaps it is not such a bad idea to have a custom implementation. The guidelines in the "TCG EK Credential Profile" provide manufacturers with considerable freedom in how to store certificates in the TPM. Additionally, there is the possibility of storing multiple certificate chains. In my opinion, the role of the Keylime agent is to read, filter, and sort the certificates. It should provide the Keylime registrar with exactly one verifiable chain at any given time. The server should not be responsible for managing multiple chains for clients or determining which chain can be verified. The location of the chain and how it should be processed (filtered/sorted) should be configurable through the agent's settings. This approach leads to the majority of the work (filtering/sorting) being handled by the agent, making the call to rust-tss-esapi equivalent to an nvread operation.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation. I can see that it then makes sense to implement in agent. If it is generic enough we can upstream it to the abstraction layer of the rust-tss-espai crate, which provides more high level common functions.

On another note we have our monthly meeting today: keylime/meetings#82. Would be great if you have the time to join.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, thank you. I will join.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if I agree completely with adding the possibility for the agent to be configured on how to sort and filter certificates. For me, the agent simply has to find the chain that signed the EK certificate and provide it, no need for the other chains (why they are there in the first place?). Probably being able to configure the range of NV indexes to read is sufficient, but the default will be to read all of them anyway (i.e. in case the range is not configured).

I agree with @THS-on that getting the certificate chain looks generic enough to be implemented on rust-tss-esapi crate. It is definitely not specific to Keylime.

Copy link
Author

Choose a reason for hiding this comment

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

The standard is just allows multiple chains.

Revision 2 January 26, 2022 Published Page 15 of 60
The NV indices MUST be populated starting with index 0x01c00100 through index 0x01c001ff. There
is no requirement to store the certificates in any particular order whatsoever. If a concatenated
certificate chain does not fit in a single index, the chain MUST overflow to the next numerically larger
index in the list of NV Indices. If the storage space in a single index is insufficient to store the entire
certificate, the certificate MAY overflow into the next numerically larger index in the list of NV Indices.
It is recommended to use the least number of indices possible for storing the chains.
If more than one EK Certificate Chain exists, the chains MUST be concatenated. If two or more chains
have common certificates, such as when they are anchored to the same intermediate or root CA, the
certificates MUST NOT be stored more than once. Verifiers are recommended to read the NV Indices
in order and store a copy of the contents (certificates) into a memory buffer. Individual certificates
may then be parsed from the buffer into a certificate store in order to perform EK Certificate chain
validation.

TCG-EK-Credential-Profile-V-2.5-R2_published.pdf, 2.2.1.5.2

Is it possible to have logic in the keylime agent, till it is merged into the rust-tss-esapi crate? I would just keep the current implementation on the keylime-agent, to read the ca chain, as in the pull request #860.


## Proposal

* keylime_agent, send `ek_ca_chain` to registrar
Copy link
Member

Choose a reason for hiding this comment

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

We might get away with just storing the entire chain in the ekcert field. @ansasaki any opinion on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @THS-on that most probably a new field is not needed. We could simply append the intermediate certificates to the EK certificate.

Copy link
Author

Choose a reason for hiding this comment

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

I will then implement the necessary changes to store the chain in the ekcert field and update the existing fields in the database. I will need to add "-----BEGIN CERTIFICATE-----" and "-----END CERTIFICATE-----" (in PEM format) to the existing ekcerts in the database, as I require a delimiter in the chain.

Please notify me when everyone agrees that this is the way to proceed.

Copy link
Member

Choose a reason for hiding this comment

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

Yep using PEM encoding for the chain is probably a good idea, as this is standard and then can also be easily read by e.g. openssl


#### Providing big chains as attack
* Limit the amount of allowed chain size
* Use mTLS to only allow verified clients access
Copy link
Member

Choose a reason for hiding this comment

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

Not really related to this change.

Copy link
Author

Choose a reason for hiding this comment

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

Will remove this

## Proposal

* keylime_agent, send `ek_ca_chain` to registrar
* keylime registrar, store `ek_ca_chain` in database
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, the chain should be stored with the EK certificate and no new entry is added to the database.
Sorry for suggesting that before, but I think the less we modify the database the better.

Copy link
Author

Choose a reason for hiding this comment

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

will update


* keylime_agent, send `ek_ca_chain` to registrar
* keylime registrar, store `ek_ca_chain` in database
* keylime tenant, verify `ekcert` against `ek_ca_chain` and `ek_ca_chain` against `tpm_cert_store`
Copy link
Contributor

Choose a reason for hiding this comment

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

The process of verifying the EK certificate necessarily involves verifying its signature against the whole chain.
I think this sentence can be removed or rephrased to not imply that there are separate steps on the verification process.

Copy link
Author

Choose a reason for hiding this comment

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

will update

### Risks and Mitigations

#### Registrar/Tenant could be become incompatible with older database
* Update database to new scheme, only a single key is added to the registar db 'ek_ca_chain'
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, avoid changing the database and store the certificate chain with the EK certificate.

* Update database to new scheme, only a single key is added to the registar db 'ek_ca_chain'

#### Registrar/Tenant could become incompatible with older Agent
* Make 'ek_ca_chain' optional
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, avoid adding a new parameter and simply append the chain to the EK certificate.

@ematery
Copy link
Author

ematery commented Nov 5, 2024

Shall i update the enhancement request and then just open a new pull request?

@THS-on
Copy link
Member

THS-on commented Nov 22, 2024

Shall i update the enhancement request and then just open a new pull request?

@ematery sorry this got lost in my inbox. You can just update the current PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants