Fix compilation error in JWT using const_cast. by soheilhy · Pull Request #18475 · grpc/grpc

soheilhy · 2019-03-22T10:57:22Z

I got this error on an unrelated patch, and I believe the Distribution Test is broken:

    /var/local/git/grpc/src/core/lib/security/credentials/jwt/jwt_verifier.cc:628:57: error: invalid conversion from 'const uint8_t* {aka const unsigned char*}' to 'unsigned char*' [-fpermissive]
                                 GRPC_SLICE_LENGTH(signature)) != 1) {
                                                             ^
    In file included from /usr/include/openssl/pem.h:69:0,
                     from /var/local/git/grpc/src/core/lib/security/credentials/jwt/jwt_verifier.cc:35:
    /usr/include/openssl/evp.h:642:5: note: initializing argument 2 of 'int EVP_DigestVerifyFinal(EVP_MD_CTX*, unsigned char*, size_t)'
     int EVP_DigestVerifyFinal(EVP_MD_CTX *ctx, unsigned char *sig, size_t siglen);

I got this error on an unrelated patch, and I believe the Distribution Test is broken: /var/local/git/grpc/src/core/lib/security/credentials/jwt/jwt_verifier.cc:628:57: error: invalid conversion from 'const uint8_t* {aka const unsigned char*}' to 'unsigned char*' [-fpermissive] GRPC_SLICE_LENGTH(signature)) != 1) { ^ In file included from /usr/include/openssl/pem.h:69:0, from /var/local/git/grpc/src/core/lib/security/credentials/jwt/jwt_verifier.cc:35: /usr/include/openssl/evp.h:642:5: note: initializing argument 2 of 'int EVP_DigestVerifyFinal(EVP_MD_CTX*, unsigned char*, size_t)' int EVP_DigestVerifyFinal(EVP_MD_CTX *ctx, unsigned char *sig, size_t siglen);

soheilhy · 2019-03-22T11:17:41Z

I think there are many build failures from openssl https://source.cloud.google.com/results/invocations/150afc8a-de46-4b66-93b9-0486a2687ad7/targets/grpc%2Fcore%2Fpull_request%2Flinux%2Fgrpc_distribtests_standalone/log

Was there a recent update?

vjpai

LGTM for code quality but please get an approval from @yihuazhang or @jiangtaoli2016

vjpai · 2019-03-26T18:07:38Z

It looks like there are still distrib test failures that are also SSL related....

soheilhy · 2019-03-26T18:12:27Z

Yes exactly that's why I didn't merge this patch. I think someone from security team should take a look. I'm not sure if the new changes have any correctness issues.

…

On Tue, Mar 26, 2019, 2:07 PM Vijay Pai ***@***.***> wrote: It looks like there are still distrib test failures that are also SSL related.... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#18475 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYGhQkCIqiA815kxRsZk5-OUO6AgBFaks5vamHtgaJpZM4cDUbz> .

yihuazhang · 2019-03-26T18:27:29Z

Sorry for the delay. I am looking at the issue now.

soheilhy · 2019-03-26T18:32:48Z

Thank you!

…

On Tue, Mar 26, 2019, 2:27 PM yihuaz ***@***.***> wrote: Sorry for the delay. I am looking at the issue now. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#18475 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYGhSPiekPUblQNDKxaZKmIeLFEEtdcks5vamaXgaJpZM4cDUbz> .

yihuazhang · 2019-03-26T18:59:39Z

From the log of distribtests, I found the test is linking openssl 1.0.1t which is out of support and that's probably the reason for SSL-related failures. I think the changes in this PR may not be necessary if linking to the up-to-date version of openssl.

nicolasnoble · 2019-03-26T19:14:11Z

Well, the issue is that Debian Jessie is still within LTS, and they do their own vendoring of OpenSSL 1.0.1 patches, so...

And last time we broke Debian Jessie, we caused an official Google outage.

nicolasnoble · 2019-03-26T20:19:38Z

So, I have verified the manual pages and the code of OpenSSL in Debian Jessie. The behavior of EVP_DigestVerifyFinal didn't change, they just correctly added the const keyword there. Therefore, the usage of const_cast is valid in this case.

Still, I'm confused as to how this is only a problem now. What did we change?

soheilhy · 2019-03-26T20:23:56Z

Thank you @nicolasnoble ! Yes, this patch is valid, but there will other errors if/when I submit this:
https://source.cloud.google.com/results/invocations/150afc8a-de46-4b66-93b9-0486a2687ad7/targets/grpc%2Fcore%2Fpull_request%2Flinux%2Fgrpc_distribtests_standalone/log

I guess there are undefined macros and such, but haven't taken a closer look.

nicolasnoble · 2019-03-26T20:25:40Z

I see. There's a recent PR then that broke OpenSSL 1.0.1. Let's dig this up.

jiangtaoli2016 · 2019-03-26T21:04:21Z

it is due to #17868
When did we add distribution linux test?

soheilhy · 2019-03-26T21:07:49Z

Thank you. So I will merge this PR, but would you be able to handle the rest of breakage?

jiangtaoli2016 · 2019-03-26T21:09:13Z

We security team always wanted a more comprehensive gRPC test coverage against boringSSL and various versions of OpenSSL. I made such request 1+ years ago. Not sure what is the progress.

At least Distribution Tests Linux checks compilation error again old OpenSSL.

Fix on the way. @soheilhy I will take care of it.

soheilhy · 2019-03-26T21:13:29Z

Awesome! Thank you @jiangtaoli2016 !

nicolasnoble · 2019-03-26T21:14:09Z

Yes, this is really fishy. We had the OpenSSL 1.0.1 test in there for a long time, since it's literally our distribution test for Debian Jessie that's been here since almost forever. I can confirm this is caused by #17868, but I have no idea why it didn't break then: https://source.cloud.google.com/results/invocations/e6f81e28-5d34-4003-8889-14948c13f18e/log

The same exact test ran then and went back green.

@jtattermusch that's really odd.

nicolasnoble · 2019-03-26T21:20:23Z

Ugh, I know exactly what's going on actually. Debian Jessie started shuffling things around in preparation of the LTS ending in June. The jessie-backports repository changed, and therefore, we've started picking up a DIFFERENT version of the Jessie libssl package. @murgatroid99 may have a better fix overall coming up by applying the solution from https://unix.stackexchange.com/questions/508724/failed-to-fetch-jessie-backports-repository

@jiangtaoli2016 please hold off making further changes to TSI until we try this first.

jiangtaoli2016 · 2019-03-26T21:22:55Z

SG. @nicolasnoble Let me know if I need to make the change in SSL TSI.

nicolasnoble · 2019-03-29T16:46:03Z

Alright, #18530 is properly fixing this. I'm not going to issue a revert of this one, but I'm glad to see that the problem was an external factor, and not our code.

soheilhy · 2019-03-29T16:48:38Z

Awesome! @nicolasnoble FWIW, I wouldn't mind if you prefer reverting the hack here.

soheilhy requested a review from nicolasnoble March 22, 2019 10:57

soheilhy added the release notes: no Indicates if PR should not be in release notes label Mar 22, 2019

soheilhy force-pushed the fix-jwt branch from 7acdb3a to 5534fb2 Compare March 22, 2019 10:59

soheilhy mentioned this pull request Mar 22, 2019

Preallocate for incoming metadatas. #18474

Merged

vjpai requested a review from yihuazhang March 23, 2019 23:52

vjpai approved these changes Mar 23, 2019

View reviewed changes

This was referenced Mar 25, 2019

Fix Potentially Dangerous Typo - locked function being run outside of combiner #18482

Merged

Remove unnecessary hack which causes data races #18481

Merged

nicolasnoble mentioned this pull request Mar 26, 2019

C++ distribtests consistently failing on master #18477

Closed

nicolasnoble approved these changes Mar 26, 2019

View reviewed changes

jiangtaoli2016 approved these changes Mar 26, 2019

View reviewed changes

soheilhy merged commit 65ec942 into grpc:master Mar 26, 2019

yashykt mentioned this pull request Mar 27, 2019

Use GRPC_CLOSURE_SCHED instead of GRPC_CLOSURE_RUN in complete_closure_step #18491

Merged

lidizheng added the priority/P0/RELEASE BLOCKER label Mar 27, 2019

nicolasnoble mentioned this pull request Mar 29, 2019

Revert "Fix compilation error in JWT using const_cast." #18572

Merged

lock bot locked as resolved and limited conversation to collaborators Jun 27, 2019

Conversation

soheilhy commented Mar 22, 2019

Uh oh!

soheilhy commented Mar 22, 2019

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai commented Mar 26, 2019

Uh oh!

soheilhy commented Mar 26, 2019 via email

Uh oh!

yihuazhang commented Mar 26, 2019

Uh oh!

soheilhy commented Mar 26, 2019 via email

Uh oh!

yihuazhang commented Mar 26, 2019

Uh oh!

nicolasnoble commented Mar 26, 2019

Uh oh!

nicolasnoble commented Mar 26, 2019

Uh oh!

soheilhy commented Mar 26, 2019

Uh oh!

nicolasnoble commented Mar 26, 2019

Uh oh!

jiangtaoli2016 commented Mar 26, 2019

Uh oh!

soheilhy commented Mar 26, 2019

Uh oh!

jiangtaoli2016 commented Mar 26, 2019

Uh oh!

soheilhy commented Mar 26, 2019

Uh oh!

nicolasnoble commented Mar 26, 2019

Uh oh!

nicolasnoble commented Mar 26, 2019

Uh oh!

jiangtaoli2016 commented Mar 26, 2019

Uh oh!

nicolasnoble commented Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soheilhy commented Mar 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nicolasnoble commented Mar 29, 2019 •

edited

Loading