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

Add support for ITlsConnectionFeature and marshall the APIGW client cert to HttpContext (#786) #787

Merged
merged 6 commits into from
Jan 8, 2021

Conversation

damianh
Copy link
Contributor

@damianh damianh commented Dec 27, 2020

Issue #, if available:

Fixes #786

Description of changes:

  • Adds ItlsConnectionFeature support to InvokeFeatures
  • Extracts incoming ClientCertificate pem to an X509Certificate2 and sets the feature property that then flows through to HttpContext
  • Works with APIGatewayHttpApiV2ProxyRequest and APIGatewayProxyRequest
  • Added a test and a specific controller to TestWebApp. Works on netcoreapp2.1 and netcoeapp3.1.
  • Non-breaking change.

image

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

Copy link
Member

@normj normj left a comment

Choose a reason for hiding this comment

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

Looks good. Just a minor change request to improve the error handling.

if (clientCertPem != null)
{
// Remove "--------BEGIN CERTIFICATE-----\n" and "-----END CERTIFICATE-----"
clientCertPem = clientCertPem.Substring(28, clientCertPem.Length - 53);
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above about verify the cert. Probably put it a utility method that converts incoming cert into the X509Certificate2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above #787 (comment)

@damianh damianh force-pushed the client-cert branch 5 times, most recently from 9b94083 to bd4a713 Compare January 7, 2021 23:57
@damianh
Copy link
Contributor Author

damianh commented Jan 8, 2021

@normj Changes done as requested. Cheers.

…tion and add a check the PEM starts and end with expected values.
@normj normj merged commit 8893051 into aws:master Jan 8, 2021
@damianh
Copy link
Contributor Author

damianh commented Jan 8, 2021

Thanks for the fast feedback and accepting @normj !

@normj
Copy link
Member

normj commented Jan 11, 2021

Thanks again. The PR was just released today in version 5.3.0 of Amazon.Lambda.AspNetCoreServer.

@damianh damianh mentioned this pull request Feb 18, 2021
1 task
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.

[AspNetCoreServer] When using mutual tls, HttpContext.Request.ConnectionInfo.ClientCertificate is null
2 participants