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

API Refactor: Allow the agent to use the experimental AttestAgent API #1739

Merged

Conversation

amartinezfayo
Copy link
Member

Pull Request check list

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

Affected functionality
Node attestation using the new experimental AttestAgent API.

Description of change
This PR adds the ability to use the new experimental AttestAgent API calling it from the agent. It's only used when the experimental enable_api setting is set to true.

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
return nil, fmt.Errorf("failed to get updated bundle %v", err)
}

b, err := api.ProtoToBundle(updatedBundle)
Copy link
Collaborator

Choose a reason for hiding this comment

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

you are using code from server inside agent, is that ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I'll move the bundleFromProto function recently added to the client package to the bundleutil package.

@@ -0,0 +1,445 @@
package attestor
Copy link
Collaborator

Choose a reason for hiding this comment

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

package attestor_test?

return ta.bundleClient
}

ta.agentClient.joinToken = testCase.joinToken
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe you can add agentClient to tt instead all separated value?and just set it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

same bundle

}

var attestReq *agent.AttestAgentRequest
if data.AttestationData != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think the new APIs force us to think about this a little differently. Only the first message should contain parameters, the rest (if any) should be challenge/response.

I'd assume something like:

1. fetch data from plugin
2. send data to server
3. loop
    3a. read response, if challenge is nil, goto 4
    3b. send challenge to plugin
    3c. recv challenge response from plugin
    3d. send challenge response to server
    3e. goto 3a
4. parse svid from response 


func getSVIDFromAttestAgentResponse(r *agent.AttestAgentResponse) ([]*x509.Certificate, error) {
if r.GetResult().Svid == nil {
return nil, errors.New("missing svid update")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return nil, errors.New("missing svid update")
return nil, errors.New("attest response is missing SVID")

for _, rawCert := range r.GetResult().Svid.CertChain {
cert, err := x509.ParseCertificate(rawCert)
if err != nil {
return nil, fmt.Errorf("invalid svid cert chain: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("invalid svid cert chain: %v", err)
return nil, fmt.Errorf("invalid SVID cert chain: %v", err)

}

var svid []*x509.Certificate
for _, rawCert := range r.GetResult().Svid.CertChain {
Copy link
Member

Choose a reason for hiding this comment

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

we have a function for this under x509util, i believe

Comment on lines 70 to 71
createNewAgentClient func(*grpc.ClientConn) agent.AgentClient
createNewBundleClient func(*grpc.ClientConn) bundle.BundleClient
Copy link
Member

Choose a reason for hiding this comment

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

this will need tweaking with the grpc update that just got merged (to swap *grpc.ClientConn for `grpc.ClientConnInterface)

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
azdagron
azdagron previously approved these changes Jul 28, 2020
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

This looks good! I've approved with one minor nit if you feel like changing it.

Comment on lines +181 to +182
CreateNewAgentClient: agent.NewAgentClient,
CreateNewBundleClient: bundle.NewBundleClient,
Copy link
Member

Choose a reason for hiding this comment

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

nit: one thing we can do to make this harder to misconfigure is to have the attestor default to agent.NewAgentClient and bundle.NewBundleClient if these functions are unset in the configuration.

…tions

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

\o/

@amartinezfayo amartinezfayo merged commit 70f0f58 into spiffe:master Jul 28, 2020
@amartinezfayo amartinezfayo deleted the api-refactor-enable-attest-agent branch July 28, 2020 17:33
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.

None yet

3 participants