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

Validate VSphere User Privs #2907

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

jonathanmeier5
Copy link
Member

@jonathanmeier5 jonathanmeier5 commented Aug 9, 2022

Issue #, if available:
#2744

Description of changes:

Add a preflight check to validate vSphere user's permissions.

Testing (if applicable):

I have tested creating clusters using the permissions I defined following our docs. I'm concerned that my test environment might not reflect real world use cases in some way that I am not aware of.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@eks-distro-bot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@eks-distro-bot eks-distro-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 9, 2022
}

func NewValidator(govc ProviderGovcClient, netClient networkutils.NetClient) *Validator {
func NewValidator(govc ProviderGovcClient, netClient networkutils.NetClient, f NewVSphereClientCallable) *Validator {
Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like I might be abusing golang here. I want a way to do the following:

  • inject a vsphere client mock when testing
  • when not testing, I want to automatically specify a sane default vsphere client

Is there a better way to achieve this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have a look at this, might be some inspo: #2887

"privsContent": vsphereAdminPrivsFile,
"path": controlPlaneMachineConfig.Spec.Folder,
},
// Not implementing a check on the template because I'm not sure if we always require/provide/allow template permissions
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand what this section means:
https://anywhere.eks.amazonaws.com/docs/reference/vsphere/vsphere-preparation/#deploy-an-ova-template

Does it mean that a user doesn't necessarily have to have access to the templates folder?

Copy link
Member Author

Choose a reason for hiding this comment

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

This validation can be conditional. If the template field in vsphere config object has been configured, we can skip the validation.

If we do apply the validation, we need to do so on:

  • control plane template directory
  • worker node template directory
  • external etcd directory

Append(sharedExtraArgs)
controllerManagerExtraArgs := clusterapi.SecureTlsCipherSuitesExtraArgs().
Append(clusterapi.NodeCIDRMaskExtraArgs(&clusterSpec.Cluster.Spec.ClusterNetwork))
type VSphereUserConfig struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

I bundled these things together because we were reading from env vars all through the code. I was hoping to consolidate the config into some kind of object we could pass around instead. Eventually, I'd love to move these configs into the CliConfig object.

As is, I can move this into the config/ package.

Comment on lines 434 to 448
ctrl := gomock.NewController(t)
vSphereClientCallable := func(ctx context.Context, host string, username string, password string, insecure bool, datacenter string) (VSphereClient, error) {
vsc := mocks.NewMockVSphereClient(ctrl)
vsc.EXPECT().Username().Return("foobar").AnyTimes()

var privs []string
err := json.Unmarshal([]byte(vsphereAdminPrivsFile), &privs)
if err != nil {
return nil, err
}

vsc.EXPECT().GetPrivsOnEntity(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(privs, nil).AnyTimes()
return vsc, nil
}
validator := NewValidator(govc, netClient, vSphereClientCallable)
Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like this code is messy. I needed a way to inject a dummy vsphere client into the validator for my tests. I am extremely open to alternative suggestions.

@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #2907 (ce4181a) into main (1cb67f1) will increase coverage by 0.46%.
The diff coverage is 83.71%.

❗ Current head ce4181a differs from pull request most recent head 7a9065b. Consider uploading reports for the commit 7a9065b to get more accurate results

@@            Coverage Diff             @@
##             main    #2907      +/-   ##
==========================================
+ Coverage   62.25%   62.72%   +0.46%     
==========================================
  Files         334      339       +5     
  Lines       26865    27235     +370     
==========================================
+ Hits        16724    17082     +358     
+ Misses       8857     8847      -10     
- Partials     1284     1306      +22     
Impacted Files Coverage Δ
pkg/executables/executables.go 7.14% <ø> (ø)
pkg/govmomi/authorizationmanagerbuilder.go 0.00% <0.00%> (ø)
pkg/govmomi/finderbuilder.go 0.00% <0.00%> (ø)
pkg/govmomi/sessionbuilder.go 0.00% <0.00%> (ø)
pkg/govmomi/clientbuilder.go 56.00% <56.00%> (ø)
pkg/providers/vsphere/envars.go 43.75% <66.66%> (ø)
pkg/providers/vsphere/vsphere.go 65.81% <76.31%> (+0.84%) ⬆️
pkg/govmomi/client.go 85.13% <85.13%> (ø)
pkg/providers/vsphere/validator.go 73.21% <89.75%> (+11.53%) ⬆️
pkg/config/vsphereuser.go 100.00% <100.00%> (ø)
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

logger.MarkPass("EKSA_VSPHERE_USER vSphere privileges validated")
}

if len(vuc.EksaVsphereCPUsername) > 0 {
Copy link
Member Author

Choose a reason for hiding this comment

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

Verify that CP/CSI values exist and usernames are different from EKSA_VSPHERE_USERNAME.

"privsContent": config.VSphereCnsVmPrivsFile,
"path": controlPlaneMachineConfig.Spec.Folder,
},
// CNS-HOST-CONFIG-STORAGE I don't know what this applies to yet
Copy link
Member Author

Choose a reason for hiding this comment

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

Ask Vignesh or Abhinav about where this should be applied.

Copy link
Member Author

@jonathanmeier5 jonathanmeier5 Aug 15, 2022

Choose a reason for hiding this comment

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

Based on a conversation with Abhinav I think this role only needs to be applied to the datastore.

@jonathanmeier5 jonathanmeier5 force-pushed the vsphere-priv-check branch 7 times, most recently from 10221e1 to fa20e5c Compare August 16, 2022 21:16
@jonathanmeier5 jonathanmeier5 self-assigned this Aug 16, 2022
Copy link
Member

@vivek-koppuru vivek-koppuru left a comment

Choose a reason for hiding this comment

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

I think I understand the validations overall and seems like the right approach. Let's make sure that we are able to test it against our vcenter as well.

)

const (
EksavSphereUsernameKey = "EKSA_VSPHERE_USERNAME"
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these constants defined elsewhere already?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like they already existed in two places:

vSphereUsernameKey = "EKSA_VSPHERE_USERNAME"

EksavSphereUsernameKey = "EKSA_VSPHERE_USERNAME"

It's also defined in many different test files. I feel like a good first step is making executables/govc import and use the const from config to button things up, and we can get the test usages later?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a case where we need to keep the validation of env vars in govc somehow and pass in parameters to the check setup method to not have multiple instances of these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I've filed concerns in a ticket here.

}

//go:embed static/globalPrivs.json
var VSphereGlobalPrivsFile string
Copy link
Member

Choose a reason for hiding this comment

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

We could also read the entire folder as a filesystem and read them at the time that we are doing the specific validation. This is okay but makes it one more place to add if we need to add a new group of privs

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually did that originally, but it meant I didn't get linting errors if a particular file wasn't present. I'm kind of paranoid about forgetting to bundle static assets in builds because python makes it suuuper easy to shoot yourself in the foot this way when creating packages.

Agreed it's not great to need to add each new file here.

var VSphereReadOnlyPrivs string

func NewVsphereUserConfig() *VSphereUserConfig {
eksaVsphereUsername := os.Getenv(EksavSphereUsernameKey)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to do validations of whether they are empty or not, or will the govmomi api handle it?

Copy link
Member Author

Choose a reason for hiding this comment

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

already in place 👍
#2907 (comment)

@@ -1046,8 +1046,12 @@ func (f *Factory) WithVSphereValidator() *Factory {
if f.dependencies.VSphereValidator != nil {
return nil
}

f.dependencies.VSphereValidator = vsphere.NewValidator(f.dependencies.Govc, &networkutils.DefaultNetClient{})
vb := vsphere.NewValidatorBuilder(
Copy link
Member

Choose a reason for hiding this comment

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

Take a look at what we adopted now, we decided not to use a validator builder but instead pull the clients as part of the validator methods: #2887

if err := os.Setenv(vSpherePasswordKey, vSpherePassword); err != nil {
return fmt.Errorf("unable to set %s: %v", EksavSpherePasswordKey, err)
return fmt.Errorf("unable to set %s: %v", config.EksavSpherePasswordKey, err)
Copy link
Member

@vivek-koppuru vivek-koppuru Aug 18, 2022

Choose a reason for hiding this comment

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

Is this where the above env vars get validated?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup 👍

}

func (vsc *VMOMIClient) GetPrivsOnEntity(ctx context.Context, path string, objType string, username string) ([]string, error) {
var objRef types.ManagedObjectReference
Copy link
Member

Choose a reason for hiding this comment

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

What does this object ref refer to?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be a ref to an object in vsphere. I've tried renaming it, let me know if you think it's clearer?

Copy link
Member

Choose a reason for hiding this comment

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

What is the official name that vcenter uses? I might like resource more otherwise

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong opinion here and am happy to go with whatever you think is best.

In general, I really don't like "object" for variable names because it's so vague. At the same time, golang's naming conventions go against a lot of what I'm used to, so I've been tossing my personal preferences out the window lately. 😅

I unfortunately do see "object" in a lot of the docs, but again am happy to go with whatever you think is best:
https://docs.vmware.com/en/VMware-vSphere/7.0/com.vmware.vsphere.vcenterhost.doc/GUID-031BDB12-D3B2-4E2D-80E6-604F304B4D0C.html

https://docs.vmware.com/en/VMware-vSphere/7.0/com.vmware.vsphere.vcenterhost.doc/GUID-7FDFBDBE-F8AC-4D00-AE5E-3F14D7472FAF.html

https://docs.vmware.com/en/VMware-vSphere/7.0/com.vmware.vsphere.vcenterhost.doc/GUID-E8E854DD-AA97-4E0C-8419-CE84F93C4058.html

I am guessing that these persistent references to "objects" in the govmomi code derive from the "object" language in the vsphere docs.

return nil
}

func diffPrivs(requiredPrivs []string, hasPrivs []string) []string {
Copy link
Member

Choose a reason for hiding this comment

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

Diff makes it sound like we are running a diff, but this is almost a subset of a diff imo

Suggested change
func diffPrivs(requiredPrivs []string, hasPrivs []string) []string {
func checkRequiredPrivs(requiredPrivs []string, hasPrivs []string) []string {

Copy link
Member Author

Choose a reason for hiding this comment

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

committed locally 👍

Copy link
Member

@vivek-koppuru vivek-koppuru left a comment

Choose a reason for hiding this comment

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

Left some more comments, let me know what you think

v := vsphere.NewValidator(
f.dependencies.Govc,
&networkutils.DefaultNetClient{},
&vsphere.VMOMIClientBuilder{},
Copy link
Member

Choose a reason for hiding this comment

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

Even tho this doesn't contain anything, I would prefer creating a constructor for it. That way, this struct can be unexported with the methods being the only ones exported

if err := p.validator.validateCSIUserPrivs(ctx, vSphereClusterSpec, vuc); err != nil {
return err
} else {
logger.MarkPass("EKSA_VSPHERE_CSI_USER vSphere privileges validated")
Copy link
Member

Choose a reason for hiding this comment

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

So the CSI and CP user privs are just a subset of the main user? No other new ones here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The documentation mentions a few permissions that are not included in the set assigned to EKSA_VSPHERE_USERNAME per our docs. However, I was not able to find anything broken in the driver when deploying without them. Presumably the majority of our users have been deploying without them and we haven't heard any complaints?

Aside from those one or two perms, the CSI and CP user privs are just subsets of EKSA_VSPHERE_USER's perms.

I think in this situation I'd rather the validator be more permissive than restrictive until we can find an example of the software breaking without the permissions. What do you think?

@@ -0,0 +1,152 @@
package vsphere
Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe move this to pkg/govmomi/client.go?

FetchUserPrivilegeOnEntities(ctx context.Context, entities []types.ManagedObjectReference, userName string) ([]types.UserPrivilegeResult, error)
}

type VSphereClient interface {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look like we need this interface

Copy link
Member Author

@jonathanmeier5 jonathanmeier5 Aug 22, 2022

Choose a reason for hiding this comment

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

I generate a mock object off of it for tests in vsphere_test.go.

vsc := mocks.NewMockVSphereClient(ctrl)

After refactoring to pkg/govmomi/client.go it will look a bit different, but I think I still want that mock object.

Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if we are interfacing too much here but I can't think of an alternate solution except for maybe passing in values like datacenter on each validate command so we can potentially remove the need of a builder. We can brainstorm maybe as I want to avoid overcomplicating and it getting stuck in code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm. Yeah let's brain storm in office hours today?


type VMOMIClientBuilder struct{}

func (*VMOMIClientBuilder) Build(ctx context.Context, host string, username string, password string, insecure bool, datacenter string) (VSphereClient, error) {
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to unit test this function, even if we are mocking some of the calls. The govmomi.NewClient call should be able to create a client with fake creds right? If not, that might be something we need to create a builder interface for and inject into the constructor of this client builder.

Copy link
Member Author

@jonathanmeier5 jonathanmeier5 Aug 22, 2022

Choose a reason for hiding this comment

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

Yeah, I've been going back and forth on this. The test will just end up verifying that a bunch of mocks are called correctly. It seemed like a lot of extra code to test ~15loc, but you're right that we should have something around it.

Also has the benefit of pushing us towards structuring the builder better with a DI approach.

Comment on lines 141 to 146
f := fields{
AuthorizationManager: am,
Finder: finder,
Path: tt.path,
}
tt.prepare(&f)
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
f := fields{
AuthorizationManager: am,
Finder: finder,
Path: tt.path,
}
tt.prepare(&f)
f := &fields{
AuthorizationManager: am,
Finder: finder,
Path: tt.path,
}
tt.prepare(f)

Comment on lines 157 to 160
t.Fatal("Missing error")
} else if !tt.wantErr && !reflect.DeepEqual(privs, wantPrivs) {
t.Fatalf("privs = %v, want %v", privs, wantPrivs)
}
Copy link
Member

Choose a reason for hiding this comment

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

We can probably make use of gomega library here with the assertions. An example that I worked on recently: https://github.com/aws/eks-anywhere/blob/main/pkg/providers/snow/reconciler/clientbuilder_test.go#L117-L121

},
}

var newRas []RequiredAccess
Copy link
Member

Choose a reason for hiding this comment

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

What does newRas mean? Maybe we need to modify the naming here between ras and newRas?

Copy link
Member Author

Choose a reason for hiding this comment

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

These names are so bad, haha. I'll make them better.

}

func checkRequiredPrivs(requiredPrivs []string, hasPrivs []string) []string {
hp := map[string]int{}
Copy link
Member

Choose a reason for hiding this comment

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

I see that we are using this as a hashset basically. I would suggest using struct{} or interface{} to show that there is no meaning to the value of the map, and it also doesn't take up any memory.

Suggested change
hp := map[string]int{}
hp := map[string]interface{}

@@ -149,22 +142,34 @@ type ClusterResourceSetManager interface {
ForceUpdate(ctx context.Context, name, namespace string, managementCluster, workloadCluster *types.Cluster) error
}

type ValidatorBuilder interface {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to avoid a ValidatorBuilder now that #2887 is merged?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh i meant to strip this out, yes 👍

if err := p.validator.validateUserPrivs(ctx, vSphereClusterSpec, vuc); err != nil {
return err
} else {
logger.MarkPass("EKSA_VSPHERE_USER vSphere privileges validated")
Copy link
Member

Choose a reason for hiding this comment

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

All of these should probably reference "USERNAME" if that's how we documented and read them, and then maybe we can say the following instead. Or better if we can just output the value of the env var below instead

Suggested change
logger.MarkPass("EKSA_VSPHERE_USER vSphere privileges validated")
logger.MarkPass("EKSA_VSPHERE_USERNAME user vSphere privileges validated")

if err := p.validator.validateUserPrivs(ctx, vSphereClusterSpec, vuc); err != nil {
return err
} else {
logger.MarkPass("EKSA_VSPHERE_USERNAME vSphere privileges validated")
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Let's print the values of this env var and add "user" after that name

@jonathanmeier5
Copy link
Member Author

/approve

@eks-distro-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonathanmeier5

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@eks-distro-bot eks-distro-bot merged commit 3a86f86 into aws:main Aug 23, 2022
jonathanmeier5 added a commit that referenced this pull request Aug 23, 2022
eks-distro-bot pushed a commit that referenced this pull request Aug 24, 2022
jonathanmeier5 added a commit to jonathanmeier5/eks-anywhere that referenced this pull request Aug 24, 2022
eks-distro-bot pushed a commit that referenced this pull request Aug 25, 2022
* Revert "Revert "Add vSphere priv check (#2907)" (#3136)"

This reverts commit 3ca04dd.

* Validate vmc privs with warning
eks-distro-pr-bot pushed a commit to eks-distro-pr-bot/eks-anywhere that referenced this pull request Aug 26, 2022
eks-distro-bot pushed a commit that referenced this pull request Aug 26, 2022
* Revert "Revert "Add vSphere priv check (#2907)" (#3136)"

This reverts commit 3ca04dd.

* Validate vmc privs with warning

Co-authored-by: Jonathan Meier <jwmeier@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants