Use newer x/sys/windows SecurityAttributes struct by zx2c4 · Pull Request #40017 · moby/moby

zx2c4 · 2019-09-30T15:08:53Z

This struct now has a properly typed member, so use the properly typed functions with it.

thaJeztah · 2019-09-30T15:51:46Z

Thanks! I see winio.SddlToSecurityDescriptor() is also used in

Line 70 in ad1b781

securityDescriptor, err := winio.SddlToSecurityDescriptor(sddlString)

Haven't looked closely, but perhaps that one can also be replaced?

Perhaps it's worth to open a pull-request in the https://github.com/Microsoft/go-winio repository to mark SddlToSecurityDescriptor as deprecated, and make it an alias for the /x/sys implementation (or at least mark it deprecated, so that people that use the go-winio package get notified that they should replace it

thaJeztah · 2019-09-30T15:51:59Z

@tklauser @jterry75 PTAL

thaJeztah · 2019-09-30T16:15:34Z

Looks like there's an import that has to be removed, and perhaps the vendored /x/sys needs to be updated to a more current version;


[2019-09-30T15:24:13.154Z] pkg/system/filesys_windows.go:14:2: imported and not used: "github.com/docker/docker/vendor/github.com/Microsoft/go-winio" as winio
[2019-09-30T15:24:13.154Z] pkg/system/filesys_windows.go:106:13: undefined: "github.com/docker/docker/vendor/golang.org/x/sys/windows".SecurityDescriptorFromString

zx2c4 · 2019-09-30T16:54:01Z

Perhaps it's worth to open a pull-request

Already done.

Haven't looked closely, but perhaps that one can also be replaced?

It could but it's not necessary since all the things its passed to treat it as an array of bytes.

Looks like there's an import that has to be removed, and perhaps the vendored /x/sys needs to be updated to a more current version;

Will fix.

zx2c4 · 2019-09-30T16:58:44Z

@thaJeztah Force-pushed. I haven't actually looked how to build this, so if it fails again, you can take over the PR if you want.

jterry75 · 2019-09-30T17:18:43Z

go-winio PR here: microsoft/go-winio#165

thaJeztah · 2019-09-30T17:23:42Z

Already done.
go-winio PR here: microsoft/go-winio#165

perfect! thanks!

@thaJeztah Force-pushed. I haven't actually looked how to build this, so if it fails again, you can take over the PR if you want.

No worries; for the vendored files, the vendor.conf needs to be updated to point to the right commit, otherwise the vendor check will fail in CI; I can push to this branch to make those changes.

thaJeztah · 2019-09-30T17:24:47Z

Looks like there's one more change needed;

# github.com/docker/docker/builder/dockerfile
builder/dockerfile/internals_windows.go:34:20: assignment mismatch: 2 variables but sid.String returns 1 values
builder/dockerfile/internals_windows.go:49:19: assignment mismatch: 2 variables but sid.String returns 1 values

This struct now has a properly typed member, so use the properly typed functions with it. Also update the vendor directory and hope nothing explodes. Signed-off-by: Jason A. Donenfeld <[email protected]>

zx2c4 · 2019-09-30T17:29:19Z

Looks like there's one more change needed;

# github.com/docker/docker/builder/dockerfile
builder/dockerfile/internals_windows.go:34:20: assignment mismatch: 2 variables but sid.String returns 1 values
builder/dockerfile/internals_windows.go:49:19: assignment mismatch: 2 variables but sid.String returns 1 values

All set.

thaJeztah · 2019-09-30T18:43:04Z

almost; 😂 🤷‍♂

# github.com/docker/docker/builder/dockerfile
builder/dockerfile/internals_windows.go:15:2: imported and not used: "github.com/docker/docker/vendor/github.com/pkg/errors"

let me carry the changes; I think I'll separate the vendor bump from the local changes in case we want to backport (which could make it slightly easier)

thaJeztah · 2019-09-30T19:03:43Z

carrying in #40021

full diff: moby/moby@a09e6e3...a9507c6 Includes: - moby/moby#40077 Update "auto-generate" comments to improve detection by linters - moby/moby#40143 registry: add a critical section to protect authTransport.modReq - moby/moby#40212 Move DefaultCapabilities() to caps package - moby/moby#40021 Use newer x/sys/windows SecurityAttributes struct (carry 40017) - carries moby/moby#40017 Use newer x/sys/windows SecurityAttributes struct - moby/moby#40135 pkg/system: make OSVersion an alias for hcsshim OSVersion - follow-up to moby/moby#39100 Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion() - moby/moby#40250 Bump hcsshim to b3f49c06ffaeef24d09c6c08ec8ec8425a0303e2 - moby/moby#40243 Use certs.d from XDG_CONFIG_HOME when in rootless mode - fixes moby/moby#40236 Docker rootless dies when unable to read /etc/docker/certs.d - moby/moby#40283 Fix possible runtime panic in Lgetxattr - moby/moby#40178 builder/remotecontext: small refactor - moby/moby#40179 builder/remotecontext: allow ssh:// for remote context URLs - fixes docker#2164 Docker build cannot resolve git context with html escapes - moby/moby#40302 client.ImagePush(): default to ":latest" instead of "all tags" - relates to docker#2214 [proposal] change "docker push" behavior to default to ":latest" instead of "all tags" - relates to docker#2220 implement docker push `-a`/ `--all-tags` - moby/moby#40263 Normalize comment formatting - moby/moby#40238 Allow client consumers like traefik to compile on illumos - moby/moby#40108 bump google.golang.org/grpc v1.23.1 - moby/moby#40312 update vendor golang.org/x/sys to 6d18c012aee9febd81bbf9806760c8c4480e870d - moby/moby#40247 pkg/system: deprecate constants in favor of golang.org/x/sys/windows - moby/moby#40246 pkg/system: minor cleanups and remove use of deprecated system.GetOSVersion() - moby/moby#40122 Update buildkit to containerd leases - vendor: update buildkit to leases support (4f4e03067523b2fc5ca2f17514a5e75ad63e02fb) - vendor: update containerd to acdcf13d5eaf0dfe0eaeabe7194a82535549bc2b - vendor: update runc to d736ef14f0288d6993a1845745d6756cfc9ddd5a (v1.0.0-rc9) Signed-off-by: Sebastiaan van Stijn <[email protected]>

full diff: moby/moby@a09e6e3...a9507c6 Includes: - moby/moby#40077 Update "auto-generate" comments to improve detection by linters - moby/moby#40143 registry: add a critical section to protect authTransport.modReq - moby/moby#40212 Move DefaultCapabilities() to caps package - moby/moby#40021 Use newer x/sys/windows SecurityAttributes struct (carry 40017) - carries moby/moby#40017 Use newer x/sys/windows SecurityAttributes struct - moby/moby#40135 pkg/system: make OSVersion an alias for hcsshim OSVersion - follow-up to moby/moby#39100 Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion() - moby/moby#40250 Bump hcsshim to b3f49c06ffaeef24d09c6c08ec8ec8425a0303e2 - moby/moby#40243 Use certs.d from XDG_CONFIG_HOME when in rootless mode - fixes moby/moby#40236 Docker rootless dies when unable to read /etc/docker/certs.d - moby/moby#40283 Fix possible runtime panic in Lgetxattr - moby/moby#40178 builder/remotecontext: small refactor - moby/moby#40179 builder/remotecontext: allow ssh:// for remote context URLs - fixes docker/cli#2164 Docker build cannot resolve git context with html escapes - moby/moby#40302 client.ImagePush(): default to ":latest" instead of "all tags" - relates to docker/cli#2214 [proposal] change "docker push" behavior to default to ":latest" instead of "all tags" - relates to docker/cli#2220 implement docker push `-a`/ `--all-tags` - moby/moby#40263 Normalize comment formatting - moby/moby#40238 Allow client consumers like traefik to compile on illumos - moby/moby#40108 bump google.golang.org/grpc v1.23.1 - moby/moby#40312 update vendor golang.org/x/sys to 6d18c012aee9febd81bbf9806760c8c4480e870d - moby/moby#40247 pkg/system: deprecate constants in favor of golang.org/x/sys/windows - moby/moby#40246 pkg/system: minor cleanups and remove use of deprecated system.GetOSVersion() - moby/moby#40122 Update buildkit to containerd leases - vendor: update buildkit to leases support (4f4e03067523b2fc5ca2f17514a5e75ad63e02fb) - vendor: update containerd to acdcf13d5eaf0dfe0eaeabe7194a82535549bc2b - vendor: update runc to d736ef14f0288d6993a1845745d6756cfc9ddd5a (v1.0.0-rc9) Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: 627a4cf7ccd0b7e92c6798c73de4dd4efc43175c Component: cli

full diff: moby/moby@a09e6e3...a9507c6 Includes: - moby/moby#40077 Update "auto-generate" comments to improve detection by linters - moby/moby#40143 registry: add a critical section to protect authTransport.modReq - moby/moby#40212 Move DefaultCapabilities() to caps package - moby/moby#40021 Use newer x/sys/windows SecurityAttributes struct (carry 40017) - carries moby/moby#40017 Use newer x/sys/windows SecurityAttributes struct - moby/moby#40135 pkg/system: make OSVersion an alias for hcsshim OSVersion - follow-up to moby/moby#39100 Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion() - moby/moby#40250 Bump hcsshim to b3f49c06ffaeef24d09c6c08ec8ec8425a0303e2 - moby/moby#40243 Use certs.d from XDG_CONFIG_HOME when in rootless mode - fixes moby/moby#40236 Docker rootless dies when unable to read /etc/docker/certs.d - moby/moby#40283 Fix possible runtime panic in Lgetxattr - moby/moby#40178 builder/remotecontext: small refactor - moby/moby#40179 builder/remotecontext: allow ssh:// for remote context URLs - fixes docker#2164 Docker build cannot resolve git context with html escapes - moby/moby#40302 client.ImagePush(): default to ":latest" instead of "all tags" - relates to docker#2214 [proposal] change "docker push" behavior to default to ":latest" instead of "all tags" - relates to docker#2220 implement docker push `-a`/ `--all-tags` - moby/moby#40263 Normalize comment formatting - moby/moby#40238 Allow client consumers like traefik to compile on illumos - moby/moby#40108 bump google.golang.org/grpc v1.23.1 - moby/moby#40312 update vendor golang.org/x/sys to 6d18c012aee9febd81bbf9806760c8c4480e870d - moby/moby#40247 pkg/system: deprecate constants in favor of golang.org/x/sys/windows - moby/moby#40246 pkg/system: minor cleanups and remove use of deprecated system.GetOSVersion() - moby/moby#40122 Update buildkit to containerd leases - vendor: update buildkit to leases support (4f4e03067523b2fc5ca2f17514a5e75ad63e02fb) - vendor: update containerd to acdcf13d5eaf0dfe0eaeabe7194a82535549bc2b - vendor: update runc to d736ef14f0288d6993a1845745d6756cfc9ddd5a (v1.0.0-rc9) Signed-off-by: Sebastiaan van Stijn <[email protected]>

zx2c4 mentioned this pull request Sep 30, 2019

x/sys/windows: API break in SecurityAttributes struct golang/go#34610

Closed

thaJeztah added platform/windows status/2-code-review labels Sep 30, 2019

zx2c4 force-pushed the jd/blah branch from 51cf809 to 79a262f Compare September 30, 2019 16:57

zx2c4 force-pushed the jd/blah branch from 79a262f to 5bfdf0c Compare September 30, 2019 17:00

Use newer x/sys/windows SecurityAttributes struct

b7614b8

This struct now has a properly typed member, so use the properly typed functions with it. Also update the vendor directory and hope nothing explodes. Signed-off-by: Jason A. Donenfeld <[email protected]>

zx2c4 force-pushed the jd/blah branch from 5bfdf0c to b7614b8 Compare September 30, 2019 17:28

zx2c4 requested a review from tonistiigi as a code owner September 30, 2019 17:28

thaJeztah mentioned this pull request Sep 30, 2019

Use newer x/sys/windows SecurityAttributes struct (carry 40017) #40021

Merged

zx2c4 closed this Oct 1, 2019

thaJeztah mentioned this pull request Jan 7, 2020

vendor: bump docker and dependencies a9507c6f76627fdc092edc542d5a7ef4a6df5eec docker/cli#2242

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use newer x/sys/windows SecurityAttributes struct#40017

Use newer x/sys/windows SecurityAttributes struct#40017
zx2c4 wants to merge 1 commit intomoby:masterfrom
zx2c4-forks:jd/blah

zx2c4 commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

zx2c4 commented Sep 30, 2019

Uh oh!

zx2c4 commented Sep 30, 2019

Uh oh!

jterry75 commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

zx2c4 commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

zx2c4 commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

zx2c4 commented Sep 30, 2019

Uh oh!

zx2c4 commented Sep 30, 2019

Uh oh!

jterry75 commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

zx2c4 commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

thaJeztah commented Sep 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants