Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL ...................................................................... Patch Set 18: Code-Review+1 (7 comments) Looks good enough, just some nits left. I'd suggest to address the rest of those non-nit issues in a separate changelist: as I understand, the original scope of this patch is just to add an extra test. http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@397 PS16, Line 397: NewTempWritableFile(opts, &name_[0], &created_filename, &tmp_file); > So the TempTestDataFile class should have a std::unique_ptr<WritableFile> t Right: I'd think it worked as that. And once the instance of this file is destroyed, the temporary file is automatically deleted when the destructor for the tmp_file_ member is called. http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@1225 PS16, Line 1225: > JWKSMockServer will be replaced in a followup patch with the MiniOIDC compo I'm totally fine with addressing that in its own patch, especially if some of the components are going to be replaced in future patches, sure. I'm advocating for more test coverage not because of some whim, but because of the following: * being more explicit how those components behave: it helps us better understand what's our expectations for this new JWT functionality * make sure those 'negative' scenarios are handled as expected: nothing crashes, no undefined behavior is manifested * be able to spot regressions in the future http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@193 PS16, Line 193: return Stat > since the FindOrNull returns the value already from the map, this line beco Yep, that's right http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@611 PS16, Line 611: RETURN_NOT_OK_PREPEND(curl.F > We can add a flag for this, but I'm not sure I understand the attack vector I'd suggest to address this in a separate changelist: the context of this patch has become too bloated already. As for possible attack vectors. If somebody gives us wrong public keys, then all valid tokens suddenly start failing upon an attempt to verify them. That's a vector for DOS attacks. If a client is given a JWT generated by a malicious actor and signed with their private key, once they are able to substitute authentic public keys with their own, suddenly their tokens are valid and the system will allow those clients to authenticate. That gives the malicious party a way to start using the system, allowing non-authorized access. Does it make sense or those concerns are not valid becaues I'm missing something? http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@615 PS16, Line 615: jwks_checksum_ = > I haven't really found anything about this in example implementations. Howe I guess we can discuss that with Wenzhe and clarify using docs and other information available so far. I'd suggest to address this in a separate changelist since the scope of this review item is already pretty bloated (it started with just adding an extra test). http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc@620 PS18, Line 620: *is_changed = true; It seems I missed to point at this earlier, but ideally should be moved to just precede 'return Status::OK()' at 643. Nothing is actually changed before the newly downloaded content is successfully parsed at line 641, right? http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc@739 PS18, Line 739: if (new_jwks->IsEmpty()) { : // new_jwks-> : } nit: is it some left-over from the editing? If not, maybe add a TODO instead with explicit description what's to address here? -- To view, visit http://gerrit.cloudera.org:8080/18468 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8 Gerrit-Change-Number: 18468 Gerrit-PatchSet: 18 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Fri, 18 Nov 2022 19:12:17 +0000 Gerrit-HasComments: Yes
