Modify RS256Algorithm to use RSA from cert directly #127

abatishchev · 2017-07-14T19:28:25Z

Resolves #124, closes #118.

Drawback:

class RSA doesn't have method SignData in .NET 3.5, so I upgraded to .NET 4.6.2

abatishchev · 2017-07-14T19:29:52Z

@nbarbettini can you please check whether I upgraded target framework properly? I'm asking because AppVeyor fails

abatishchev · 2017-07-14T19:30:33Z

@StefH can you please help with test, and what's more important with proper unit testing?

StefH · 2017-07-15T07:16:05Z

0]
I did also add some code-review comments...

Testing can be done like:

1] Generate certificate and private key

openssl req -x509 -sha256 -nodes -days 365 -newkey rsa:2048 -keyout private.key -out certificate_pub.crt

2]
Make a X509 certificate based on the two file above (https://github.com/StefH/JwtTest/blob/master/JwtTest/CertificateFromFileProvider.cs)

3]
Generate a token based on RS256 using your old (current) and new (this PR) RS256Algorithm.

4]
Copy paste this token in Encoded field on https://jwt.io/

5]
Copy paste your public certificate key (certificate_pub.crt) into that VERIFY SIGNATURE block (first block) only.

6]
When you get a blue box beneath saying signature verified then the JWT is valid.

nbarbettini · 2017-07-17T16:08:01Z

src/JWT/JWT.csproj

@@ -3,10 +3,9 @@
  <PropertyGroup>
    <TargetFrameworks>net35;netstandard1.3</TargetFrameworks>


This line needs to be net462;netstandard1.3

Done, thanks!

nbarbettini · 2017-07-17T16:08:29Z

src/JWT/JWT.csproj

-    <FrameworkPathOverride>C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v3.5\Profile\Client</FrameworkPathOverride>
+  <PropertyGroup Condition="'$(TargetFramework)' == 'net462'">
+    <TargetFramework>net462</TargetFramework>
+    <DefineConstants>NET46</DefineConstants>


Do we have any #if directives that look for either NET35 or NET46?

No, only 1 places (this file) that looks for NET_STANDARD. Good point thought!

Adding netstandard1.3 back

abatishchev · 2017-07-18T15:49:03Z

@ubercow sorry, it took some time to come back to the issue you're opened. Can you please take a look on this project? Should be same.

abatishchev · 2017-07-18T15:53:21Z

@StefH right but I meant some input how to update existing/add new unit test(s). Can you please help here?

abatishchev · 2017-07-18T20:13:41Z

AppVeyor can't build anything:

Could not find Cake.exe at C:\projects\jwt\tools\Cake\Cake.exe

I question its stability :(

nbarbettini · 2017-07-18T20:14:59Z

AppVeyor can't build anything

That doesn't seem like a typical error - can you try restarting the build?

abatishchev · 2017-07-18T20:28:39Z

It fails with same error for a while now: https://ci.appveyor.com/project/abatishchev/jwt/history

StefH · 2017-07-15T07:03:42Z

src/JWT/Algorithms/RS256Algorithm.cs

 #if NETSTANDARD1_3
-            var rsa = (RSACryptoServiceProvider)_cert.GetRSAPrivateKey();
+            return (RSA)cert.GetRSAPrivateKey();


Why casting to RSA ? This is not needed ?

Indeed, thanks!

StefH · 2017-07-15T07:10:30Z

src/JWT/JWT.csproj

-    <TargetFrameworkIdentifier>.NETFramework</TargetFrameworkIdentifier>
-    <DefineConstants>NET35</DefineConstants>
-    <FrameworkPathOverride>C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v3.5\Profile\Client</FrameworkPathOverride>
+  <PropertyGroup Condition="'$(TargetFramework)' == 'net462'">


This line is not used because you only define <TargetFrameworks>net35;netstandard1.3</TargetFrameworks>.

So just revert back this change.

Here I want to upgrade to net462. What would be the right way to do that?

you can just add net462 like:
<TargetFrameworks>net35;net462;netstandard1.3</TargetFrameworks>.

But this is not needed because the net35 can be used by all frameworks.

Currently it's this:

<TargetFrameworks>net462;netstandard1.3</TargetFrameworks>

Isn't this correct?

StefH · 2017-07-19T18:31:07Z

@abatishchev
About unit-tests : you don't really need to test all rsa encryption code. Just use Moq to mock some calls.

abatishchev · 2017-07-19T18:43:39Z

Usually we test end-to-end by having hard-coded JWT and asserting expected outcome

… fix-rs-256-1

StefH · 2017-07-19T19:19:47Z

If the build works fine, and all code is merged to master branch, I will take a quick look how to do unit test.

… fix-rs-256-1

abatishchev · 2017-07-24T18:25:49Z

@nbarbettini please take a look to the latest changes

…4.6.2

nbarbettini · 2017-07-24T22:53:29Z

I'm not super familiar with the RSACryptoServiceProvider stuff, but it looks reasonable.

Are you intending to raise the minimum framework version to .NET 4.6.2?

abatishchev · 2017-07-24T22:55:58Z

Yeah, I'm not either :) Hope it works, and now works better.

Yes, to normalize the code between .NetFramework and .Net Core. Hope everybody will be fine with that too.

StefH · 2017-07-25T08:06:11Z

If possible, can you lower the dependency for 4.6.2?

Can you lower this to .net 4.0? In that way your librabry can also be used by .net 4 and .net 4.5 libraries/programs.

abatishchev · 2017-07-25T17:30:51Z

Let's continue in #130. Can you please describe your scenario/environment/requirements? The thing from my perspective is that 4.6.2 is the lowest supported as of today version, all lower are out of support. And transition from 4.0 to 4.5. would be long due, and from 4.5 to 4.6.2 usually is seamless. But again I'd love to understand your perspective.

abatishchev added 2 commits July 14, 2017 12:22

Using RSA from cert, do not instantiating new RSACryptoServiceProvider

a39b079

Upgrading to .NET 4.6.2

efbf7b7

abatishchev added the enchantment label Jul 14, 2017

abatishchev added this to the 3.0 milestone Jul 14, 2017

abatishchev self-assigned this Jul 14, 2017

abatishchev requested a review from nbarbettini July 14, 2017 19:29

nbarbettini suggested changes Jul 17, 2017

View reviewed changes

Update JWT.csproj

6d53ca3

Adding netstandard1.3 back

abatishchev added 2 commits July 18, 2017 12:05

Update JWT.csproj

3a14a0e

Update JWT.csproj

5358c92

StefH reviewed Jul 19, 2017

View reviewed changes

Removing cast to RSA

b9110b8

Merge branch 'fix-rs-256-1' of https://github.com/jwt-dotnet/jwt into…

a2f4840

… fix-rs-256-1

abatishchev and others added 6 commits July 21, 2017 12:40

Merge branch 'master' into fix-rs-256-1

ab5ee50

Dropping Version=4.6.2 from TargetFrameworkIdentifier

6fdd711

Merge branch 'fix-rs-256-1' of https://github.com/jwt-dotnet/jwt into…

492161d

… fix-rs-256-1

Adding cast back. Conforming the RSA.Sign() signature

171fbe0

Merge remote-tracking branch 'origin/master' into fix-rs-256-1

636f244

Bumping version to 3.0.2

6d6cfb7

Updating, removing nuget packages. Trageting Tests..NETFramerwork to …

cade88b

…4.6.2

nbarbettini approved these changes Jul 24, 2017

View reviewed changes

abatishchev merged commit c7a7145 into master Jul 24, 2017

abatishchev deleted the fix-rs-256-1 branch July 25, 2017 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify RS256Algorithm to use RSA from cert directly #127

Modify RS256Algorithm to use RSA from cert directly #127

abatishchev commented Jul 14, 2017 •

edited

Loading

abatishchev commented Jul 14, 2017

abatishchev commented Jul 14, 2017

StefH commented Jul 15, 2017 •

edited

Loading

nbarbettini Jul 17, 2017

abatishchev Jul 18, 2017

nbarbettini Jul 17, 2017

abatishchev Jul 18, 2017

abatishchev commented Jul 18, 2017

abatishchev commented Jul 18, 2017

abatishchev commented Jul 18, 2017 •

edited

Loading

nbarbettini commented Jul 18, 2017

abatishchev commented Jul 18, 2017

StefH Jul 15, 2017

abatishchev Jul 19, 2017

StefH Jul 15, 2017

abatishchev Jul 19, 2017

StefH Jul 19, 2017

abatishchev Jul 19, 2017

StefH commented Jul 19, 2017

abatishchev commented Jul 19, 2017

StefH commented Jul 19, 2017

abatishchev commented Jul 24, 2017

nbarbettini commented Jul 24, 2017

abatishchev commented Jul 24, 2017

StefH commented Jul 25, 2017

abatishchev commented Jul 25, 2017

		@@ -3,10 +3,9 @@
		<PropertyGroup>
		<TargetFrameworks>net35;netstandard1.3</TargetFrameworks>

Modify RS256Algorithm to use RSA from cert directly #127

Modify RS256Algorithm to use RSA from cert directly #127

Conversation

abatishchev commented Jul 14, 2017 • edited Loading

abatishchev commented Jul 14, 2017

abatishchev commented Jul 14, 2017

StefH commented Jul 15, 2017 • 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

abatishchev commented Jul 18, 2017

abatishchev commented Jul 18, 2017

abatishchev commented Jul 18, 2017 • edited Loading

nbarbettini commented Jul 18, 2017

abatishchev commented Jul 18, 2017

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

StefH commented Jul 19, 2017

abatishchev commented Jul 19, 2017

StefH commented Jul 19, 2017

abatishchev commented Jul 24, 2017

nbarbettini commented Jul 24, 2017

abatishchev commented Jul 24, 2017

StefH commented Jul 25, 2017

abatishchev commented Jul 25, 2017

abatishchev commented Jul 14, 2017 •

edited

Loading

StefH commented Jul 15, 2017 •

edited

Loading

abatishchev commented Jul 18, 2017 •

edited

Loading