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

Use newer x/sys/windows SecurityAttributes struct #40017

Closed
wants to merge 1 commit into from

Conversation

zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Sep 30, 2019

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

@thaJeztah
Copy link
Member

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

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
Copy link
Member

@tklauser @jterry75 PTAL

@thaJeztah
Copy link
Member

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
Copy link
Contributor Author

zx2c4 commented Sep 30, 2019

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
Copy link
Contributor Author

zx2c4 commented Sep 30, 2019

@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
Copy link
Contributor

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

@thaJeztah
Copy link
Member

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
Copy link
Member

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 <Jason@zx2c4.com>
@zx2c4
Copy link
Contributor Author

zx2c4 commented Sep 30, 2019

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
Copy link
Member

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
Copy link
Member

carrying in #40021

@zx2c4 zx2c4 closed this Oct 1, 2019
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jan 7, 2020
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 <github@gone.nl>
docker-jenkins pushed a commit to docker/docker-ce that referenced this pull request Jan 9, 2020
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 <github@gone.nl>
Upstream-commit: 627a4cf7ccd0b7e92c6798c73de4dd4efc43175c
Component: cli
eiffel-fl pushed a commit to eiffel-fl/cli that referenced this pull request Jul 28, 2020
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 <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants