Introduce DevID node attestor #2111

marcosy · 2021-02-13T00:39:36Z

Pull Request check list

Commit conforms to CONTRIBUTING.md?
Proper tests/regressions included?
Documentation updated?

Affected functionality

DevID node attestor.

Description of change

This PR introduces a DevID node attestor plugin according to the proposal made in #1003 (comment)
It is based on the external plugin prototype developed by @langbeck.

In addition to the behavior described in that proposal, an optional configurable was added (check_devid_residency) to validate that the DevID resides in a TPM whose endorsement certificate is signed by a particular manufacturer CA.

This validation is generally done in the TPM provisioning phase. However, SPIRE could also perform this verification as an extra security guarantee.

This work is functional but still in progress. Documentation, unit tests and integration tests are missing as well as some tooling to facilitate the creation of DevIDs. However, I am creating this draft PR now since it may help to illustrate the proposed approach. It would be great to get some early feedback, questions and suggestions at this point.

Which issue this PR fixes

Related to but not fully address #1003. There are other proposed approaches that implement node attestation using directly the TPM endorsement certificate instead of DevIDs. We may want to reserve the "tpm" plugin name for a future SPIRE implementation of those approaches.

Signed-off-by: Marcos Yedro [email protected]

Signed-off-by: Marcos Yedro <[email protected]>

…ode-attestor

MarcosDY · 2021-03-04T13:09:13Z

pkg/agent/plugin/nodeattestor/devid/devid.go

+func (p *Plugin) FetchAttestationData(stream nodeattestor.NodeAttestor_FetchAttestationDataServer) error {
+	conf := p.getConfig()
+	if conf == nil {
+		return devid.Error("not configured")


since it is a GRPC call, maybe you can return status.Error here. (same comment for all errors)
for example:

https://github.com/spiffe/spire/blob/master/pkg/server/plugin/upstreamauthority/awspca/pca.go#L271

MarcosDY · 2021-03-04T13:10:24Z

pkg/agent/plugin/nodeattestor/devid/devid.go

+func (p *Plugin) FetchAttestationData(stream nodeattestor.NodeAttestor_FetchAttestationDataServer) error {
+	conf := p.getConfig()
+	if conf == nil {
+		return devid.Error("not configured")


devid package can be confusing because you are in a devid package, can you set a name like common_devid on import?

MarcosDY · 2021-03-04T13:24:12Z

pkg/agent/plugin/nodeattestor/devid/devid.go

+	// Send attestation request
+	err = stream.Send(&nodeattestor.FetchAttestationDataResponse{
+		AttestationData: &spc.AttestationData{
+			Type: devid.PluginName,


mmm not sure we must have same name for both plugins at least no using the same constant

Hmm, I can't think of any reason why this wouldn't be appropriate. We are already doing it in other attestors as well.

MarcosDY · 2021-03-04T13:30:17Z

pkg/agent/plugin/nodeattestor/devid/devid.go

+
+	// If DevID residency verification configured
+	var credActChallengeResp []byte
+	if conf.checkDevIDResidency && challenges.CredActivation != nil {


what happens in case checkDevIDResidency == true and CredActivation == nil,
is it expected that it is a success challenge?

what happens in case checkDevIDResidency == true and CredActivation == nil

That state indicates that the agent was configured to perform the proof of residency but the server was configured to not require it. The server doesn't send any credential activation challenge to solve (nil).

is it expected that it is a success challenge?

I wouldn't say that it is a successful resolution of the challenge. The challenge is not send by the sever, so the agent can (and must) ignore the "solving" function.

MarcosDY · 2021-03-04T13:42:26Z

pkg/agent/plugin/nodeattestor/devid/devid.go

+}
+
+func (p *Plugin) Configure(ctx context.Context, req *plugin.ConfigureRequest) (*plugin.ConfigureResponse, error) {
+	err := devid.ValidateGlobalConfig(req.GlobalConfig)


global config with trustdomain will be always there, you added it as a belt and suspender approach right?

Yes, I saw other node attestors doing that verification so I wanted to be consistent.

MarcosDY · 2021-03-04T16:07:42Z

pkg/agent/plugin/nodeattestor/devid/tpm/signingkey.go

+				return nil, nil, fmt.Errorf("max attempts reached: %w", err)
+			}
+
+			log.Printf("TPM was not able to start the command. Retrying: attempt (%d/%d)", i, maxAttempts)


you must be use some logs here maybe the one that is provided from plugin, but not stdout log.

Thanks! I forgot to add it after a refactor 🤦

MarcosDY · 2021-03-04T16:09:12Z

pkg/agent/plugin/nodeattestor/devid/tpm/signingkey.go

+// SRKTemplateRSA returns SRK template used during provisioning.
+func SRKTemplateRSA() tpm2.Public {
+	template := tpm2tools.SRKTemplateRSA()
+	template.RSAParameters.ModulusRaw = []byte{}


is possible. that ModulusRaw has some value? is it expected it to be replaced?

The gotpm library does not have a method to create the default SRK template recommended by the latest version of the specification. So we create the template on our side. I’ll add a comment and rename the function to make this clearer.

According to TCG TPM v2.0 Provisioning Guidance (section 7.5.1) the Storage Root Key (SRK) template should be based on the Endorsement Key (EK) template.
The EK template is defined in TCG EK Credential Profile for TPM Family 2.0 (section B). The latest version of this document (published in july 2020) defined a new template (called H-1) for the RSA EK template.
The new template sets the unique field (named here as ModulusRaw) to an empty buffer instead of a well-known buffer (filled with 0s).

MarcosDY · 2021-03-04T16:35:23Z

pkg/server/plugin/nodeattestor/devid/devid.go

+
+func validatePluginConfig(extConf *ExternalConfig) error {
+	// DevID bundle path is always required
+	if extConf.DevIDBundlePath == "" {


switch can simplify it

MarcosDY · 2021-03-04T16:38:00Z

pkg/server/plugin/nodeattestor/devid/devid.go

+	return nil
+}
+
+func verifyDevIDSignature(cert *x509.Certificate, roots *x509.CertPool) error {


it is used only once, maybe you can avoid creating this function?

I see your point, but I believe encapsulating this piece of code in a function is still useful to improve code readability.

MarcosDY · 2021-03-04T17:02:00Z

pkg/server/plugin/nodeattestor/devid/selectors.go

+	"2.5.4.17": "POSTALCODE",
+}
+
+func FromCertificate(selectorType, prefix string, cert *x509.Certificate) []*spc.Selector {


SelectorsFromCertificate?

or move to a package called selectors

evan2645 · 2021-03-09T20:51:00Z

Thank you for this @marcosy! I've started reviewing it, will probably take a little longer to complete. I do have one high level question though

In addition to the behavior described in that proposal, an optional configurable was added (check_devid_residency) to validate that the DevID resides in a TPM whose endorsement certificate is signed by a particular manufacturer CA.

This validation is generally done in the TPM provisioning phase. However, SPIRE could also perform this verification as an extra security guarantee.

I feel like being able to prove that the cert we're using is backed a key that resides in a TPM is the main motivating factor for making this a dedicated plugin rather than a feature in x509pop. If we're not proving this fact to the server, then it's hard for me to see a difference.

Is there a reason that someone wouldn't want to do this?

marcosy · 2021-03-10T15:18:32Z

Thanks for your comments @MarcosDY and @evan2645!

Is there a reason that someone wouldn't want to do this?

The only reason I can think of for someone not doing this is due to the extra configurations that are needed.

To verify that the DevID is backed by a key that resides in a TPM from a particular manufacturer, the user has to additionally provide:

An attestation key pair:
The attestation key pair can be generated using the TPM in the same way DevIDs are created. I don’t see any difficulty in getting them.
The endorsement certificate roots.
The problem with this point is that not all TPMs have an endorsement certificate (some manufacturers only include an endorsement key).

However, we could find an alternative validation method in case the endorsement certificate is not available (e.g. using a public key allowed-list in the server-side plugin configuration)

- Setting 'common_devid' import name for 'devid' pkg - Create errors using status pkg for gRPC handles - Rename internalConfig/ExternalConfig to config/Config - Change 'if' to 'switch' for config validation - Rename some variables for better readibility - Add some early returns - Clarify comment for EK handle value Signed-off-by: Marcos Yedro <[email protected]>

Signed-off-by: Marcos Yedro <[email protected]>

- Rename functions that get selectors - Change set of 'if' for switch - Replace newCachedKey function for library function - Add comment and rename function that creates SRK from default template Signed-off-by: Marcos Yedro <[email protected]>

Signed-off-by: Marcos Yedro <[email protected]>

marcosy · 2021-03-22T14:55:02Z

I addressed the initial round of comments. Thanks for the early feedback!
I am still working on unit tests and on some tooling (to facilitate DevID creation). However, I believe this PR can be marked as "Ready for review".

…evid-node-attestor Signed-off-by: Andrew Harding <[email protected]>

Signed-off-by: Andrew Harding <[email protected]>

azdagron · 2021-03-31T00:10:36Z

I've just merged #2180. Consequently, you'll need to touch up the import for the nodeattestor definitions.

Old import:
nodeattestorv0 "github.com/spiffe/spire/proto/spire/server/nodeattestor/v0"
New import:
nodeattestorv0 "github.com/spiffe/spire/proto/spire/plugin/server/nodeattestor/v0"

If you'd rather me touch it up and push a commit, I'm happy to oblige. Just let me know.

…ode-attestor Signed-off-by: Marcos Yedro <[email protected]>

Signed-off-by: Marcos Yedro <[email protected]>

marcosy · 2021-04-01T13:51:13Z

Thanks for letting me know! I'll update the PR 🙂

marcosy · 2021-04-01T14:59:56Z

I’ve been working on some tools to facilitate the testing/use of this plugin.

This repository contains some useful scripts to create and run a TPM emulator in a docker container.
It contains two scripts. One creates a new TPM (it outputs a TPM state file). And the other one runs the TPM emulator (the user needs to provide a TPM state file).
This repository contains example files to configure the node attestor with the emulator (DevIDs, AK, TPM state).
This repository contains a provisioning tool to generate DevID and AK credentials at will.

I still need to add unit tests for this plugin.

Signed-off-by: Marcos Yedro <[email protected]>

evan2645

This is looking GREAT @marcosy! Huge improvements here structurally, it is much much easier to read and review. Good job! :-D

I left some more comments here, much more narrowly scoped than before but still mostly about naming, typo/error cleanup, the autodetect logic etc. I need to do another pass analyzing the exchanges and flow between agent/server, key usages etc, but I think that overall we're looking pretty good and getting close. Thanks for your continued work on this ❤️

evan2645 · 2021-05-25T19:44:12Z

doc/plugin_server_nodeattestor_tpm_devid.md

+key pair was generated and resides in a TPM. Additionally, the server verifies 
+that the TPM belongs to a trusted vendor by verifying that the endorsement 
+certificate is rooted to a trusted set of CAs.


I think the primary purpose for doing this is to ensure that the TPM is authentic (as opposed to a software TPM, where the keys may not be protected at all). Assuming my thinking there is correct, I suggest the following

Suggested change

key pair was generated and resides in a TPM. Additionally, the server verifies

that the TPM belongs to a trusted vendor by verifying that the endorsement

certificate is rooted to a trusted set of CAs.

key pair was generated and resides in a TPM. Additionally, the server verifies

that the TPM is authentic by verifying that the endorsement certificate is

rooted to a trusted set of manufacturer CAs.

Thanks! Looks clearer in that way, I'll update it.

evan2645 · 2021-05-25T19:46:48Z

doc/plugin_server_nodeattestor_tpm_devid.md

+| Configuration 			| Description | Default                 |
+| -------------------------	| ----------- | ----------------------- |
+| `devid_bundle_path` 		| The path to the trusted CA bundle on disk for the DevID certificate. The file must contain one or more PEM blocks forming the set of trusted root CA's for chain-of-trust verification. | |
+| `endorsement_bundle_path` | The path to the trusted CA bundle on disk for the endorsement certificate. The file must contain one or more PEM blocks forming the set of trusted root CA's for chain-of-trust verification. | |


Similar to above comments, minor nit on the wording:

Suggested change

| `endorsement_bundle_path` | The path to the trusted CA bundle on disk for the endorsement certificate. The file must contain one or more PEM blocks forming the set of trusted root CA's for chain-of-trust verification. | |

| `endorsement_ca_path` | The path to the trusted manufacturer CA certificate(s) on disk. The file must contain one or more PEM blocks forming the set of trusted manufacturer CA's for chain-of-trust verification. | |

evan2645 · 2021-05-25T19:49:29Z

doc/plugin_server_nodeattestor_tpm_devid.md

+
+| Configuration 			| Description | Default                 |
+| -------------------------	| ----------- | ----------------------- |
+| `devid_bundle_path` 		| The path to the trusted CA bundle on disk for the DevID certificate. The file must contain one or more PEM blocks forming the set of trusted root CA's for chain-of-trust verification. | |


What do you think about using ca instead of bundle on the naming here? We use the word bundle a lot, and it frequently means different things ... I feel like ca is a little bit clearer.

Suggested change

| `devid_bundle_path` | The path to the trusted CA bundle on disk for the DevID certificate. The file must contain one or more PEM blocks forming the set of trusted root CA's for chain-of-trust verification. | |

| `devid_ca_path` | The path to the trusted CA certificate(s) on disk to use for DevID validation. The file must contain one or more PEM blocks forming the set of trusted root CA's for chain-of-trust verification. | |

I agree, CA looks more precise. I'll update it.

evan2645 · 2021-05-25T19:51:31Z

doc/plugin_server_nodeattestor_tpm_devid.md

+| Selector                  	| Example															| Description						|
+| ---------------------------- 	| -----------------------------------------------------------------	| ---------------------------------	|
+| Fingerprint					|`tpm_devid:fingerprint:9ba51e2643bea24e91d24bdec3a1aaf8e967b6e5`	| The certificate SHA1 fingerprint as a hex string.|
+| Certificate serial number		|`tpm_devid:certificate:serialnumber:835861456985135479204994168`	| The certificate serial number.	| 


When we are talking about devid selectors, we are pretty much always talking about certs right? Perhaps we can omit the certificate part here? e.g. tpm_devid:fingerprint:1234, tpm_devid:serialnumber:1234, tpm_devid:subject:cn:foo etc?

nit: serial_number feels a little more consistent since the plugin name is snake case and x509 defines it as serialNumber

sounds good!

Reading your response to my other comment about IDevID vs LDevID, I realize now that perhaps we will have more than one serial_number type selector in the future if we grow support for IDevID? Maybe instead of omitting certificate from the selector, it should be replaced with ldevid?
tpm_devid:ldevid:serial_number:1234

I agree with your comment below related to selectors. I believe that for this initial version of the attestor we should be good with having the subject common name and the fingerprints (same selectors than x509pop). We can add more selectors later if there is a use case not covered.

Regarding adding a "ldevid"/"idevid" prefix, since probably only one of them will be used at the time, maybe it is not necessary to specify if it is an IDevID or LDevID in the selectors?

evan2645 · 2021-05-25T19:53:33Z

doc/plugin_server_nodeattestor_tpm_devid.md

+| Fingerprint					|`tpm_devid:fingerprint:9ba51e2643bea24e91d24bdec3a1aaf8e967b6e5`	| The certificate SHA1 fingerprint as a hex string.|
+| Certificate serial number		|`tpm_devid:certificate:serialnumber:835861456985135479204994168`	| The certificate serial number.	| 
+| Subject common name			|`tpm_devid:certificate:subject:cn:example.org`						| The subject's common name.		|
+| Subject common name			|`tpm_devid:certificate:subject:cn:example.org`						| The subject's common name.		|


evan2645 · 2021-05-26T23:26:10Z

pkg/server/plugin/nodeattestor/tpmdevid/devid.go

+	// Verify DevID residency (if configured)
+	var nonce []byte
+	var credActivationChallenge *common_devid.CredActivation
+	if conf.checkDevIDResidency {


Should this still exist? I think it is left over?

evan2645 · 2021-05-26T23:26:27Z

pkg/server/plugin/nodeattestor/tpmdevid/devid.go

+	}
+
+	// Verify credential activation challenge (if configured)
+	if conf.checkDevIDResidency {


Same comment here

evan2645 · 2021-05-26T23:28:00Z

pkg/server/plugin/nodeattestor/tpmdevid/devid.go

+	}
+
+	// Create SPIFFE ID and selectors
+	certSelectors := selectorsFromCertificate(common_devid.PluginName, "certificate", devIDCert)


The number of places that we're generating selectors from a cert is growing. We can already see some divergence between this and x509pop. I wonder if we should have a shared function for this. We've been talking about having similar shared functions exposed for other attestors (e.g. k8s attestor podspec -> selectors function).

I think @azdagron may have some thoughts here?

Sounds good, I can create a shared function and put it in package "pkg/common/selector", what do you think?

evan2645 · 2021-05-26T23:28:44Z

pkg/server/plugin/nodeattestor/tpmdevid/devid.go

+
+	// Create SPIFFE ID and selectors
+	certSelectors := selectorsFromCertificate(common_devid.PluginName, "certificate", devIDCert)
+	fingerprint := fingerprint(devIDCert)


This fingerprint part can also be inside selectorsFromCertificate if we are going to share with x509pop

evan2645 · 2021-05-26T23:37:26Z

pkg/server/plugin/nodeattestor/tpmdevid/selectors.go

+	"2.5.4.6":  "c",            // Country
+	"2.5.4.8":  "st",           // State
+	"2.5.4.10": "o",            // Organization
+	"2.5.4.11": "ou",           // Organizational unit


I'd prefer to not deal with codepoints like this. The pkix name struct given by golang x509 certificate has all of this stuff already and we can create selectors from them explicitly?

I also wonder if we really need all of these. Selector naming and count is a general concern, and it would be good to have some clarity on the need before adding a bunch of them. The x509pop attestor emits only fingerprints and cn, and we have not had any request for more than that... it has been there for some time now.

To be clear, I'm all for having more selectors if it's useful to have them. What I'm trying to avoid is supporting selectors that users may be less likely to consume.

I talked to some folks which are planning to use this plugin internally. They seem to be ok with using the subject common name and fingerprint. So I think that we should be fine generating the same set of selectors that are generated in x509pop (at least for this initial version).
I'll update this.

Signed-off-by: Marcos Yedro <[email protected]>

amartinezfayo

This is looking great @marcosy!
While I'm doing a more detailed review, I thought to leave a first set of comments.
Awesome job!

amartinezfayo · 2021-06-07T17:34:29Z

pkg/agent/plugin/nodeattestor/tpmdevid/devid.go

+	// DevID certificate, public and private key are always required
+	switch {
+	case c.DevIDCertPath == "":
+		return fmt.Errorf("devid_cert_path is required")


No need for formatted error:

Suggested change

return fmt.Errorf("devid_cert_path is required")

return errors.New("devid_cert_path is required")

amartinezfayo · 2021-06-07T17:35:20Z

pkg/agent/plugin/nodeattestor/tpmdevid/devid.go

+		return fmt.Errorf("devid_cert_path is required")
+
+	case c.DevIDPrivPath == "":
+		return fmt.Errorf("devid_priv_path is required")


No need for formatted error:

Suggested change

return fmt.Errorf("devid_priv_path is required")

return errors.New("devid_priv_path is required")

amartinezfayo · 2021-06-07T17:35:40Z

pkg/agent/plugin/nodeattestor/tpmdevid/devid.go

+		return fmt.Errorf("devid_priv_path is required")
+
+	case c.DevIDPubPath == "":
+		return fmt.Errorf("devid_pub_path is required")


No need for formatted error:

Suggested change

return fmt.Errorf("devid_pub_path is required")

return errors.New("devid_pub_path is required")

amartinezfayo · 2021-06-07T17:36:39Z

pkg/agent/plugin/nodeattestor/tpmdevid/tpmutil/autodetect.go

+	return "", errors.New("unable to autodetect TPM")
+}
+
+// VerifyTPM verifies that the given path belongs to a functional TPM 2.0


Suggested change

// VerifyTPM verifies that the given path belongs to a functional TPM 2.0

// isValidTPM verifies that the given path belongs to a functional TPM 2.0

amartinezfayo · 2021-06-07T19:34:17Z

pkg/agent/plugin/nodeattestor/tpmdevid/tpmutil/signingkey.go

+		}
+
+	default:
+		return nil, fmt.Errorf("bad key type: 0x%04x", pub.Type)


0x%04x is always returned instead of pub.Type.

Hmm I think it is working ok? https://play.golang.org/p/2ecIQALLuu7
The type is printed in hexadecimal format to make it easier to search for it in the go-tpm constants.

amartinezfayo · 2021-06-07T20:05:14Z

pkg/server/plugin/nodeattestor/tpmdevid/devid.go

+	}
+
+	if keyFromCert.E != keyFromTemplate.E {
+		return fmt.Errorf("exponent missmatch")


No need for formatted error.
There is also a typo:

Suggested change

return fmt.Errorf("exponent missmatch")

return errors.New("exponent mismatch")

amartinezfayo · 2021-06-07T20:05:51Z

pkg/server/plugin/nodeattestor/tpmdevid/devid.go

+	}
+
+	if keyFromCert.N.Cmp(keyFromTemplate.N) != 0 {
+		return fmt.Errorf("modulus missmatch")


No need for formatted error.
There is also a typo:

Suggested change

return fmt.Errorf("modulus missmatch")

return errors.New("modulus mismatch")

amartinezfayo · 2021-06-07T20:07:33Z

pkg/server/plugin/nodeattestor/tpmdevid/devid.go

+		return params.Sign, nil
+
+	default:
+		return nil, fmt.Errorf("unsupported key type 0x%04x", pub.Type)


0x%04x is always returned instead of pub.Type.

Hmm I think it is working ok? https://play.golang.org/p/2ecIQALLuu7 but maybe I'm missing something

You are right!

amartinezfayo · 2021-06-07T20:13:36Z

pkg/agent/plugin/nodeattestor/tpmdevid/tpmutil/signingkey.go

+
+		case isRetry(err):
+			if i == maxAttempts {
+				return nil, fmt.Errorf("max attempts reached: %w", err)


It would be useful to include information about the action involved, otherwise the error may be difficult to understand. Maybe something like max attempts reached while trying to sign the given data.

amartinezfayo · 2021-06-07T20:15:48Z

pkg/agent/plugin/nodeattestor/tpmdevid/tpmutil/signingkey.go

+
+		case isRetry(err):
+			if i == maxAttempts {
+				return nil, nil, fmt.Errorf("max attempts reached: %w", err)


I think that here also would benefit from being more detailed about the action that is being performed while the error happened.

amartinezfayo · 2021-06-08T11:49:35Z

doc/plugin_server_nodeattestor_tpm_devid.md

+
+1. A proof-of-possession challenge: This is required to verify the node is in 
+possession of the private key that corresponds to the DevID certificate. 
+Additionally, the server verifies that the DevID certificate  is rooted to


Suggested change

Additionally, the server verifies that the DevID certificate is rooted to

Additionally, the server verifies that the DevID certificate is rooted to

amartinezfayo · 2021-06-08T16:26:15Z

pkg/agent/plugin/nodeattestor/tpmdevid/tpmutil/signingkey.go

+	}
+
+	var sig *tpm2.Signature
+	for i := 0; i <= maxAttempts; i++ {


I think that this count should start with 1 instead of 0, otherwise the maximum number of attempts will be maxAttempts + 1.

amartinezfayo · 2021-06-08T18:32:42Z

pkg/agent/plugin/nodeattestor/tpmdevid/tpmutil/signingkey.go

+		sig, err = tpm2.Sign(k.rw, k.Handle, "", digest, token, nil)
+		switch {
+		case err == nil:
+			break


This break statement affects the switch only, it doesn't terminate the for loop, that means that the Sign operation will continue to happen until maxAttempts is reached.

Signed-off-by: Marcos Yedro <[email protected]>

amartinezfayo

Thank you for your patience with this @marcosy!
I've noticed that agent_full.conf and server_full.conf are missing updates for the agent and server plugins configs. We should update them.
I left a bunch more comments, but all of them should be easy to address.

amartinezfayo · 2021-06-23T15:13:32Z

pkg/agent/plugin/nodeattestor/tpmdevid/tpmutil/session.go

+		return nil, fmt.Errorf("failed to read NV index %08x: %w", EKCertificateHandleRSA, err)
+	}
+
+	// In some TPMs, when we read bytes from a NV index, the readed content


Suggested change

// In some TPMs, when we read bytes from a NV index, the readed content

// In some TPMs, when we read bytes from an NV index, the content read

amartinezfayo · 2021-06-23T15:14:41Z

pkg/agent/plugin/nodeattestor/tpmdevid/tpmutil/session.go

+	}
+
+	// In some TPMs, when we read bytes from a NV index, the readed content
+	// includes the DER encoded x509 certificate + trailing data. We need to


Suggested change

// includes the DER encoded x509 certificate + trailing data. We need to

// includes the DER encoded X.509 certificate + trailing data. We need to

amartinezfayo · 2021-06-23T15:25:23Z

pkg/agent/plugin/nodeattestor/tpmdevid/tpmutil/session.go

+func (c *Session) loadKey(pubKey, privKey []byte, parentKeyPassword, keyPassword string) (*SigningKey, error) {
+	pub, err := tpm2.DecodePublic(pubKey)
+	if err != nil {
+		return nil, fmt.Errorf("tpm2.Public decoding failed: %w", err)


Maybe tpm2.DecodePublic failed?

amartinezfayo · 2021-06-23T15:30:28Z

pkg/agent/plugin/nodeattestor/tpmdevid/tpmutil/session.go

+		},
+		hSession, // policyHandle:	Handle for the policy session being extended.
+		nil,      // policyNonce:	The policy nonce for the session (can be the Empty Buffer).
+		nil,      // cpHash:		Digest of the command parameters to which this authorization is limited. (If it is not limited, the parameter will be the Empty Buffer).


Suggested change

nil, // cpHash: Digest of the command parameters to which this authorization is limited. (If it is not limited, the parameter will be the Empty Buffer).

nil, // cpHash: Digest of the command parameters to which this authorization is limited (if it is not limited, the parameter will be the Empty Buffer).

amartinezfayo · 2021-06-29T14:06:08Z

pkg/agent/plugin/nodeattestor/tpmdevid/devid.go

+		Log:        p.log,
+	})
+	if err != nil {
+		return status.Errorf(codes.Internal, "unable start a new TPM session: %v", err)


Suggested change

return status.Errorf(codes.Internal, "unable start a new TPM session: %v", err)

return status.Errorf(codes.Internal, "unable to start a new TPM session: %v", err)

amartinezfayo · 2021-06-29T22:35:55Z

test/tpmsimulator/simulator.go

+// certificate and key are generated.
+func NewProvisioningCA(c *ProvisioningConf) (*ProvisioningAuthority, error) {
+	if c == nil {
+		return nil, fmt.Errorf("provisioning config is nil")


Suggested change

return nil, fmt.Errorf("provisioning config is nil")

return nil, errors.New("provisioning config is nil")

amartinezfayo · 2021-06-29T22:41:06Z

test/tpmsimulator/simulator.go

+		}
+
+	default:
+		return nil, fmt.Errorf("the root certificate or private key is nil but not both")


Suggested change

return nil, fmt.Errorf("the root certificate or private key is nil but not both")

return nil, errors.New("the root certificate or private key is nil but not both")

amartinezfayo · 2021-06-29T22:42:01Z

test/tpmsimulator/simulator.go

+	return provisioningAuthority, nil
+}
+
+// Chain returns the leaf and intermediates certificates in DER format


Suggested change

// Chain returns the leaf and intermediates certificates in DER format

// Chain returns the leaf and intermediate certificates in DER format

amartinezfayo · 2021-06-29T22:42:29Z

test/tpmsimulator/simulator.go

+	return chain
+}
+
+// ChainPem returns the leaf and intermediates certificates in PEM format


Suggested change

// ChainPem returns the leaf and intermediates certificates in PEM format

// ChainPem returns the leaf and intermediate certificates in PEM format

amartinezfayo · 2021-06-29T22:43:55Z

test/tpmsimulator/simulator.go

+		privateKey = p.RootKey
+
+	default:
+		return nil, fmt.Errorf("the intermediate certificate or private key is nil but not both")


Suggested change

return nil, fmt.Errorf("the intermediate certificate or private key is nil but not both")

return nil, errors.New("the intermediate certificate or private key is nil but not both")

Signed-off-by: Marcos Yedro <[email protected]>

amartinezfayo

Thank you very much for this effort, @marcosy! 🎉

* Introduce DevID Node Attestor Signed-off-by: Marcos Yedro <[email protected]>

rgl · 2021-10-29T18:28:43Z

Can the documentation at https://github.com/spiffe/spire/blob/main/doc/plugin_server_nodeattestor_tpm_devid.md be updated to describe the out-of-band mechanism? From reading the @marcosy comment, it seems the tool to do that is at https://github.com/HewlettPackard/devid-provisioning-tool. Maybe add a link to that?

marcosy added 2 commits February 12, 2021 21:07

Introduce devid node attestor

643aa49

Signed-off-by: Marcos Yedro <[email protected]>

Merge remote-tracking branch 'upstream/master' into introduce-devid-n…

ab540c1

…ode-attestor

MarcosDY reviewed Mar 4, 2021

View reviewed changes

Merge branch 'master' into introduce-devid-node-attestor

3ac8ae8

azdagron assigned evan2645 Mar 9, 2021

marcosy added 5 commits March 15, 2021 11:06

Add missing limit to retry

a805d01

Signed-off-by: Marcos Yedro <[email protected]>

Add hclog logger to SigningKey type

4025a81

Signed-off-by: Marcos Yedro <[email protected]>

Address PR comments

bf02bde

- Rename functions that get selectors - Change set of 'if' for switch - Replace newCachedKey function for library function - Add comment and rename function that creates SRK from default template Signed-off-by: Marcos Yedro <[email protected]>

Merge branch 'master' into introduce-devid-node-attestor

b632a5c

Signed-off-by: Marcos Yedro <[email protected]>

marcosy marked this pull request as ready for review March 22, 2021 14:55

marcosy requested review from amartinezfayo, APTy, azdagron, evan2645 and rturner3 as code owners March 22, 2021 14:55

marcosy changed the title ~~[WIP] Introduce DevID node attestor~~ Introduce DevID node attestor Mar 22, 2021

azdagron added 2 commits March 25, 2021 13:58

Merge remote-tracking branch 'origin/master' into marcosy-introduce-d…

735451e

…evid-node-attestor Signed-off-by: Andrew Harding <[email protected]>

Apply NodeAttestor Facade related changes to DevID NodeAttestor

763f55a

Signed-off-by: Andrew Harding <[email protected]>

marcosy added 2 commits April 1, 2021 10:42

Merge remote-tracking branch 'upstream/master' into introduce-devid-n…

81fa81e

…ode-attestor Signed-off-by: Marcos Yedro <[email protected]>

Update nodeattestor import path

b21077a

Signed-off-by: Marcos Yedro <[email protected]>

marcosy added 3 commits April 2, 2021 14:26

Do not call Close() when using emulator

112dd33

Signed-off-by: Marcos Yedro <[email protected]>

Use internal rwc instead of function argument

5751601

Signed-off-by: Marcos Yedro <[email protected]>

Flush session on error only and close EK session after use

5649d6b

Signed-off-by: Marcos Yedro <[email protected]>

marcosy added 11 commits May 17, 2021 22:57

Rename devid directory to tpm_devid

48bff98

Signed-off-by: Marcos Yedro <[email protected]>

Adding TPM auto-detection

f4fd926

Signed-off-by: Marcos Yedro <[email protected]>

Rename package to tpmdevid due to golint conflict with tpm_devid

ca15d71

Signed-off-by: Marcos Yedro <[email protected]>

Merge branch 'main' into introduce-devid-node-attestor

f261d4c

Signed-off-by: Marcos Yedro <[email protected]>

Updating plugin type after merging main branch

dae5910

Signed-off-by: Marcos Yedro <[email protected]>

Fix lint warning

3af664e

Signed-off-by: Marcos Yedro <[email protected]>

Add unit tests for tpmutil

50b5c4f

Signed-off-by: Marcos Yedro <[email protected]>

Migrate tpmdevid agent plugin to v1

f03cbae

Signed-off-by: Marcos Yedro <[email protected]>

Migrate tpmdevid server plugin to v1

22221e2

Signed-off-by: Marcos Yedro <[email protected]>

Merge branch 'main' into introduce-devid-node-attestor

20e5b45

Add unit tests for tpm_devid attestor (agent side)

b9fa3db

Signed-off-by: Marcos Yedro <[email protected]>

evan2645 reviewed May 26, 2021

View reviewed changes

Add unit tests for tpm_devid attestor (server side)

80cfadb

Signed-off-by: Marcos Yedro <[email protected]>

amartinezfayo reviewed Jun 7, 2021

View reviewed changes

amartinezfayo reviewed Jun 8, 2021

View reviewed changes

marcosy added 6 commits June 16, 2021 11:58

Address PR comments

c738c6d

Signed-off-by: Marcos Yedro <[email protected]>

Merge branch 'main' into introduce-devid-node-attestor

4be46cd

Signed-off-by: Marcos Yedro <[email protected]>

Add libssl-dev to dev container (used by tpm simulator)

36cec62

Signed-off-by: Marcos Yedro <[email protected]>

Replace io/util by os in tpm plugin

f698932

Signed-off-by: Marcos Yedro <[email protected]>

Merge branch 'main' into introduce-devid-node-attestor

0b32c26

Fix lint errors

61d762c

Signed-off-by: Marcos Yedro <[email protected]>

amartinezfayo reviewed Jun 30, 2021

View reviewed changes

marcosy and others added 3 commits June 30, 2021 23:06

Address PR comments

b4a980c

Signed-off-by: Marcos Yedro <[email protected]>

Merge branch 'main' into introduce-devid-node-attestor

2596839

Merge branch 'main' into introduce-devid-node-attestor

e89cb69

amartinezfayo approved these changes Jul 2, 2021

View reviewed changes

amartinezfayo merged commit 32b99d8 into spiffe:main Jul 2, 2021

rturner3 pushed a commit to rturner3/spire that referenced this pull request Aug 4, 2021

Introduce DevID node attestor (spiffe#2111)

17f8ed5

* Introduce DevID Node Attestor Signed-off-by: Marcos Yedro <[email protected]>

rturner3 pushed a commit that referenced this pull request Aug 5, 2021

Introduce DevID node attestor (#2111)

bdc0dca

* Introduce DevID Node Attestor Signed-off-by: Marcos Yedro <[email protected]>

	\| `endorsement_bundle_path` \| The path to the trusted CA bundle on disk for the endorsement certificate. The file must contain one or more PEM blocks forming the set of trusted root CA's for chain-of-trust verification. \| \|
	\| `endorsement_ca_path` \| The path to the trusted manufacturer CA certificate(s) on disk. The file must contain one or more PEM blocks forming the set of trusted manufacturer CA's for chain-of-trust verification. \| \|

	\| `devid_bundle_path` \| The path to the trusted CA bundle on disk for the DevID certificate. The file must contain one or more PEM blocks forming the set of trusted root CA's for chain-of-trust verification. \| \|
	\| `devid_ca_path` \| The path to the trusted CA certificate(s) on disk to use for DevID validation. The file must contain one or more PEM blocks forming the set of trusted root CA's for chain-of-trust verification. \| \|

	return fmt.Errorf("devid_cert_path is required")
	return errors.New("devid_cert_path is required")

	return fmt.Errorf("devid_priv_path is required")
	return errors.New("devid_priv_path is required")

	return fmt.Errorf("devid_pub_path is required")
	return errors.New("devid_pub_path is required")

	// VerifyTPM verifies that the given path belongs to a functional TPM 2.0
	// isValidTPM verifies that the given path belongs to a functional TPM 2.0

	return fmt.Errorf("exponent missmatch")
	return errors.New("exponent mismatch")

	return fmt.Errorf("modulus missmatch")
	return errors.New("modulus mismatch")

	Additionally, the server verifies that the DevID certificate is rooted to
	Additionally, the server verifies that the DevID certificate is rooted to

	// In some TPMs, when we read bytes from a NV index, the readed content
	// In some TPMs, when we read bytes from an NV index, the content read

	// includes the DER encoded x509 certificate + trailing data. We need to
	// includes the DER encoded X.509 certificate + trailing data. We need to

	nil, // cpHash: Digest of the command parameters to which this authorization is limited. (If it is not limited, the parameter will be the Empty Buffer).
	nil, // cpHash: Digest of the command parameters to which this authorization is limited (if it is not limited, the parameter will be the Empty Buffer).

	return status.Errorf(codes.Internal, "unable start a new TPM session: %v", err)
	return status.Errorf(codes.Internal, "unable to start a new TPM session: %v", err)

	return nil, fmt.Errorf("provisioning config is nil")
	return nil, errors.New("provisioning config is nil")

	return nil, fmt.Errorf("the root certificate or private key is nil but not both")
	return nil, errors.New("the root certificate or private key is nil but not both")

	// Chain returns the leaf and intermediates certificates in DER format
	// Chain returns the leaf and intermediate certificates in DER format

	// ChainPem returns the leaf and intermediates certificates in PEM format
	// ChainPem returns the leaf and intermediate certificates in PEM format

	return nil, fmt.Errorf("the intermediate certificate or private key is nil but not both")
	return nil, errors.New("the intermediate certificate or private key is nil but not both")

Introduce DevID node attestor #2111

Introduce DevID node attestor #2111

Conversation

marcosy commented Feb 13, 2021 • edited Loading

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

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

Choose a reason for hiding this comment

evan2645 commented Mar 9, 2021

marcosy commented Mar 10, 2021

marcosy commented Mar 22, 2021

azdagron commented Mar 31, 2021

marcosy commented Apr 1, 2021

marcosy commented Apr 1, 2021

evan2645 left a 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

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

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

amartinezfayo left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amartinezfayo Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

amartinezfayo left a 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

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

amartinezfayo left a comment

marcosy commented Feb 13, 2021 •

edited

Loading

amartinezfayo Jun 8, 2021 •

edited

Loading