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 GMSA support for V2 HCS schema xenon containers #856

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Jul 28, 2020

  • Add new UVM function 'UpdateHvSockServiceTable' to be able to hot add
    Hvsocket service table entries.
  • Add disabled field to HvSocketServiceConfig (used to be private in the schema)
  • Remove hardcoded error if supplying a cred spec and the client asked for a
    hypervisor isolated container.
  • Misc refactors (comments, style)

Signed-off-by: Daniel Canter [email protected]

@dcantah dcantah requested a review from a team July 28, 2020 02:10
@dcantah
Copy link
Contributor Author

dcantah commented Jul 28, 2020

Draft just to get feedback/OS changes get in. Pretty simple change.

@dcantah
Copy link
Contributor Author

dcantah commented Jul 28, 2020

Looks like we want hot-remove as well so this will be updated quite a bit more

@dcantah
Copy link
Contributor Author

dcantah commented Aug 1, 2020

Ok after investigating further with Nitin, hot-remove is not actually needed for this scenario. A ccg.exe process gets launched on the host per unique credential spec to host the hvsocket service. If the same credential spec is used multiple times an internal ref of how many there are is kept and when it reaches 0, the ccg process exits and brings the service down with it automatically, no need to keep a ref on our end and emulate this. The hot-remove changes are still being added as there's no reason to not include them if we find a use case for the future.

@dcantah dcantah marked this pull request as ready for review August 5, 2020 11:54
@dcantah dcantah force-pushed the hyperv-gmsa branch 2 times, most recently from bc1b75a to 868f615 Compare August 5, 2020 12:03
@dcantah dcantah requested a review from a team August 5, 2020 19:11
@dcantah dcantah force-pushed the hyperv-gmsa branch 2 times, most recently from c3f3d17 to c3619fe Compare August 11, 2020 17:47
Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

LGTM

@dcantah
Copy link
Contributor Author

dcantah commented Aug 12, 2020

@kevpar When would we want to merge this in, when it gets far enough back in the backporting cycle or? Nitins waiting for a build to finish but merging the OS changes right after.

@dcantah
Copy link
Contributor Author

dcantah commented Oct 22, 2020

@kevpar Added a build check. Could you give this another peek?

coi.ccgState = ccgInstance.CredentialGuard
r.Add(ccgResource)
if hypervisorIsolated {
if osversion.Get().Build < 19041 {
Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline, but need to add a better check here to determine if the support for hv-iso GMSA is present in the build.

Are there fixes in the guest that we rely on as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the hvsocket functionality no not that I'm aware of. The container.dll fix will be needed in the image for smb access to work but thats not related to this functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevpar So the plan seems like just remove the check and just document what build this will work on?

@@ -1,8 +1,5 @@
// +build windows

// Package credentials holds the necessary structs and functions for adding
Copy link
Member

Choose a reason for hiding this comment

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

Are we losing our package doc comment? Those are really nice to have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha commented the same below. Must have gotten removed during rebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevpar done

@@ -1,8 +1,5 @@
// +build windows

// Package credentials holds the necessary structs and functions for adding
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just now realizing this got removed for some reason..

* Add new UVM function 'UpdateHvSocketService' to be able to hot add
Hvsocket service table entries.
* Add new UVM function 'RemoveHvSocketService' to be able to hot remove
an Hvsocket service.
* Add disabled field to HvSocketServiceConfig (used to be private in the schema)
* Remove hardcoded error if supplying a cred spec and the client asked for a
hypervisor isolated container.
* Misc refactors (comments, style)

Signed-off-by: Daniel Canter <[email protected]>
Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

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