Skip to content

removeSecure() should close the file before removing it on Windows#32948

Merged
gzdunek merged 2 commits intomasterfrom
gzdunek/close-file-in-remove-secure
Oct 4, 2023
Merged

removeSecure() should close the file before removing it on Windows#32948
gzdunek merged 2 commits intomasterfrom
gzdunek/close-file-in-remove-secure

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Oct 4, 2023

After merging #32260 removing keys on Windows is broken.

It affects logging out:

PS C:\Users\grzegorz> tsh logout --debug
2023-10-04T11:25:22+02:00 DEBU [KEYSTORE]  Reading certificates from path "C:\\Users\\grzegorz\\.tsh\\keys\\mercury.cloud.gravitational.io\\grzegorz.zdunek@goteleport.com-ssh\\mercury.cloud.gravitational.io-cert.pub". client\keystore.go:355
2023-10-04T11:25:22+02:00 DEBU [KEYSTORE]  Teleport TLS certificate valid until "2023-10-04 09:26:16 +0000 UTC". client\client_store.go:106
2023-10-04T11:25:22+02:00 INFO [CLIENT]    ALPN connection upgrade required for "mercury.cloud.gravitational.io:443": false. client\api.go:723
2023-10-04T11:25:22+02:00 INFO [CLIENT]    no host login given. defaulting to grzegorz client\api.go:1060
2023-10-04T11:25:22+02:00 ERRO [CLIENT]    [KEY AGENT] Unable to connect to SSH agent on socket: "". error:[
ERROR REPORT:
Original Error: *fs.PathError open \\.\pipe\openssh-ssh-agent: The system cannot find the file specified.
Stack Trace:
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/lib/utils/agentconn/agent_windows.go:59 github.com/gravitational/teleport/lib/utils/agentconn.Dial
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/lib/client/api.go:4556 github.com/gravitational/teleport/lib/client.connectToSSHAgent
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/lib/client/keyagent.go:135 github.com/gravitational/teleport/lib/client.NewLocalAgent
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/lib/client/api.go:1115 github.com/gravitational/teleport/lib/client.NewClient
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/tool/tsh/common/tsh.go:3381 github.com/gravitational/teleport/tool/tsh/common.makeClientForProxy
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/tool/tsh/common/tsh.go:3366 github.com/gravitational/teleport/tool/tsh/common.makeClient
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/tool/tsh/common/tsh.go:2099 github.com/gravitational/teleport/tool/tsh/common.onLogout
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/tool/tsh/common/tsh.go:1327 github.com/gravitational/teleport/tool/tsh/common.Run
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/tool/tsh/common/tsh.go:544 github.com/gravitational/teleport/tool/tsh/common.Main
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/tool/tsh/main.go:24 main.main
        C:/Drone/Workspace/build-native-windows-amd64/28944/toolchains/go/src/runtime/proc.go:267 runtime.main
        C:/Drone/Workspace/build-native-windows-amd64/28944/toolchains/go/src/runtime/asm_amd64.s:1650 runtime.goexit
User Message: open \\.\pipe\openssh-ssh-agent: The system cannot find the file specified.] client\api.go:4558
2023-10-04T11:25:22+02:00 DEBU [KEYSTORE]  Reading certificates from path "C:\\Users\\grzegorz\\.tsh\\keys\\mercury.cloud.gravitational.io\\grzegorz.zdunek@goteleport.com-ssh\\mercury.cloud.gravitational.io-cert.pub". client\keystore.go:355
2023-10-04T11:25:22+02:00 DEBU [KEYSTORE]  Teleport TLS certificate valid until "2023-10-04 09:26:16 +0000 UTC". client\client_store.go:106
2023-10-04T11:25:22+02:00 INFO [KEYAGENT]  Loading SSH key for user "grzegorz.zdunek@goteleport.com" and cluster "mercury.cloud.gravitational.io". client\keyagent.go:196
2023-10-04T11:25:22+02:00 DEBU [TSH]       Removing Teleport related entries with server '[https://mercury.cloud.gravitational.io:443](https://mercury.cloud.gravitational.io/)' from kubeconfig. common\tsh.go:2104
2023-10-04T11:25:22+02:00 DEBU [TSH]       Removing Teleport related entries for cluster 'mercury.cloud.gravitational.io' from kubeconfig. common\tsh.go:2111

ERROR REPORT:
Original Error: *trace.AccessDeniedError failed to execute command C:\Users\grzegorz\.tsh\current-profile error:  The process cannot access the file because it is being used by another process.
Stack Trace:
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/lib/utils/fs.go:326 github.com/gravitational/teleport/lib/utils.removeSecure
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/lib/utils/fs.go:265 github.com/gravitational/teleport/lib/utils.RemoveAllSecure
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/lib/client/keystore.go:299 github.com/gravitational/teleport/lib/client.(*FSKeyStore).DeleteKeys
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/lib/client/keyagent.go:589 github.com/gravitational/teleport/lib/client.(*LocalKeyAgent).DeleteKeys
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/lib/client/api.go:3262 github.com/gravitational/teleport/lib/client.(*TeleportClient).LogoutAll
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/tool/tsh/common/tsh.go:2131 github.com/gravitational/teleport/tool/tsh/common.onLogout
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/tool/tsh/common/tsh.go:1327 github.com/gravitational/teleport/tool/tsh/common.Run
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/tool/tsh/common/tsh.go:544 github.com/gravitational/teleport/tool/tsh/common.Main
        C:/Drone/Workspace/build-native-windows-amd64/28944/go/src/github.com/gravitational/teleport/tool/tsh/main.go:24 main.main
        C:/Drone/Workspace/build-native-windows-amd64/28944/toolchains/go/src/runtime/proc.go:267 runtime.main
        C:/Drone/Workspace/build-native-windows-amd64/28944/toolchains/go/src/runtime/asm_amd64.s:1650 runtime.goexit
User Message: failed to execute command C:\Users\grzegorz\.tsh\current-profile error:  The process cannot access the file because it is being used by another process.

and probably some other things.

In the original PR, Edoardo pointed out that filling the original file with random data may cause problems. I was sure that this was a source of the issue. However, it turned out that removing the renamed file still resulted in the access denied error.

Finally, I found this comment that explains that we should close the file before removing it on Windows.

Closes #32788

Changelog: Fixed issue causing keys to be incorrectly removed in tsh and Teleport Connect on Windows.

Comment thread lib/utils/fs.go Outdated
Copy link
Copy Markdown
Contributor

@jentfoo jentfoo left a comment

Choose a reason for hiding this comment

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

Thank you!

@gzdunek gzdunek added this pull request to the merge queue Oct 4, 2023
Merged via the queue into master with commit dffb562 Oct 4, 2023
@gzdunek gzdunek deleted the gzdunek/close-file-in-remove-secure branch October 4, 2023 15:49
@public-teleport-github-review-bot
Copy link
Copy Markdown

@gzdunek See the table below for backport results.

Branch Result
branch/v12 Create PR
branch/v13 Create PR
branch/v14 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Teleport Connect error: Expected PEM encoded private key

4 participants