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

testing: use $GOTMPDIR for temporary files when set #61585

Open
UnaffiliatedCode opened this issue Jul 25, 2023 · 16 comments
Open

testing: use $GOTMPDIR for temporary files when set #61585

UnaffiliatedCode opened this issue Jul 25, 2023 · 16 comments

Comments

@UnaffiliatedCode
Copy link

Outline

This started as a bug report, however this appears to be a greater change which has a far reaching impact. The use case is regarding devices under restrictive security profiles which require specific directories to be used for specific purposes.

Due to the nature of current security profiles, when generating unit test data which requires a file system usage, the TMPDIR usage is causing the file to become locked by antivirus or other security applications. We can create a security profile which allows for an exception for golang application files (based on directory). This would require an override to TMPDIR or TEMPDIR env vars with the value from GOTMPDIR.

We can bypass this restriction by using a VM on windows (WSL or otherwise) which is unrestricted within the VM. However, this is severely sub-optimal.

What version of Go are you using (go version)?

$ go version
go version go1.20.4 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Dev\GO_ENV\CACHE
set GOENV=C:\Users\anonymous\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\anonymous\go\pkg\mod
set GONOPROXY=none
set GOOS=windows
set GOPATH=C:\Users\anonymous\go
set GOROOT=C:\Dev\GO
set GOSUMDB=sum.golang.org
set GOTMPDIR=C:\Dev\GoTemp
set GOTOOLDIR=C:\Dev\GO\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.20.4
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Dev\GoTemp\go-build3481635822=/tmp/go-build -gno-record-gcc-switches

What did you do?

Using GoLang across many different environments.

func TestUserStore_writeRead(t *testing.T) {
	t.Parallel()
	dir := os.TempDir()
	dir := t.TempDir()	
}

dir = TMP/TEMP system variable in both cases.

What did you expect to see?

Allow for GOTMPDIR to override OS setting for TMPDIR if set. This allows us to use TMPDIR for all other operations while enabling golang to adhere to security posture on development machines which are windows based.

What did you see instead?

TMP / TEMP dir setting within Windows ENV VAR.

@gopherbot gopherbot added this to the Proposal milestone Jul 25, 2023
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Jul 25, 2023
@ianlancetaylor
Copy link
Member

Some points of clarification to make sure that I understand the proposal.

cmd/go already recognizes GOTMPDIR as the place to put build artifacts.

It sounds like you are only talking about Windows.

On Windows, we currently call GetTempPath2W to get the temporary directory.

It sounds like you are saying that on Windows we should change all Go programs to check GOTMPDIR and use it as the temporary directory, if it is set.

What is the advantage of doing that over just changing the TMP or TEMP environment variable? Either way the user has to set an environment variable.

@ianlancetaylor ianlancetaylor changed the title proposal: affected/package: os & testing proposal: os: change default temporary directory on Windows Jul 25, 2023
@ianlancetaylor
Copy link
Member

CC @golang/windows

@UnaffiliatedCode
Copy link
Author

Use Case

With corporate devices there are device level posture requirements which state that tmp/temp environment variables must be specific values/locked. Since I am only looking to override GoLangs usage of tmp/temp. GoLang, already has an artifact path and override for this purpose. There are sections within our codebase with use the following code (for testing purposes):

	dir := os.TempDir()
	dir := t.TempDir()	

This problem is an extension of the problem where build artifacts were being scanned by AV, blocking GoLang from successfully deleting the artifacts. I was able to get past this with a simple GO Configuration change to an established directory which is only used for my GoLang work. I was able to update the AV profile to match.

Why GOTMPDIR

The GOTMPDIR env is already established as an override for build artifacts and other temporary files, which are intended to be cleared after usage. The pattern used to generate the files does not conflict with existing "build artifacts" pattern.

GetTempPath2

Usage of GetTempPath2 would replicate the current results.

The GetTempPath2 function checks for the existence of environment variables in the following order and uses the first path found:

The path specified by the TMP environment variable.
The path specified by the TEMP environment variable.
The path specified by the USERPROFILE environment variable.
The Windows directory.

Why not Change TMP/TEMP variables

This would redirect all applications which generate items within the TMP / TEMP directory (such as Microsoft Office applications) to a directory which natively bypasses the AV.

This would create a larger security vulnerability on the monitored device. Follow-up Impact on the device would be for security profiles to remove configured "safe" directories (causing build artifacts to fail at destruction stage). Which makes Windows based development not viable on corporate devices.

I will however note that there are some other options which could resolve this issue:

Develop within a VM

This is a viable option, however it does not resolve the issue. Currently, I'll be ignoring the tests locally since the remote pipelines are still viable test hosts.

@arp242
Copy link

arp242 commented Jul 27, 2023

I set GOTMPDIR on my system to /extra/go, which is an second SSD rather than the default (in memory) /tmp tmpfs. My system has relatively little RAM, and after a few days of uptime and a few ^C's I found there's quite a lot of files hanging around in memory (alternatively, I suppose I could set it to /tmp/go and automatically clean files older than >1 hour or so).

I can unapologetically rm -rf this directory as long as no go builds are running.

Changing the meaning of GOTMPDIR would be pretty surprising, and potentially breaking.


Your request is essentially "I want to use a system-wide temporary directory, but only for Go programs I don't want to use the standard system-wide temporary directory". I think it should matter as little as possible in which language something in written: do we also need PYTHONTMPDIR? RUBYTMPDIR? RUSTTMPDIR? ZIGTMPDIR?

If you don't want the system default temporary directory, then maybe your application shouldn't use os.TempDir() and t.TempDir(), but write your own replacements for them?

@UnaffiliatedCode
Copy link
Author

Looking at your feedback, lets review the release documentation:

GOTMPDIR will allow you to configure where the temporary files created by Go during compilation of your applications are stored. The default path is the same as in the previous Go versions, the operating system’s temporary files directory.

The default for a non-assigned GOTMPDIR is the TMP/TEMP directory. Which could be interpreted as "When you want to override the TMP directory, update this env". This use-case matches the secondary definition. That being said, GOTMPDIR is for compilation changes, and does not explicitly state run-time. This is the change I am proposing.

I can unapologetically rm -rf this directory as long as no go builds are running.

This is still the case. All TMP/TEMP directories should be treated as such. If I, as a developer, set it to a directory which cannot be rm -rf'd, then that's on the developer.

Change Impact

This change would impact machines which have a GOTMPDIR configured and use a os.Tempdir command.

I think it should matter as little as possible in which language something in written: do we also need PYTHONTMPDIR? RUBYTMPDIR? RUSTTMPDIR? ZIGTMPDIR?

There is no actionable way to address this feedback. The view that we should not make a change because other languages refuse to make a change, which I believe is in direct contrast to the founding principals of Golang (A language which prides itself in being unconventional).

If you don't want the system default temporary directory, then maybe your application shouldn't use os.TempDir() and t.TempDir(), but write your own replacements for them?

I mentioned previously that there are alternative solutions available, however they work around the problem as opposed to solving them. We agree that programming languages should work regardless of developer environment. We do not agree on what that means. Personally, I believe that developers should not have to worry about native configuration management within a language, it should be abstracted away from that necessity. If I configure my language (environment) to override a TMP directory, it should do so for both compilation and runtime.

Concern about the "breaking" nature of this change

Regarding the potential for this change to be a breaking change, maybe GOTMPDIR isn't the right spot for this then. Perhaps an alternative is required. I brought up this proposal to further discussion regarding the issue.

@apparentlymart
Copy link

I develop an application that is written in Go where, from an end-user perspective, it is purely an implementation detail that it is written in Go. As much as possible, my application ought to behave like a "normal application" without the user needing to think about what language I wrote the application in.

I expect os.TempDir to honor the current operating system's conventions for selecting a temporary directory. It would be very surprising if someone who is both a Go developer and a user of my software to see my software behave differently just because they changed the configuration of their Go toolchain.

If this change were implemented, I would need to replace my uses of os.TempDir with a different function so that my application can continue to honor the operating system's typical conventions; os.TempDir would no longer be useful to me. That seems a very strange outcome for a function in package "os", which means "operating system".


Given that this use-case seems very specialized to your particular situation, I might suggest writing a function in your own codebase to meet this need, rather than changing the Go standard library. For example:

func MySpecialTempDir() string {
    if dir := os.Getenv("GOTMPDIR"); dir != "" {
        return dir
    }
    return os.TempDir()
}

If that turns out to be a problem that many different Go codebases share then perhaps there could be a new function in the Go standard library which behaves like the above.

The way we typically learn that something is a common enough need to appear in the standard library is to first implement it in third-party codebases and then observe that either the same code is being written repeatedly or that someone has written a very popular library offering the capability. Therefore, if you believe this is a common need then I would suggest starting by building your own library with a function like the above and then observe how commonly it is used.

(I am personally skeptical that this particular framing is a common need. I can believe that "ability to override the temporary directory for just one program" is a common need, and I understand that you are asserting that reassigning the standard environment variable is not a sufficient answer in your environment even though that is the typical answer to this problem, but it doesn't necessarily follow that the Go standard library must do something special to solve that problem for you, or that a solution to this problem ought to be programming-language-specific rather than something offered by your operating system.)

@apparentlymart
Copy link

I notice that in the body of your proposal you showed an example belonging to a test case.

It might be more defensible for t.TempDir in particular to honor GOTMPDIR because that result is only relevant in the context of tests for programs written in Go.

I don't personally have any need for that, but it would also not impact me and so I would not be concerned if a proposal limited just to that function were to be accepted.

@arp242
Copy link

arp242 commented Jul 28, 2023

I can unapologetically rm -rf this directory as long as no go builds are running.

This is still the case. All TMP/TEMP directories should be treated as such. If I, as a developer, set it to a directory which cannot be rm -rf'd, then that's on the developer.

This is absolutely not the case. Deleting files in /tmp while they're in use will break any program that's using them, you cannot safely "just" delete stuff in /tmp. There's a reason I used the "as long as no go builds are running" qualifier.

@UnaffiliatedCode
Copy link
Author

I notice that in the body of your proposal you showed an example belonging to a test case.

It might be more defensible for t.TempDir in particular to honor GOTMPDIR because that result is only relevant in the context of tests for programs written in Go.

I think this is a very good point. os.TempDir() may be an overreach due to the additional issues mentioned by @arp242 .
For our use case, t.TempDir() would also meet our needs to resolve the problem. This is only a problem for unit testing.

@rsc rsc moved this from Incoming to Active in Proposals Aug 9, 2023
@rsc
Copy link
Contributor

rsc commented Aug 9, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Aug 16, 2023

It sounds like the proposal is to use GOTMPDIR in package testing for t.TempDir. It doesn't sound like this necessarily has to be Windows-specific.

The current definition of GOTMPDIR is

//	GOTMPDIR
//		The directory where the go command will write
//		temporary source files, packages, and binaries.

Adding test-time temporaries seems plausible.

Do I have that right?

@rsc rsc changed the title proposal: os: change default temporary directory on Windows proposal: testing: use $GOTMPDIR for temporary files when set Aug 16, 2023
@UnaffiliatedCode
Copy link
Author

@rsc , Yes.
You have that 100% correct.

After some great discussion by everyone here, we can agree that os.TempDir was an overreach and that we should update only the testing framework to utilize the override (regardless of host OS).

@bcmills
Copy link
Contributor

bcmills commented Aug 16, 2023

I do not agree that os.TempDir was an overreach — I think it has exactly the right semantics for temporary files written by tests. That said, I think it also has the right semantics for temporary files written by the go command too. 🤔

It looks like GOTMPDIR was originally added (for #8451) to give cmd/go somewhere to put executables in case TMPDIR is mounted with the noexec flag. But of course a test might reasonably try to invoke go build or go test -c to compile an executable into t.TempDir() and then run it, and many of the Go toolchain's own tests do exactly that.

So I guess if it's good enough for go build it's good enough for go test too.

Then the question is: does this change belong in the testing package (preferring GOTMPDIR if set, instead of os.TempDir())? Or does it belong in cmd/go (setting the platform-specific variable for the test subprocess, to make os.TempDir() resolve to GOTMPDIR)?

@bcmills
Copy link
Contributor

bcmills commented Aug 16, 2023

I suppose that if we implemented this in cmd/go we would have to find a different solution for net.testUnixAddr, which currently assumes that os.MkdirTemp("", "") returns a path that is short enough to be used for a Unix domain socket. 😅

@rsc rsc moved this from Active to Likely Accept in Proposals Aug 30, 2023
@rsc
Copy link
Contributor

rsc commented Aug 30, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Sep 7, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc moved this from Likely Accept to Accepted in Proposals Sep 7, 2023
@rsc rsc changed the title proposal: testing: use $GOTMPDIR for temporary files when set testing: use $GOTMPDIR for temporary files when set Sep 7, 2023
@rsc rsc modified the milestones: Proposal, Backlog Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

7 participants