Todd Lipcon has posted comments on this change. Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed ......................................................................
Patch Set 9: (11 comments) http://gerrit.cloudera.org:8080/#/c/5820/9/src/kudu/security/init.cc File src/kudu/security/init.cc: PS9, Line 69: // Does not do actual ticket renewal since there is a fundamental race in the way ticket : // renewal is done. Even from krb5:src/clients/kinit/kinit.c is this still accurate? If this method isn't renewing a ticket, but rather reacquiring, I think 'DoReacquire()' is probably a better name. Line 83: int32_t sleep_interval() { return sleep_interval_; } please rename to include units (sleep_interval_secs()) or have it return a MonoDelta so that units aren't confused PS9, Line 95: kinit_ctx nit: name g_kinit_ctx Line 100: RWMutex kerberos_reinit_lock(RWMutex::Priority::PREFER_WRITING); I think non-POD global data is a no-no. Better to use a pointer and explicitly leak it. Otherwise we can hit weird destruction-order related crashes PS9, Line 120: d.length I don't think the d.length check is necessary, since memcmp with 0 length is guaranteed to return 0, according to the man page. (same above) Line 176: KRB5_RETURN_NOT_OK_PREPEND(krb5_get_init_creds_keytab(krb5_ctx_, &new_creds, principal_, did we determine that the underlying get_init_creds_keytab held the appropriate internal locks such that it isn't necessary to wrap this in the writelock? I thought that even the krb5 library itself didn't prevent against this race. PS9, Line 197: KRB5_RETURN_NOT_OK_PREPEND(krb5_get_init_creds_opt_set_out_ccache(krb5_ctx_, opts_, : ccache_), : "unable to set init_creds options"); isn't this already set when we initialized opts_? PS9, Line 203: std::lock_guard<RWMutex> l(kerberos_reinit_lock); : // Clear existing credentials in cache. : KRB5_RETURN_NOT_OK_PREPEND(krb5_cc_initialize(krb5_ctx_, ccache_, principal_), : "Failed to re-initialize ccache"); : : // Store the new credentials in the cache. : KRB5_RETURN_NOT_OK_PREPEND(krb5_cc_store_cred(krb5_ctx_, ccache_, &new_creds), : "Failed to store credentials in ccache"); wrap this in a block so that the log message below isn't holding the lock PS9, Line 252: // If the interval between now and ticket expiry is: : // * > 10 minutes: We attempt to renew the ticket between 5 seconds and 5 minutes before the : // ticket expires. : // * 5 - 10 minutes: We attempt to renew the ticket betwen 5 seconds and 1 minute before the : // ticket expires. : // * < 5 minutes: Attempt to renew the ticket every 'interval'. : // The jitter is added to make sure that every server doesn't flood the KDC at the same time. : { I'm worried that, since we're just setting the interval to wae up just before expiration, that if there's any slight blip with the KDC (eg it's being restarted), we'll then go to sleep for an entire 2nd interval. I think we either need to reset the interval after each renewal attempt (regardless of failure) so that you get a very short interval just after a failure, or need to make it so that on failure it does some kind of backoff. http://gerrit.cloudera.org:8080/#/c/5820/9/src/kudu/security/init.h File src/kudu/security/init.h: Line 20: #include "kudu/util/rw_mutex.h" can just use a forward decl Line 29: // Returns the process lock 'kerberos_reinit_lock' I think it's worth documenting this further in the header. Something like: This lock is acquired in write mode while the ticket is being renewed, and acquired in read mode before using the SASL library which might require a ticket. -- 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: 9 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
