- Notifications
You must be signed in to change notification settings - Fork494
Avoid double processing of default cert bundle in AL2023#1661
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
… loading of certificates.
This simplifies the logic of setting the SSL_CERT_FILE without having to make additional syscalls.
Beau-Gosse-dev left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
| [Theory] | ||
| #ifNET8_0_OR_GREATER | ||
| [InlineData("SSL_CERT_FILE","\"/tmp/noop\"",null)] | ||
| [InlineData("SSL_CERT_FILE","\"/tmp/my-bundle\"","/tmp/my-bundle")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This inline data does support that setting/getting env variable work, but I don't think it's important (here AFAIK).
Is there a way we can start a process via shell and set SSL_CERT_FILE=somevalue and ensure our shell script honor that. If we can't I would understand, but that's a value addition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This inline data does support that setting/getting env variable work, but I don't think it's important (here AFAIK).
I need to make sure if a user wants to override theSSL_CERT_FILE our setting of the env var doesn't override their value.
Is there a way we can start a process via shell and set SSL_CERT_FILE=somevalue and ensure our shell script honor that. If we can't I would understand, but that's a value addition.
Not sure what you mean. The shell script is being executed because it was built into the OCI image of the Lambda function. So it executed just like a normal Lambda invocation. I can't run the script locally for variety of reasons but the biggest one is the test is likely being run from a Windows dev environment.
birojnayakJan 23, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure what you mean. The shell script is being executed because it was built into the OCI image of the Lambda function. So it executed just like a normal Lambda invocation. I can't run the script locally for variety of reasons but the biggest one is the test is likely being run from a Windows dev environment.
I meant to say if there is a way to test below
if [ -z "${SSL_CERT_FILE}"]; then export SSL_CERT_FILE="/tmp/noop"fiIf it's running in Windows , we can't do much.
birojnayak left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Approving..
Improve cold-start performance for HTTPS requests running on Amazon Linux 2023.Seeaws/aws-lambda-dotnet#1661.
Improve cold-start performance for HTTPS requests running on Amazon Linux 2023.Seeaws/aws-lambda-dotnet#1661.
Improve cold start performance for .NET 8 when HTTP requests are made on Amazon Linux 2023.Seeaws/aws-lambda-dotnet#1661.
Description of changes:
We detected a cold start performance regression with .NET on Amazon Linux 2023 (AL2023) when making HTTPS calls. When the first HTTPS call is made itloads the machine store certificates. At a high level the function loads the cert bundle from OpenSSL default file and then from the default directory.
On AL2023 after you follow the symbolic links the default file resolves inside the default directory . This triggers a double load of the default cert bundle causing a regression in cold start performance.
This console below demonstrates how file structure layout for the default file and default directory for loading cert bundles into the machine store.
To prevent the double loading the bootstrap shell script sets the
SSL_CERT_FILEenvironment variable to a non existing file. This allows the default cert file bundle to be loaded as part of the default directory load. The bootstrap script only sets theSSL_CERT_FILEif the user hasn't explicit set it themselves. This allows users to override environment variable for their own cert bundles.Solution
The solution was to add the following to the top of the bootstrap shell script.
We don't want to affect the behavior of .NET 6 on AL2 so a new bootstrap script was created. The only difference between
bootstrap.shandbootstrap-al2023.shis the code above added. I decided against having a single file that checks to see if it is on AL2023 because those extra syscalls could affect cold start. I also debated building a mechanism to prefix the above script to thebootstrap.shwhen building for .NET 8 but decided against it because it seemed unnecessary since .NET 6 goes out of support in 11 months. Once it does we can just remove the oldbootstrap.shfile.Testing
Lots of manual testing to confirm behavior stays the same. Check comparisons of the number of certs loaded from the bundle before and after the change and confirmed the number stayed the same. Also added new smoke tests to confirm the environment variable is set for AL2023 and not AL2.
Long term fix
A PR has been submitted to the
dotnet/runtimeto avoid double loading certs in the .NET runtime level. When that is merged and released we can revert setting this environment variable.dotnet/runtime#97267By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.