Skip to content

Host data validation#2518

Merged
ambarve merged 8 commits into
microsoft:mainfrom
takuro-sato:host-data-validation
Sep 17, 2025
Merged

Host data validation#2518
ambarve merged 8 commits into
microsoft:mainfrom
takuro-sato:host-data-validation

Conversation

@takuro-sato
Copy link
Copy Markdown
Contributor

@takuro-sato takuro-sato commented Sep 15, 2025

Changes

  • Add internal/pspdriver for CWCOW. (This has some duplication with pkg/amdsevsnp as commented, so that it doesn't affect current C-LCOW. We need to do refactoring later)
  • Start PSP driver
  • Use the above to validate host data.

TODOs for future PRs

Address duplication with LCOW, for now we avoid change in LCOW

Signed-off-by: Takuro Sato <takurosato@microsoft.com>
Signed-off-by: Takuro Sato <takurosato@microsoft.com>
Comment thread cmd/gcs-sidecar/main.go Outdated
Comment on lines +223 to +236
initialPolicyStance := "allow"
initialPolicyStance := "deny"
Copy link
Copy Markdown
Contributor Author

@takuro-sato takuro-sato Sep 15, 2025

Choose a reason for hiding this comment

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

The original plan was to do the following (0afe43a). Then Ken decided to always use "deny". I didn't have enough time to ask why. If anyone has major objections, I think we can stick to the original way for now, then come back to it later. At least I see less potential security problem happing by accident, while debugging might be harder.

initialPolicyStance := "allow"
	if pspdriver.GetPspDriverError() != nil || snpMode /*Value from psp driver*/ {
		// If the driver failed to start, policy should keep returning "deny" for anything.
		// For SNP environment, the initial policy is "deny" but it will be updated
		// per the user's security policy.
		initialPolicyStance = "deny"
	}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we are hardcoding the policy stance to "deny" here, wouldn't it always go to the "deny" switch case? When/how would it go to the "allow" case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. Deleted it in 43d5d74

Signed-off-by: Takuro Sato <takurosato@microsoft.com>
Signed-off-by: Takuro Sato <takurosato@microsoft.com>
@takuro-sato takuro-sato marked this pull request as ready for review September 15, 2025 15:51
@takuro-sato takuro-sato requested a review from a team as a code owner September 15, 2025 15:51
Comment thread internal/pspdriver/pspdriver.go
Comment thread internal/pspdriver/pspdriver.go Outdated
}

var (
amdsnppspapi = windows.NewLazySystemDLL("amdsnppspapi.dll")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you want to call a win32 API, please add it under internal/winapi (in a separate file if necessary) and then run go generate on that package to automatically generate the go code for calling that API. Look at other files under internal/winapi to see the format in which you would need to add your methods.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed it in 2f21484

PspStatus uint64
}

type report struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this a well-known, public type? If yes, can we include a link to the documentation here? If not, we should probably try to name these types better. For e.g Report can be ParsedReport and report can be reportData etc. with some explanation in comments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, added comment in 09a5eaf. Still we could change the name, but for now I'll leave them as they are. They are copied from the LCOW equivalent so that I don't have to change it to support WCOW. By keeping the name, later we can fix the duplication easier.

Signed-off-by: Takuro Sato <takurosato@microsoft.com>
Signed-off-by: Takuro Sato <takurosato@microsoft.com>
Signed-off-by: Takuro Sato <takurosato@microsoft.com>
Comment thread cmd/gcs-sidecar/main.go Outdated
logrus.WithError(err).Errorf("failed to start PSP driver")
}

// gcs-sidecar can be used for non-confidentail hyperv wcow
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

gcs-sidecar will not be used for non confidential WCOW, that is not in the plans right now.

To answer Amit's question. initialpolicystance can be in "deny" mode - during the first message, we set the securitypolicy and use that enforcer instead of the closed door.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated comment in affe226

Signed-off-by: Takuro Sato <takurosato@microsoft.com>
Copy link
Copy Markdown
Member

@MahatiC MahatiC left a comment

Choose a reason for hiding this comment

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

LGTM

@ambarve ambarve merged commit e437d4b into microsoft:main Sep 17, 2025
17 checks passed
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