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

Avoid double processing of default cert bundle in AL2023 #1661

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

normj
Copy link
Member

@normj normj commented Jan 22, 2024

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 it loads 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.

[ec2-user@ip-172-31-31-127 ~]$ openssl version -d
OPENSSLDIR: "/etc/pki/tls"
**File Processing**
[ec2-user@ip-172-31-31-127 ~]$ ls -la /etc/pki/tls/cert.pem 
lrwxrwxrwx. 1 root root 49 Aug 29 17:21 /etc/pki/tls/cert.pem -> /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem
**Directory Processing**
[ec2-user@ip-172-31-31-127 ~]$ ls -la /etc/pki/tls/certs
total 0
drwxr-xr-x. 2 root root  54 Nov  8 07:41 .
drwxr-xr-x. 5 root root 126 Nov  8 07:41 ..
lrwxrwxrwx. 1 root root  49 Aug 29 17:21 ca-bundle.crt -> /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem
lrwxrwxrwx. 1 root root  55 Aug 29 17:21 ca-bundle.trust.crt -> /etc/pki/ca-trust/extracted/openssl/ca-bundle.trust.crt

To prevent the double loading the bootstrap shell script sets the SSL_CERT_FILE environment 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 the SSL_CERT_FILE if 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.

if [ -z "${SSL_CERT_FILE}"]; then
  export SSL_CERT_FILE="/tmp/noop"
fi

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.sh and bootstrap-al2023.sh is 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 the bootstrap.sh when 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 old bootstrap.sh file.

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/runtime to avoid double loading certs in the .NET runtime level. When that is merged and released we can revert setting this environment variable. dotnet/runtime#97267

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

normj added 3 commits January 16, 2024 17:06
This simplifies the logic of setting the SSL_CERT_FILE without having to make additional syscalls.
Copy link
Contributor

@Beau-Gosse-dev Beau-Gosse-dev left a comment

Choose a reason for hiding this comment

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

LGTM

[Theory]
#if NET8_0_OR_GREATER
[InlineData("SSL_CERT_FILE", "\"/tmp/noop\"", null)]
[InlineData("SSL_CERT_FILE", "\"/tmp/my-bundle\"", "/tmp/my-bundle")]

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.

Copy link
Member Author

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 the SSL_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.

Copy link

@birojnayak birojnayak Jan 23, 2024

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"
fi

If it's running in Windows , we can't do much.

Copy link

@birojnayak birojnayak left a comment

Choose a reason for hiding this comment

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

Approving..

@normj normj merged commit 9ab1c37 into master Jan 26, 2024
4 checks passed
@normj normj deleted the normj/al2023-certs branch January 26, 2024 18:33
martincostello added a commit to martincostello/alexa-london-travel that referenced this pull request Jan 27, 2024
Improve cold-start performance for HTTPS requests running on Amazon Linux 2023.
See aws/aws-lambda-dotnet#1661.
martincostello added a commit to martincostello/alexa-london-travel that referenced this pull request Jan 27, 2024
Improve cold-start performance for HTTPS requests running on Amazon Linux 2023.
See aws/aws-lambda-dotnet#1661.
@normj normj mentioned this pull request Jan 29, 2024
maxday pushed a commit to maxday/lambda-perf that referenced this pull request Feb 2, 2024
Improve cold start performance for .NET 8 when HTTP requests are made on Amazon Linux 2023.
See aws/aws-lambda-dotnet#1661.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants