Removed dependency on System.IdentityModel.Tokens.Jwt from WPS #22112

KrzysztofCwalina · 2021-06-23T19:59:01Z

This PR includes a custom implementation of a JWT builder/writer.
The JwtBuilder is now used instead of an external dependency on System.IdentityModel.Tokens.Jwt

I wanted to make the code fast and non-allocating. The System.IdentityModel.Tokens.Jwt library caches tokens. This new implementation does not. The cache has been identified as problematic in preview 1.

KrzysztofCwalina · 2021-06-23T21:00:43Z

sdk/webpubsub/Azure.Messaging.WebPubSub/src/WebPubSubAuthenticationPolicy.cs


-            string accessToken = JwtUtils.GenerateJwtBearer(audience, claims: null, expiresAt, _credential);
+            // TODO: is this a bug in the service?


@vicancy, I noticed something strange when I was changing this code. The key string comes from the portal. If I Base64 decode it, this library will not successfully authenticate with the service. If I just use ASCII bytes of the key string, it does work. The same behavior was present in preview 1, but I find it strange. Key strings of other services need to be Base64 decoded.

KrzysztofCwalina · 2021-06-23T21:50:59Z

@GrabYourPitchforks, you might want to look into the NS2Bridge.cs for BCL text transformation APIs we would love to have in a package.

sdk/webpubsub/Azure.Messaging.WebPubSub/src/JwtBuilder.cs

ellismg · 2021-06-23T21:16:00Z

sdk/webpubsub/Azure.Messaging.WebPubSub/src/JwtBuilder.cs

+            int maxBufferLength =
+                Base64.GetMaxEncodedToUtf8Length(headerSha256.Length + payloadLength)
+                + 1 // dot
+                + Base64.GetMaxEncodedToUtf8Length(32); // signature SHA256 hash size


Could we make this static readonly to avoid computing it once per call?

Payload length is per call. The Uri and DateTimeOffsets change between calls. This computation is very cheap. It's just 4/3 * length. https://github.com/dotnet/runtime/blob/main/src/libraries/System.Memory/src/System/Buffers/Text/Base64Encoder.cs#L150

Feedback was specifically about the just making the result of a Base64.GetMaxEncodedToUtf8Length(32) constant, but no need to change things here.

sdk/webpubsub/Azure.Messaging.WebPubSub/tests/WebPubSubParseConnectionStringTests.cs

ellismg · 2021-06-23T21:19:14Z

sdk/webpubsub/Azure.Messaging.WebPubSub/tests/WebPubSubParseConnectionStringTests.cs

+            Assert.NotNull(exp);
+            Assert.IsTrue(long.TryParse(exp, out var expireAt));
+
+            // default expire atfte should be ~5 minutes (~300 seconds)


Suggested change

// default expire atfte should be ~5 minutes (~300 seconds)

// default expire after should be ~5 minutes (~300 seconds)

ellismg · 2021-06-23T21:53:45Z

sdk/webpubsub/Azure.Messaging.WebPubSub/src/NS2Bridge.cs

+            return result;
+        }
+
+        public static void Append(this StringBuilder sb, ReadOnlySpan<char> value)


This method is unused, can it be removed?

Yeah, I had a WriteTo(StringBulder) overload. But I removed it. If we ever make the JwtBulder public, it would be good to have these, but I will add them at that point.

sdk/webpubsub/Azure.Messaging.WebPubSub/src/WebPubSubAuthenticationPolicy.cs

ellismg · 2021-06-23T22:07:06Z

sdk/webpubsub/Azure.Messaging.WebPubSub/src/JwtBuilder.cs

+        public static ReadOnlySpan<byte> Iss => s_iss;
+        public static ReadOnlySpan<byte> Jti => s_jti;
+
+        // this is the standrd JWT header. { "alg": "HS256", "typ": "JWT" }


Suggested change

// this is the standrd JWT header. { "alg": "HS256", "typ": "JWT" }

// this is Base64 encoding of the standard JWT header. { "alg": "HS256", "typ": "JWT" }

ellismg · 2021-06-23T22:07:35Z

sdk/webpubsub/Azure.Messaging.WebPubSub/src/JwtBuilder.cs

+        private Utf8JsonWriter _writer;
+        private MemoryStream _memoryStream;
+        private byte[] _key;
+        private bool isDisposed;


NIT: should we name it _isDisposed?

ellismg · 2021-06-23T22:10:59Z

sdk/webpubsub/Azure.Messaging.WebPubSub/src/JwtBuilder.cs

+            int payloadLength = (int)_writer.BytesCommitted; // writer is wrrapping MemoryStream, and so the length will never overflow int.
+
+            int payloadIndex = headerSha256.Length;
+            int maxBufferLength =


Any concerns about /this/ computation overflowing?

It would be very hard to make it overflow, but I changed the code to do checked additions.

…nnectionStringTests.cs Co-authored-by: Matt Ellis <matt.ellis@microsoft.com>

…o WPSJWT

ellismg

LGTM - Thanks for the changes!

Dev keyvault microsoft.key vault 2022 02 01 preview swagger completeness (Azure#22154) * Adds base for updating Microsoft.KeyVault from version stable/2022-07-01 to version 2022-02-01-preview * Updates readme * Updates API version in new specs and examples * Fixed breaking changes in managedHsm.json. (Azure#22112) Fixed LRO_RESPONSE_HEADER, INVALID_REQUEST_PARAMETER and OBJECT_MISSING_REQUIRED_PROPERTY errors. Co-authored-by: Rahul Alapati <rahulalapati@microsoft.com> * Undid the API version changes to the Vaults_List API (Azure#22142) * Fixed breaking changes in managedHsm.json. Fixed LRO_RESPONSE_HEADER, INVALID_REQUEST_PARAMETER and OBJECT_MISSING_REQUIRED_PROPERTY errors. * Undid the API Version change to the VaultListFilterTypes API. Co-authored-by: Rahul Alapati <rahulalapati@microsoft.com> * Update listVault.json (Azure#22155) * Update listVault.json * Remove the preview API version for the enum * Delete Vault_Lists API specification * Added the Vaults_List swagger specification back. * Abstracted Vaults_List API into 2015-11-01 directory to Swagger ModelValidation Errors. * Resolved unresolvable references. * Reorganized the definitions. * Reorganized the 2015-11-01 directory under 2022-02-01-preview * Moved the 2015-11-10 directory to stable. * Undo the abstraction changes. * Readd the listVault.json. * Added suppression directives for INVALID_REQUEST_PARAMETER and OBJECT_MISSING_REQUIRED_PROPERTY. * Modify suppress rule. * Update the suppression directive. * Update suppression directive. * Update suppression directive. * Update suppression directives. * Updated suppression directive. * Added suppression directives for INVALID_REQUEST_PARAMETER, OBJECT_MISSING_REQUIRED_PROPERTY * Updated suppression directives. * Updated suppression directives. * Updated suppression directives. * Updated suppression directives. * Updated suppression directives. * Added x-ms-validation rules to the api-version parameter. * Updated suppression directives. * Added individual suppression directives. * Removed where clause from the OBJECT_MISSING_REQUIRED_PROPERTY suppression directive. * Corrected yaml title for suppression directives. Co-authored-by: Rahul Alapati <rahulalapati@microsoft.com> --------- Co-authored-by: Rahul Alapati <rahulalapati@microsoft.com>

KrzysztofCwalina added 6 commits June 23, 2021 10:26

Added JWT writer

77934b1

fixed bugs

9667e41

Removed dependency on System.IdentityModel.Tokens.Jwt

fccc0bb

cleaned up tests

bd7ed9b

Exposed registered claims as properties

180fcec

cleaned up code

93c86f7

KrzysztofCwalina requested review from vicancy, tg-msft and schaabs June 23, 2021 19:59

fixed build break

999069f

KrzysztofCwalina requested a review from ellismg June 23, 2021 20:55

KrzysztofCwalina commented Jun 23, 2021

View reviewed changes

KrzysztofCwalina marked this pull request as ready for review June 23, 2021 21:51

KrzysztofCwalina requested a review from JialinXin as a code owner June 23, 2021 21:51

ellismg reviewed Jun 23, 2021

View reviewed changes

KrzysztofCwalina removed the request for review from JialinXin June 23, 2021 22:27

KrzysztofCwalina and others added 8 commits June 23, 2021 15:34

Update sdk/webpubsub/Azure.Messaging.WebPubSub/tests/WebPubSubParseCo…

cb81ac2

…nnectionStringTests.cs Co-authored-by: Matt Ellis <matt.ellis@microsoft.com>

PR review feedback

d88c17e

fixed build break

c1c5dc6

we are ok with ASCII

b1f0a33

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-net int…

07010e4

…o WPSJWT

merged with main and updated to build

132405b

Added error checks

016be0e

fixed misspelling

529fe87

ellismg approved these changes Jun 24, 2021

View reviewed changes

KrzysztofCwalina merged commit 77dc25a into Azure:main Jun 24, 2021

KrzysztofCwalina deleted the WPSJWT branch July 8, 2021 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed dependency on System.IdentityModel.Tokens.Jwt from WPS #22112

Removed dependency on System.IdentityModel.Tokens.Jwt from WPS #22112

KrzysztofCwalina commented Jun 23, 2021 •

edited

Loading

KrzysztofCwalina Jun 23, 2021

KrzysztofCwalina commented Jun 23, 2021

ellismg Jun 23, 2021

KrzysztofCwalina Jun 23, 2021 •

edited

Loading

ellismg Jun 24, 2021

ellismg Jun 23, 2021

ellismg Jun 23, 2021

KrzysztofCwalina Jun 23, 2021

ellismg Jun 23, 2021

ellismg Jun 23, 2021

ellismg Jun 23, 2021

KrzysztofCwalina Jun 23, 2021

ellismg left a comment


		string accessToken = JwtUtils.GenerateJwtBearer(audience, claims: null, expiresAt, _credential);
		// TODO: is this a bug in the service?

	// default expire atfte should be ~5 minutes (~300 seconds)
	// default expire after should be ~5 minutes (~300 seconds)

	// this is the standrd JWT header. { "alg": "HS256", "typ": "JWT" }
	// this is Base64 encoding of the standard JWT header. { "alg": "HS256", "typ": "JWT" }

Removed dependency on System.IdentityModel.Tokens.Jwt from WPS #22112

Removed dependency on System.IdentityModel.Tokens.Jwt from WPS #22112

Conversation

KrzysztofCwalina commented Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

KrzysztofCwalina commented Jun 23, 2021

Choose a reason for hiding this comment

KrzysztofCwalina Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellismg left a comment

Choose a reason for hiding this comment

KrzysztofCwalina commented Jun 23, 2021 •

edited

Loading

KrzysztofCwalina Jun 23, 2021 •

edited

Loading