Add RSASSA-PSS support #53

ackerleytng · 2020-07-08T23:27:05Z

Would appreciate any feedback on this work in progress! I'll be adding more test cases

and update BUILD to run those

since we don't want additional code in the main library if it's not required there

googlebot · 2020-07-08T23:27:44Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

ackerleytng · 2020-07-08T23:30:13Z

@googlebot I signed it!

googlebot · 2020-07-08T23:30:17Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

src/verify.cc

src/jwt.cc

qiwzhang · 2020-07-09T01:07:18Z

Thanks you for your contribution.

src/verify_jwk_rsa_pss_test.cc

Used jwt.io, where I can quickly modify the header of the token

ackerleytng · 2020-07-09T14:33:58Z

I'm a little confused about variable naming on the google style guide, do let me know if there's anything I should change!

src/jwt.cc

src/verify_jwk_rsa_pss_test.cc

qiwzhang

Thanks, just a small nit change.

to avoid the static initialization order problem

qiwzhang · 2020-07-10T02:56:59Z

src/jwt.cc

-namespace {
-
-static const absl::flat_hash_set<std::string> kImplementedAlgs = {
+bool hasImplementedAlg(std::string alg) {


please use string_view

Fixed in ad3a68b, thanks!

src/jwt.cc

See http://www.cs.technion.ac.il/users/yechiel/c++-faq/construct-on-first-use-v2.html

qiwzhang · 2020-07-10T04:27:45Z

src/jwt.cc

@@ -23,6 +24,19 @@
 namespace google {
 namespace jwt_verify {

+bool isImplemented(absl::string_view alg) {


please add annoymous namespace for this function.

Added in 0eac53f!

src/verify_jwk_rsa_pss_test.cc

ackerleytng · 2020-07-10T05:58:47Z

I'd like to propose using the direct initializer syntax (without new). Specifically for this implementation, the handle to implemented_args never leaves the isImplemented function, so C++ is able to insert the destructor appropriately (just before the program exits).

I wrote a small program to illustrate this here and here's the output:

$ g++ ok.cc && valgrind --leak-check=full --show-leak-kinds=all ./a.out
==354120== Memcheck, a memory error detector
==354120== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==354120== Using Valgrind-3.16.0.GIT and LibVEX; rerun with -h for copyright info
==354120== Command: ./a.out
==354120==
Foobar()
11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
Foobar()
11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111~Foobar()
==354120==
==354120== HEAP SUMMARY:
==354120==     in use at exit: 32 bytes in 1 blocks
==354120==   total heap usage: 3 allocs, 2 frees, 73,760 bytes allocated
==354120==
==354120== 32 bytes in 1 blocks are still reachable in loss record 1 of 1
==354120==    at 0x4839DEF: operator new(unsigned long) (vg_replace_malloc.c:342)
==354120==    by 0x1092A1: leak(char) (in /home/ackerleytng/scratch/a.out)
==354120==    by 0x109424: main (in /home/ackerleytng/scratch/a.out)
==354120==
==354120== LEAK SUMMARY:
==354120==    definitely lost: 0 bytes in 0 blocks
==354120==    indirectly lost: 0 bytes in 0 blocks
==354120==      possibly lost: 0 bytes in 0 blocks
==354120==    still reachable: 32 bytes in 1 blocks
==354120==         suppressed: 0 bytes in 0 blocks
==354120==
==354120== For lists of detected and suppressed errors, rerun with: -s
==354120== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

The output shows that when leak() is called, the destructor ~Foobar() is never called, and valgrind reports a leak due to the call to new, whereas the destructor does its work correctly in noLeak(), not in every iteration of the loop, but at the end of the program.

qiwzhang · 2020-07-10T06:11:19Z

OK, thanks.

qiwzhang

Thank you very much for your contribution.

ackerleytng · 2020-07-10T06:52:33Z

Thanks! I suppose we can close #49 ?

ackerleytng added 6 commits July 7, 2020 21:17

Add some test cases for RSASSA-PSS

286dfea

and update BUILD to run those

Override << operator for better test error messages

aa36997

Allow PS256, PS384 and PS512 jwts to be parsed

516c298

Move operator<< override into test_common.h

450f479

since we don't want additional code in the main library if it's not required there

Add timeout specification for tests

7471688

Add implementation to verify RSAPSS and basic testing

07ea1fc

ackerleytng mentioned this pull request Jul 8, 2020

jwt_verify_lib complains about invalid kty/alg combination #49

Closed

qiwzhang reviewed Jul 9, 2020

View reviewed changes

src/verify.cc Outdated Show resolved Hide resolved

qiwzhang reviewed Jul 9, 2020

View reviewed changes

src/jwt.cc Outdated Show resolved Hide resolved

qiwzhang reviewed Jul 9, 2020

View reviewed changes

src/verify_jwk_rsa_pss_test.cc Show resolved Hide resolved

ackerleytng added 4 commits July 9, 2020 20:36

Add test cases for PS384 and PS512

2bf18ac

Add more test cases including negative tests

06d7cc4

Used jwt.io, where I can quickly modify the header of the token

Adjust class data member names to follow style guide

8a284ae

Use absl::flat_hash_set to optimize checking of implemented_algs

9119883

qiwzhang reviewed Jul 9, 2020

View reviewed changes

src/jwt.cc Outdated Show resolved Hide resolved

qiwzhang reviewed Jul 9, 2020

View reviewed changes

ackerleytng added 5 commits July 10, 2020 07:27

Add note to indicate no need to free pctx

bc56e98

Remove inheritance for better readability in test case

01a41aa

Unroll parameterized test for better readability

04b0673

Mark kImplementedAlgs as const

e9757de

Explain test cases better in comments

c2b0718

qiwzhang reviewed Jul 10, 2020

View reviewed changes

src/verify_jwk_rsa_pss_test.cc Outdated Show resolved Hide resolved

qiwzhang reviewed Jul 10, 2020

View reviewed changes

ackerleytng added 2 commits July 10, 2020 09:09

Use shorter variable names with descriptive comments

ddf8cdf

Move global static const flat_hash_set into function

9cb97fe

to avoid the static initialization order problem

qiwzhang reviewed Jul 10, 2020

View reviewed changes

ackerleytng added 3 commits July 10, 2020 11:08

Improve helper function name

56f0f6d

Switched to use string_view

ad3a68b

Fixed static deinitialization problem

1015e94

See http://www.cs.technion.ac.il/users/yechiel/c++-faq/construct-on-first-use-v2.html

qiwzhang reviewed Jul 10, 2020

View reviewed changes

src/verify_jwk_rsa_pss_test.cc Outdated Show resolved Hide resolved

ackerleytng added 2 commits July 10, 2020 13:44

Add anonymous namespace for isImplemented

0eac53f

Allow destructor to do cleanup of allocated static memory

5e4b5e7

Applied linting with ./script/check-style

2bafc33

qiwzhang approved these changes Jul 10, 2020

View reviewed changes

qiwzhang merged commit 894ba8e into google:master Jul 10, 2020

ackerleytng changed the title ~~[WIP] Add RSASSA-PSS support~~ Add RSASSA-PSS support Jul 10, 2020

ackerleytng deleted the add-rsassa-pss-support branch July 10, 2020 06:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RSASSA-PSS support #53

Add RSASSA-PSS support #53

ackerleytng commented Jul 8, 2020

googlebot commented Jul 8, 2020

ackerleytng commented Jul 8, 2020

googlebot commented Jul 8, 2020

qiwzhang commented Jul 9, 2020

ackerleytng commented Jul 9, 2020 •

edited

qiwzhang left a comment

qiwzhang Jul 10, 2020

ackerleytng Jul 10, 2020

qiwzhang Jul 10, 2020

ackerleytng Jul 10, 2020

ackerleytng commented Jul 10, 2020 •

edited

qiwzhang commented Jul 10, 2020

qiwzhang left a comment

ackerleytng commented Jul 10, 2020

Add RSASSA-PSS support #53

Add RSASSA-PSS support #53

Conversation

ackerleytng commented Jul 8, 2020

googlebot commented Jul 8, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

ackerleytng commented Jul 8, 2020

googlebot commented Jul 8, 2020

qiwzhang commented Jul 9, 2020

ackerleytng commented Jul 9, 2020 • edited

qiwzhang left a comment

Choose a reason for hiding this comment

qiwzhang Jul 10, 2020

Choose a reason for hiding this comment

ackerleytng Jul 10, 2020

Choose a reason for hiding this comment

qiwzhang Jul 10, 2020

Choose a reason for hiding this comment

ackerleytng Jul 10, 2020

Choose a reason for hiding this comment

ackerleytng commented Jul 10, 2020 • edited

qiwzhang commented Jul 10, 2020

qiwzhang left a comment

Choose a reason for hiding this comment

ackerleytng commented Jul 10, 2020

ackerleytng commented Jul 9, 2020 •

edited

ackerleytng commented Jul 10, 2020 •

edited