Todd Lipcon has posted comments on this change. Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed ......................................................................
Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/5820/1//COMMIT_MSG Commit Message: Line 19: TODO: Add tests. Still working on it. I think with our minikdc, it should be possible to set the TGT expiration time to something like 5 or 10 seconds, and then verify that the renewal works properly by having the test run for 30 seconds or so? http://gerrit.cloudera.org:8080/#/c/5820/1/src/kudu/security/init.cc File src/kudu/security/init.cc: Line 38: DEFINE_int32(kerberos_reinit_interval, 60, "Duration in minutes before which an attempt to " > I don't have much context on the security work going on in Kudu, but I was yea, I agree we should be able to do this dynamically based on the expiration time. Basing it on the messenger is nice but I agree the circular dependency makes it troublesome. I believe the LevelDB thread wrapper has some kind of 'SchedulePeriodicTask()' helper which allows all of these various periodic-but-light-weight things to run on a single thread, instead of a bunch of separate ones... but either way, what's one more thread on top of our 1000+? :-D Line 53: struct KinitContext { it seems like this global instance is accessed a bunch by some free functions that could just as well be class members (and avoid a bunch of 'kinit_ctx->' expressions)? Line 62: static scoped_refptr<Thread> renew_thread; > warning: 'renew_thread' is a static definition in anonymous namespace; stat hrm, does this need to be kept around? Our default behavior if you just drop a Thread reference is to detach the thread, which is what we want. Line 71: #define KRB5_LOG_NOT_OK_ERROR(call, prepend_msg, ret) \ I think you could avoid needing this macro by instead moving the body of the renewal thread into a new function like: Status DoRenewal(); and then just use the existing KRB5_RETURN_NOT_OK_PREPEND within that function, and at the call site use WARN_NOT_OK(DoRenewal()) Line 86: SleepFor(MonoDelta::FromSeconds(FLAGS_kerberos_reinit_interval * 60)); > Shouldn't this thread participate in the rest of the server lifecycle thoug I think this only runs in the "real daemon" context, in which case there is no "shutdown", there is only getting killed. eg I don't think we attempt to shut down our log rotater thread, async logger threads, etc, and that's ok Line 108: strcmp(creds.server->data[1].data, kinit_ctx->principal->realm.data) || Looking at the 'is_local_tgt' function in krb5:src/clients/klist/klist.c it does: return princ->length == 2 && data_eq(princ->realm, *realm) && data_eq_string(princ->data[0], KRB5_TGS_NAME) && data_eq(princ->data[1], *realm); we seem to be missing the check on creds.server->realm (not sure when it might differ from the creds.server->data[1] but apparently they might?) It might also be worth referencing this function in a comment so we know where this incantation came from PS1, Line 109: creds.server->length != 2) I think this probably belongs first in the series of conditions, otherwise we risk dereferencing data[0] and data[1] when they don't exist. Line 110: krb5_free_cred_contents(kinit_ctx->krb5_ctx, &creds); can you do this as a scoped cleanup up above to avoid repeating it three times? PS1, Line 115: MonoDelta I don't know if MonoDelta is giving us much in this case. Typically we use monotime/monodelta only in the case when it's truly a monotonic clock reading, whereas here we have wall times. PS1, Line 120: krb5_creds new_creds; : auto cleanup_new_creds = MakeScopedCleanup([&]() { : krb5_free_cred_contents(kinit_ctx->krb5_ctx, &new_creds); }); I believe for this to be safe, you'd need to memset the 'new_creds' object to \0s first, right? Otherwise it is arbitrary data and the free call could crash? Line 132: kinit_ctx->principal, alignment slightly off Line 145: if (ret) goto end; if you use a ScopedCleanup up above, then no need for the gotos and such here. -- To view, visit http://gerrit.cloudera.org:8080/5820 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
