Enable agent path template customization for azure_msi node attestor plugin #3488

guilhermocc · 2022-10-06T19:04:26Z

Pull Request check list

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

Affected functionality

Added the ability to use a custom agent path template for azure_msi server node attestor plugin.

Description of change

Keep the actual azure_msi agent SPIFFE ID form as the default to maintain compatibility
azure_msi plugin now accepts a new optional configuration agent_path_template that is used for building the agent's SPIFFE ID.
Added missing documentation for gcp_iit plugin

Which issue this PR fixes
fixes #3420

…e_msi plugin spiffe#3420 Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

azdagron · 2022-10-10T15:01:57Z

Thanks for opening this, @guilhermocc ! I'll see if I can get it reviewed here shortly. Just for information, you don't need to bother keeping the branch up to date. Once it has been approved, we (the maintainers) will update it as needed.

azdagron

Thanks @guilhermocc! Just a few comments.

pkg/common/plugin/azure/msi.go

azdagron · 2022-10-12T16:27:31Z

pkg/common/plugin/azure/msi.go

-	}
-	return u.String()
+	TenantID    string `json:"tid,omitempty"`
+	PrincipalID string `json:"sub,omitempty"`


I did a double take and had to remind myself what happens with the JSON decoder when there are two fields with the same tag. Fortunately the top level field takes precedence over the embedded struct but this is going to be a little subtle if there is any code that tries to use the .Subject field (since it will be unset). Should be easy enough to catch in testing though. I think we're ok here.

I thought about creating a new struct for only storing claims/fields that we want to expose and use. But we would still have another struct being composed by jwt.Claims only, since we use the ValidateWithLeeway method for validation, but I'm not sure if that would make the code clearer since we will be going to deserialize the token two times for two different structs, what do you think?

I think we're ok with it as-is. If the Subject field is used at all the unit-tests should catch that it is empty.

Co-authored-by: Andrew Harding <azdagron@gmail.com> Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

MarcosDY · 2022-10-13T15:03:40Z

pkg/common/plugin/azure/msi_test.go

+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			td, _ := spiffeid.TrustDomainFromString(test.args.td)


Suggested change

td, _ := spiffeid.TrustDomainFromString(test.args.td)

td := spiffeid.RequireTrustDomainFromString(test.args.td)

MarcosDY · 2022-10-13T15:05:24Z

pkg/common/plugin/azure/msi_test.go

+		{
+			name: "successfully applies template",
+			args: args{
+				"example.org",


can you set far names in all cases you create a new struct? it can resutl in unexpected errors if we choose to add more fields

MarcosDY · 2022-10-13T15:06:18Z

pkg/common/plugin/azure/msi_test.go

+		name    string
+		args    args
+		want    string
+		wantErr bool


instead of want error, can you instead validate the error you are getting? it is possible to get into situations where you get another error and not the expected one

MarcosDY · 2022-10-13T15:32:50Z

pkg/server/plugin/nodeattestor/azuremsi/msi_test.go

+	var selectorValues []string
+	selectorValues = append(selectorValues, vmSelectors...)


Suggested change

var selectorValues []string

selectorValues = append(selectorValues, vmSelectors...)

selectorValues := append([]string(nil), vmSelectors...)

MarcosDY · 2022-10-13T15:43:58Z

doc/plugin_server_nodeattestor_azure_msi.md

+| Configuration         | Required | Description                                                                                                             | Default                                                   |
+|-----------------------|----------|-------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------|
+| `tenants`             | Required | A map of tenants, keyed by tenant ID, that are authorized for attestation. Tokens for unspecified tenants are rejected. |                                                           |
+| `agent_path_template` | Optional | A URL path portion format of Agent's SPIFFE ID. Describe in text/template format.                                       | `"/{{ .PluginName }}/{{ .TenantID }}/{{ .PrincipalID }}"` |


can you update server_full.conf, with this new configuration?

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

MarcosDY

LGTM!!!

…plugin (spiffe#3488) Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc added 4 commits October 6, 2022 15:34

Enable the usage of agent path template for building agent id in azur…

d47c456

…e_msi plugin spiffe#3420 Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Update azure_msi server node attestor documentation spiffe#3420

c799d2c

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Update gcp_iit server node attestor documentation

feb7c06

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Reformat node attestor plugins documentation tables

b2288d1

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc requested review from evan2645, amartinezfayo, azdagron, MarcosDY and rturner3 as code owners October 6, 2022 19:04

guilhermocc added 3 commits October 6, 2022 18:52

Merge branch 'main' into enable-agent-path-template-azuremsi-nattestor

81a64ae

Merge branch 'main' into enable-agent-path-template-azuremsi-nattestor

6c27162

Merge branch 'main' into enable-agent-path-template-azuremsi-nattestor

054c361

azdagron added this to the 1.5.0 milestone Oct 7, 2022

Merge branch 'main' into enable-agent-path-template-azuremsi-nattestor

4077887

evan2645 assigned azdagron Oct 11, 2022

azdagron reviewed Oct 12, 2022

View reviewed changes

Update pkg/common/plugin/azure/msi.go

d9c540f

Co-authored-by: Andrew Harding <azdagron@gmail.com> Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc force-pushed the enable-agent-path-template-azuremsi-nattestor branch from fa1ff86 to d9c540f Compare October 13, 2022 13:01

Fix lint issues

430a6c6

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc force-pushed the enable-agent-path-template-azuremsi-nattestor branch from b038635 to 430a6c6 Compare October 13, 2022 13:20

azdagron previously approved these changes Oct 13, 2022

View reviewed changes

Merge branch 'main' into enable-agent-path-template-azuremsi-nattestor

3066e24

MarcosDY reviewed Oct 13, 2022

View reviewed changes

Improve MakeAgentID test assertion

5cef0ef

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc dismissed azdagron’s stale review via 5cef0ef October 13, 2022 19:16

guilhermocc added 2 commits October 13, 2022 16:31

Add new azure plugin option in server_full.conf

7575ad5

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Remove white lines from imports

ab6f893

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc requested a review from MarcosDY October 13, 2022 19:45

MarcosDY approved these changes Oct 14, 2022

View reviewed changes

guilhermocc requested a review from azdagron October 17, 2022 16:11

Merge branch 'main' into enable-agent-path-template-azuremsi-nattestor

327af58

azdagron merged commit 50d677f into spiffe:main Oct 19, 2022

stevend-uber pushed a commit to stevend-uber/spire that referenced this pull request Oct 16, 2023

Enable agent path template customization for azure_msi node attestor …

6890072

…plugin (spiffe#3488) Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable agent path template customization for azure_msi node attestor plugin #3488

Enable agent path template customization for azure_msi node attestor plugin #3488

guilhermocc commented Oct 6, 2022 •

edited

Loading

azdagron commented Oct 10, 2022

azdagron left a comment

azdagron Oct 12, 2022

guilhermocc Oct 13, 2022 •

edited

Loading

azdagron Oct 13, 2022

MarcosDY Oct 13, 2022 •

edited

Loading

MarcosDY Oct 13, 2022

MarcosDY Oct 13, 2022

MarcosDY Oct 13, 2022

MarcosDY Oct 13, 2022

guilhermocc Oct 13, 2022

MarcosDY left a comment

	td, _ := spiffeid.TrustDomainFromString(test.args.td)
	td := spiffeid.RequireTrustDomainFromString(test.args.td)

		var selectorValues []string
		selectorValues = append(selectorValues, vmSelectors...)

	var selectorValues []string
	selectorValues = append(selectorValues, vmSelectors...)
	selectorValues := append([]string(nil), vmSelectors...)

Enable agent path template customization for azure_msi node attestor plugin #3488

Enable agent path template customization for azure_msi node attestor plugin #3488

Conversation

guilhermocc commented Oct 6, 2022 • edited Loading

azdagron commented Oct 10, 2022

azdagron left a comment

Choose a reason for hiding this comment

azdagron Oct 12, 2022

Choose a reason for hiding this comment

guilhermocc Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

azdagron Oct 13, 2022

Choose a reason for hiding this comment

MarcosDY Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

MarcosDY Oct 13, 2022

Choose a reason for hiding this comment

MarcosDY Oct 13, 2022

Choose a reason for hiding this comment

MarcosDY Oct 13, 2022

Choose a reason for hiding this comment

MarcosDY Oct 13, 2022

Choose a reason for hiding this comment

guilhermocc Oct 13, 2022

Choose a reason for hiding this comment

MarcosDY left a comment

Choose a reason for hiding this comment

guilhermocc commented Oct 6, 2022 •

edited

Loading

guilhermocc Oct 13, 2022 •

edited

Loading

MarcosDY Oct 13, 2022 •

edited

Loading