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

Return 500 instead of 400 when temp directory isn't writeable #44765

Open
1 task done
M1keF opened this issue Oct 27, 2022 · 26 comments
Open
1 task done

Return 500 instead of 400 when temp directory isn't writeable #44765

M1keF opened this issue Oct 27, 2022 · 26 comments
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Docs This issue tracks updating documentation

Comments

@M1keF
Copy link

M1keF commented Oct 27, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

I have just upgraded my Web App from 6.0.9 to 6.0.10 and am now getting a 400 Bad Request response when making large requests to my API. My web app is hosted on a Kubernetes container using Linux with a read-only root file system with the ASPNETCORE_TEMP environment variable to be a separate file system which is writable. If I make the root file system on the container writable then the problem does not occur.

The problem occurs when making requests which exceed 64KB and results in response below being returned to the client.

{type: "https://tools.ietf.org/html/rfc7231#section-6.5.1",…}
errors
: 
{"": ["Failed to read the request form. Read-only file system"]}
""
: 
["Failed to read the request form. Read-only file system"]
status
: 
400
title
: 
"One or more validation errors occurred."
traceId
: 
"00-c7f84b7e6c1e57951d1568eb3f94cab6-aeec3a125ea5243e-00"
type
: 
"https://tools.ietf.org/html/rfc7231#section-6.5.1"

On investigating the issue, I suspected that the problem was to do with large requests being written out to the file system and having looked into the code changes for 6.0.10 I suspect that the changes made in Use Path.GetTempFileName() for 600 Unix file mode' could be the cause.

Some of the changes have introduced calls to Path.GetTemplFileName() which creates a temporary file in the temp folder of the root file system, and if the root file system is read-only then this will be a problem. For example, the method below retrieves the temp folder location, taking into account ASPNETCORE_TEMP, but then in the case of a non-Windows platform then calls Path.GetTemplFileName() which will use the root file system.

aspnetcore/src/Http/WebUtilities/src/FileBufferingWriteStream.cs

    [MemberNotNull(nameof(FileStream))]
    private void EnsureFileStream()
    {
        if (FileStream == null)
        {
            var tempFileDirectory = _tempFileDirectoryAccessor();
            var tempFileName = Path.Combine(tempFileDirectory, "ASPNETCORE_" + Guid.NewGuid() + ".tmp");

            // Create a temp file with the correct Unix file mode before moving it to the assigned tempFileName in the _tempFileDirectory.
            if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
            {
                var tempTempFileName = Path.GetTempFileName();
                File.Move(tempTempFileName, tempFileName);
            }

            FileStream = new FileStream(
                tempFileName,
                FileMode.Create,
                FileAccess.Write,
                FileShare.Delete | FileShare.ReadWrite,
                bufferSize: 1,
                FileOptions.SequentialScan | FileOptions.DeleteOnClose);
        }
    }

Expected Behavior

I expect that by setting the temporary folder location via the ASPNETCORE_TEMP environment variable that all temporary files will be written to this folder, and so avoiding any errors caused by attempting to write to a read-only root file system.

Steps To Reproduce

Hopefully the code example I have shared is enough to go on to fix this.

Exceptions (if any)

I have not been able to locate any exceptions being raised for this problem - the only clue as to what the problem is is the message returned in the response 'Failed to read the request form. Read-only file system'. Maybe because the problem occurs when trying to access the form data the error is treated as being a problem with the form data supplied by the client and so the exception is caught and sent back to the client as a '400 Bad Request'.

.NET Version

6.0.402

Anything else?

ASP.NET Core 6.0.10
Azure Kubernetes 1.23.12

@Tratcher
Copy link
Member

@halter73

@halter73
Copy link
Member

@M1keF Can you work around this issue by setting the TMPDIR environment variable to a directory the application has write access to? Maybe it could match ASPNETCORE_TEMP.

This is a regression, so we can look at backporting a fix that doesn't write to TMPDIR, but that is probably going to require using private reflection to call unix file mode APIs @eerhardt. On the other hand, it does seem a bit unusual to be unable to write to TMPDIR. It feels like that could break a lot of things. And it seems like the workaround should be fairly straightforward.

@davidfowl
Copy link
Member

I would like to introduce a temp file abstract that we write to that is controllable via DI that we use for these subsystems.

@M1keF
Copy link
Author

M1keF commented Oct 28, 2022

Thanks - I can confirm that the workaround works. For my use case, it leads me to question whether it would be better to just set TMPDIR to cover all eventualities and not use ASPNETCORE_TEMP.

Something I discovered whilst testing this out is that the response returned to the client is leaking out the location of the temp folder if it doesn't exist - is there a setting to prevent this detail being returned? I would also expect a 500 Internal Server error in this case rather than a 400 Bad Request. I can raise this as a separate issue if appropriate.

{type: "https://tools.ietf.org/html/rfc7231#section-6.5.1",…}
errors
: 
{"": ["Failed to read the request form. /mnt/tmpz"]}
status
: 
400
title
: 
"One or more validation errors occurred."
traceId
: 
"00-7b8b047142c3b9c70d1106ad5f9498fe-a7a8da7c389c9047-00"
type
: 
"https://tools.ietf.org/html/rfc7231#section-6.5.1"

Thanks for your help.

@adityamandaleeka
Copy link
Member

Having the temp directory location being configurable is tracked by an existing issue: #9466

Responding with a 400 instead of a 500 when the directory isn't set up correctly seems weird for sure. We're not sure how complex that would be to fix though.

@blowdart What is our guidance on having ProblemDetails enabled in production? In this case, that happens to reveal the temp path.

@adityamandaleeka
Copy link
Member

@brunolins16 points out that fixing this to be a 500 will prevent the exception from being exposed. 😄

@adityamandaleeka adityamandaleeka added this to the .NET 8 Planning milestone Oct 31, 2022
@adityamandaleeka adityamandaleeka changed the title Large API requests fail on Kubernetes Linux container with read-only file system Return 500 instead of 400 when temp directory isn't writeable Oct 31, 2022
@adityamandaleeka
Copy link
Member

Retitled this issue to track that ^

@blowdart
Copy link
Contributor

@blowdart What is our guidance on having ProblemDetails enabled in production?

As it's been for 20+ years. Don't do that. If you are returning details somehow in prod by default that needs to change.

@boukisge
Copy link

We are experiencing the same issue with your docker image. Please provide assistance

@drauch
Copy link

drauch commented Dec 12, 2022

I don't know if I should create a separate ticket for it, but as an addition:

If there is not enough disk space to read the form data (e.g., huge file upload), also a 400 is returned (incl. the not-enough-diskspace information!) instead of a plain 500.

Best regards,
D.R.

@tarunkumarrajak
Copy link

I have an API created on .net core 6 and it is deployed on the Google cloud Kubernetes container. API is wrapped under Apigee API gateway.
The API is created to upload files into Azure blob containers. Things are working fine on the local IIS Express setup but when tried through AK Pod, Getting the below error when the file size is more than 64Kb.

{
"type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
"title": "One or more validation errors occurred.",
"status": 400,
"traceId": "00-ccd6cf53a3979e94f97b1a587fc2c3b6-cdf10f9fb44fb4a2-00",
"errors": {
"": [
"Failed to read the request form. Read-only file system"
]
}
}

Request:
curl -X 'POST'
'https://XXXXX
-H 'accept: text/plain'
-H 'Authorization: Bearer XXXX
-H 'Content-Type: multipart/form-data'
-F 'formFile=@2019080358.pdf;type=application/pdf'

Please suggest a fix for it.

@drauch
Copy link

drauch commented Jan 5, 2023

@tarunkumarrajak: Yep, this should also result in a 500. If it's bigger than 64KB ASP.NET Core tries to stream it to the disk which fails in your case as you have a read-only file system.

@tarunkumarrajak
Copy link

tarunkumarrajak commented Jan 5, 2023

@drauch Could you suggest any resolution for this or any workaround?

@M1keF
Copy link
Author

M1keF commented Jan 5, 2023

@tarunkumarrajak: As mentioned above, the workaround is to set the TMPDIR to a writable location. So you will need to add the following config to your Kubernetes Deployment YAML:

apiVersion: apps/v1
kind: Deployment
spec:
  template:
    spec:
      containers:
        env:
        - name: TMPDIR
          value: "/mnt/tmp"
        volumeMounts:
        - name: tmpfs
          mountPath: /mnt/tmp  
      volumes:
      - name: tmpfs
        emptyDir: {}

@Tratcher
Copy link
Member

Tratcher commented Jan 5, 2023

Note the 400 vs 500 is an aesthetic issue, the real problem is that the disk isn't writable and that requires changes to your deployment environment. E.g. we can't make this problem go away for you, but we can more clearly indicate what the problem is.

@drauch
Copy link

drauch commented Jan 5, 2023

Note the 400 vs 500 is an aesthetic issue, the real problem is that the disk isn't writable and that requires changes to your deployment environment. E.g. we can't make this problem go away for you, but we can more clearly indicate what the problem is.

I don't think that this is an aesthetic issue for two big reasons:

  1. We're having an information disclosure, it's no good to inform the client about the state of the server (e.g., that there is no disk space left anymore).
  2. Our monitoring systems don't trigger alerts because 4xx are assumed to be client errors which can be ignored from an operations view.

So it's a real issue for us, and we hope you can at least provide a workaround if you're not backporting a fix.

Best regards,
D.R.

@tarunkumarrajak
Copy link

@tarunkumarrajak: As mentioned above, the workaround is to set the TMPDIR to a writable location. So you will need to add the following config to your Kubernetes Deployment YAML:

apiVersion: apps/v1
kind: Deployment
spec:
  template:
    spec:
      containers:
        env:
        - name: TMPDIR
          value: "/mnt/tmp"
        volumeMounts:
        - name: tmpfs
          mountPath: /mnt/tmp  
      volumes:
      - name: tmpfs
        emptyDir: {}

Thanks for your help. @M1keF It worked

@davidfowl
Copy link
Member

Do we document this for k8s?

cc @richlander

@tarunkumarrajak
Copy link

@M1keF Will this workaround save files in the folder "/mnt/tmp" folder? If Yes, Do we need to clean it up, once the upload job is done?

@M1keF
Copy link
Author

M1keF commented Jan 23, 2023

@tarunkumarrajak Yes, all files that would normally get written to the tmp folder on the root file system will now be written to /mnt/tmp instead. Just do the same clean-up that you would normally do - the actual location of the tmp folder should be transparent to your app. When your Kubernetes container is restarted, the old /mnt/tmp folder will be deleted and a new empty one created.

@davidfowl
Copy link
Member

You don't need to cleanup after the upload. The file is delete on close and will be removed at the end of the upload by the framework.

@richlander
Copy link
Member

I'm writing a bunch of k8s samples. I can add one for this. Describing all the patterns that require mounts (and how to do it correctly) would be very helpful. Are we going to write this topic up as guidance somewhere (more than a bug report)?

@davidfowl
Copy link
Member

Are we going to write this topic up as guidance somewhere (more than a bug report)?

I think it's the bare minimum we should do here. We can also describe workarounds (like doing it all in memory and paying that cost).

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
@Ruud-cb
Copy link

Ruud-cb commented Jan 25, 2024

Huge thanks for this issue. After having the same 'pain' to find out that at a certain size files are written to the file system instead of keeping it in memory, I was directly searching for formdata temporary storage location, which I hoped would lead me to ASPNETCORE_TEMP and TMPDIR, yet only this GitHub issue describes the full problem.

I do would like to see the same expected results as @M1keF . Would expect ASPNETCORE_TEMP to be the solution and not changing the (linux wide) TMPDIR env variable. Perhaps this has already changed according to these docs => verified that still the TMPDIR is needed to change with the latest .NET 6 version.
Besides that it should definatly be a 500 instead of 400 error.

@bbrandt
Copy link
Contributor

bbrandt commented Feb 3, 2024

What's the story with this in .NET 8 if containerizing with dotnet publish?

@amcasey amcasey added the Docs This issue tracks updating documentation label Feb 14, 2024
@satishviswanathan
Copy link

We were setting the path for ASPNETCORE_TEMP in the K8s environment and still facing this issue and the dotnet version we are using is dotnet 8. Is the expectation that we need to set the write permission path for TMPDIR ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Docs This issue tracks updating documentation
Projects
None yet
Development

No branches or pull requests