Add managed SigV4a signer. - #3923
Conversation
Now that the signer is managed, we create it at static initialization.
| #if NET7_0_OR_GREATER | ||
| return key.SignData(data, HashAlgorithmName.SHA256, DSASignatureFormat.Rfc3279DerSequence); | ||
| #else | ||
| return ConvertToRfc3279DerSequence(key.SignData(data, HashAlgorithmName.SHA256)); | ||
| #endif | ||
| } | ||
|
|
||
| #if !NET7_0_OR_GREATER | ||
| private static byte[] ConvertToRfc3279DerSequence(byte[] signature) | ||
| { | ||
| var writer = new AsnWriter(AsnEncodingRules.DER); | ||
| writer.PushSequence(); | ||
| writer.WriteIntegerUnsigned(signature.AsSpan(0, signature.Length / 2)); // R value | ||
| writer.WriteIntegerUnsigned(signature.AsSpan(signature.Length / 2)); // S value | ||
| writer.PopSequence(); | ||
| return writer.Encode(); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
The fact that we format the signature in ASN.1 is not documented anywhere, and stumped me for some days. I sent feedback to update the documentation.
| /// <summary> | ||
| /// Returns the full presigned Uri | ||
| /// </summary> | ||
| [Obsolete("This property is always empty in objects returned by AWS4aSigner. Use the ForQueryParameters property instead, to get the query parameters for a presigned URL.")] |
There was a problem hiding this comment.
There is a difference in how the SigV4 and SigV4a signing result types return presigned URLs. I think that obsoleting this SigV4a-only property and unifying with SigV4 is the better choice; for reference, there are zero usages of this property outside of the S3 library, which will need the Core bump from the replacement of AWS4aSignerCRTWrapper either way.
| /// <summary> | ||
| /// AWS4a protocol signer for Amazon S3 presigned urls. | ||
| /// </summary> | ||
| public static class AWS4aPreSignedUrlSigner |
There was a problem hiding this comment.
I had the idea of making this a static class, unlike the SigV4 counterpart, which mostly has static members and an overriden method that always throws.
There was a problem hiding this comment.
This might need to be updated to account for the fact that the S3 library needs a newer Core version. How to write this?
The logic is moved to a separate function, and we guard from temporary resource leaks if multiple threads try to populate the cache.
|
I know it's been a while but we're finally starting to look at this PR (I've reached out to the AppSec team at AWS asking for them to review it as well). I also tried to run this branch in our build systems and some existing tests (such as the ones in https://github.com/aws/aws-sdk-net/blob/main/sdk/test/Services/EventBridge/UnitTests/Custom/EventBridgeMRAPTests.cs) failed: Our build system tests on Windows, so I believe that may be related. |
Now both the v4 and v4a result classes have it.
|
Thanks for the update @dscpinheiro. The unit test I added succeeded on .NET 8, but I reproduced the failure on .NET Framework, and upon closer look it does not support importing EC parameter sets with only This also means that SigV4a signing will fail on .NET Core 3.1 as well, which is long out of support but is still targeted by tests. Update: opened .NET Framework backport request in https://developercommunity.visualstudio.com/t/Cannot-import-elliptic-curve-parameter-s/11058177 |
|
Thanks for opening that feature request, will keep an eye on it (we'd definitely prefer not having to write custom cryptography code in the SDK if we can avoid it). On a side note I did another pass of the PR and only had two minor comments:
|
|
I fixed |
|
Personally I do not know if this is likely to be backported to any version of .NET Framework. The backport bar to .NET Framework is very high. One thing that might work, specifically in the case of .NET Framework, is to create a PKCS#8 with an In an ECPrivateKey, the private portion, D, is required, but the public Q parameters Then pass that key to a new instance of So: ECDsa key;
#if NETFRAMEWORK
byte[] assemblePkcsPrivateKey;
CngKey cngKey = CngKey.Import(assemblePkcsPrivateKey, CngKeyBlobFormat. Pkcs8PrivateBlob);
key = new ECDsaCng(cngKey);
#else
key = ECDsa.Create();
// use key.ImportParameters with D populated
#endifThe Windows CNG key importer should handle the PKCS#8 of the ECPrivateKey omitting the It’s not exactly trivial to implement, but it’s “just” assembling some ASN.1. No crypto point multiplication required. |
|
Thanks for chiming in @vcsjones. Upon further investigation, we could simply import a |
|
I updated the .NET Framework implementation to construct and import a |
|
I ran the latest changes through our build system and there were 3 test failures (2 of them are the same but in different methods):
|
|
I fixed the signature format conversion. I copied the .NET sources that do it and added links to them, hoping it will make security review easier. With regards to .NET Core 3.1, there's not much we can do. I thought of importing the CNG key like on .NET Framework, but I'm not sure how much helpful it would be, since it would still fail on non-Windows platforms. Are there plans to drop .NET Core 3.1 support? The AWS SDK would still be installable and (mostly) usable on .NET Core 3.1 after stopping explicit targeting and testing, through the .NET Standard 2.0 binaries. |
Yes, this is something we plan to do in the next minor version of the SDK (i.e. |
|
Thanks; I guess we can't merge it until then. No problem, I can wait. |
dscpinheiro
added
the
pr/blocked
|
Update: We just published https://aws.amazon.com/blogs/developer/annual-net-target-updates-for-the-aws-sdk-for-net/, which includes our plan to drop support for .NET Core 3.1 (the reason the PR was marked as "do not merge"). We can't commit to a specific date (as the post says We will attempt to align as closely as possible with the .NET November release cycle but may fall back to December depending on demands for other priorities, particularly for AWS re:Invent), but the managed SigV4A signer is something we plan to include in our V4.1 release. |
|
Sorry for more conflicts, but I just changed the destination branch here to Sometime next week we'll also create a tracking issue for |
|
Conflicts resolved. I also did some upkeeping. |
dscpinheiro
removed
the
pr/blocked
| var prefix = HeaderSigningResult is AWS4aSigningResult ? V4A_TRAILING_HEADER_STRING_TO_SIGN_PREFIX : V4_TRAILING_HEADER_STRING_TO_SIGN_PREFIX; | ||
| var chunkStringToSign = | ||
| prefix + "\n" + | ||
| HeaderSigningResult.ISO8601DateTime + "\n" + | ||
| HeaderSigningResult.Scope + "\n" + | ||
| PreviousChunkSignature + "\n" + | ||
| AWSSDKUtils.ToHex(AWS4Signer.ComputeHash(canonicalizedTrailingHeaders), true); |
There was a problem hiding this comment.
While not new code, would it be better to do this with a StringBuilder?
There was a problem hiding this comment.
It would be better, but I'm not going to do it in this PR to minimize scope creep. Same with your proposed changes to other parts, which match the equivalent code in SigV4.
| .AppendFormat("{0}={1}", AWSSDKUtils.UrlEncode(HeaderKeys.XAmzAlgorithm, false), AWSSDKUtils.UrlEncode(AWS4Signer.AWS4aAlgorithmTag, false)) | ||
| .AppendFormat("&{0}={1}", AWSSDKUtils.UrlEncode(HeaderKeys.XAmzRegionSetHeader, false), AWSSDKUtils.UrlEncode(RegionSet, false)) | ||
| .AppendFormat("&{0}={1}", AWSSDKUtils.UrlEncode(HeaderKeys.XAmzCredential, false), AWSSDKUtils.UrlEncode(string.Format(CultureInfo.InvariantCulture, "{0}/{1}", AccessKeyId, Scope), false)) | ||
| .AppendFormat("&{0}={1}", AWSSDKUtils.UrlEncode(HeaderKeys.XAmzDateHeader, false), AWSSDKUtils.UrlEncode(ISO8601DateTime, false)) | ||
| .AppendFormat("&{0}={1}", AWSSDKUtils.UrlEncode(HeaderKeys.XAmzSignedHeadersHeader, false), AWSSDKUtils.UrlEncode(SignedHeaders, false)) | ||
| .AppendFormat("&{0}={1}", AWSSDKUtils.UrlEncode(HeaderKeys.XAmzSignature, false), AWSSDKUtils.UrlEncode(Signature, false)); |
There was a problem hiding this comment.
Would it be more performant to avoid AppendFormat and just append the individual elements directly?
.Append(AWSSDKUtils.UrlEncode(HeaderKeys.XAmzAlgorithm, false))
.Append('=')
.Append(AWSSDKUtils.UrlEncode(AWS4Signer.AWS4aAlgorithmTag, false))
.Append('&')
.Append(AWSSDKUtils.UrlEncode(HeaderKeys.XAmzRegionSetHeader, false))
.Append('=')
// etc| var scope = string.Format(CultureInfo.InvariantCulture, "{0}/{1}/{2}", dateStamp, service, Terminator); | ||
|
|
||
| var stringToSignBuilder = new StringBuilder(); | ||
| stringToSignBuilder.AppendFormat(CultureInfo.InvariantCulture, "{0}\n{1}\n{2}\n", | ||
| AWS4aAlgorithmTag, | ||
| FormatDateTime(signedAt, AWSSDKUtils.ISO8601BasicDateTimeFormat), | ||
| scope); |
4 participants
Description
This PR adds a managed implementation of the AWS SigV4a algorithm, contained in the newly added
Amazon.Runtime.Internal.Auth.AWS4aSignerclass. All uses of the old signer that forward to the CRT were replaced. The CRT signer and related APIs were deprecated, but were left otherwise untouched.Motivation and Context
Fixes #3881.
Testing
extensions.Screenshots (if appropriate)
Types of changes
Checklist
License