Sailesh Mukil has posted comments on this change. Change subject: WIP: KUDU-1845: Kerberos client keytab should be periodically renewed ......................................................................
Patch Set 2: (17 comments) Sorry for the slow response, I kept getting pulled onto other things. Will respond more quickly henceforth. Anyway, the testing has been going on rather unsuccessfully. For one, even though I set the ticket lifetime as 5 seconds, unless I ping the peer ~3 minutes after the initial connection negotiation, it always succeeds (even when I disable the renewal thread). (This is probably because of a connection keep alive timeout as Todd mentioned) And two, even with the renewal thread running, after a ~3 minute sleep, when I try to ping the peer, the connection tries to renegotiate and fails stating "Ticket expired". But the KDC is actually renewing the tickets as I can see the KDC logs being printed out periodically. I'm not able to explain why the new ticket is not being picked up. It would be great to have another pair of eyes on the test to help debug the issue. http://gerrit.cloudera.org:8080/#/c/5820/1/src/kudu/security/init.cc File src/kudu/security/init.cc: Line 38: // TODO(todd): this currently only affects the keytab login which is used > yea, I agree we should be able to do this dynamically based on the expirati I wasn't able to find this LevelDB thread wrapper, so I've just left it as a thread for now. I also got rid of the flag and am now doing it based on the expiration time. Line 53: > it seems like this global instance is accessed a bunch by some free functio Done Line 62: > hrm, does this need to be kept around? Our default behavior if you just dro Ah, I wasn't aware of that. Thanks. Done. Line 71: krb5_context* krb5_ctx() { return &krb5_ctx_; } > I think you could avoid needing this macro by instead moving the body of th Good idea. Done. Line 72: > warning: macro argument should be enclosed in parentheses [misc-macro-paren Done Line 73: private: > warning: macro argument should be enclosed in parentheses [misc-macro-paren Done Line 74: krb5_principal principal_; > warning: macro argument should be enclosed in parentheses [misc-macro-paren Done Line 74: krb5_principal principal_; > warning: macro argument should be enclosed in parentheses [misc-macro-paren Done Line 86: } > Yea, but we never currently configure security in miniclusters, AFAIK, beca Thanks Adar and Todd, I wasn't too sure how the minicluster shutdown worked. My take was that since this is process-wide, we don't need to have a shutdown (as that's the case in a bunch of other places). Should I leave a TODO stating that a shutdown routine needs to be added if MiniClusters with security is supported later on? Line 107: KRB5_RETURN_NOT_OK_PREPEND(krb5_cc_start_seq_get(krb5_ctx_, ccache_, &cursor), > warning: function 'strcmp' is called without explicitly comparing result [m Done Line 108: "Failed to peek into ccache"); > Looking at the 'is_local_tgt' function in krb5:src/clients/klist/klist.c it For our purposes, it should be the same. I tried looking through the docs to see when they could be different, but to no avail. I added the check just to be safe though and also referenced the function in a comment. PS1, Line 109: anup_cursor = MakeScopedCl > I think this probably belongs first in the series of conditions, otherwise Oops, yes. Done. Line 110: krb5_cc_end_seq_get(krb5_ctx_, ccache_, &cursor); }); > can you do this as a scoped cleanup up above to avoid repeating it three ti Done PS1, Line 115: krb5_free > I don't know if MonoDelta is giving us much in this case. Typically we use Done PS1, Line 120: if (krb5_is_config_principal(krb5_ctx_, creds.server)) continue; : : // We only want to renew the TGT (Ticket Granting Ticket). Ignore a > I believe for this to be safe, you'd need to memset the 'new_creds' object You're right. I missed that somehow. Done. Line 132: time_t ticket_expiry = creds.times.endtime; > alignment slightly off Done Line 145: // credential cache. > if you use a ScopedCleanup up above, then no need for the gotos and such he Done -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
