You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2017/01/28 01:34:02 UTC
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Sailesh Mukil has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5820
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
KUDU-1845: Kerberos client keytab should be periodically renewed
With the current kerberos support, if a ticket expires, all
communication on a secure cluster would stop as we do not attempt
to renew the ticket.
This patch adds a renewal thread which periodically wakes up and
either renews the Ticket Granting Ticket or acquires a new one if
the renewal window is closed. The renew interval is controlled by
a newly introduced flag called 'kerberos_reinit_interval'.
Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
TODO: Add tests. Still working on it.
---
M src/kudu/security/init.cc
1 file changed, 137 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5820/1
--
To view, visit http://gerrit.cloudera.org:8080/5820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5820
to look at the new patch set (#10).
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
KUDU-1845: Kerberos client keytab should be periodically renewed
With the current kerberos support, if a ticket expires, all
communication on a secure cluster would stop as we do not attempt
to renew the ticket.
This patch adds a renewal thread which periodically wakes up and
either renews the Ticket Granting Ticket or acquires a new one if
the renewal window is closed. The renew interval is controlled by
a newly introduced flag called 'kerberos_reinit_interval'.
A reader-writer lock is used to avoid a race at the time of ticket
reinitializtaion, which can occur between the time the credential
cache is reinitialized and the time the new credentials are placed
in the cache. The reader lock is taken before any call to the
SASL library (if kerberos is enabled) and the writer lock is
taken before reinitializing the cache and placing the new creds
in it.
Testing: Added 2 tests, one for ticket renewal and one for ticket
reacquisition. I haven't added tests to confirm if disabling the
renew thread makes these tests fail as that would require quite
some plumbing which I think is unnecessary. Instead, I manually
tested that disabling the renew thread causes the tests to fail.
Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
8 files changed, 318 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5820/10
--
To view, visit http://gerrit.cloudera.org:8080/5820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 6:
> Uploaded patch set 6.
I just had to #include <random>
The util/random library wouldn't have helped here because we need to seed it with something random as well. If we use the same seed across nodes, we will end up with the same random number on all the nodes, which means all of them will still end up flooding the KDC at the same time, completely defeating the purpose.
--
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
Re: [kudu-CR] KUDU-1845: Kerberos client keytab should be
periodically renewed
Posted by Sailesh Mukil <sa...@cloudera.com>.
Will fix it now and post an update.
On Fri, Feb 3, 2017 at 9:34 AM, Todd Lipcon (Code Review) <
gerrit@cloudera.org> wrote:
> Todd Lipcon has posted comments on this change.
>
> Change subject: KUDU-1845: Kerberos client keytab should be periodically
> renewed
> ......................................................................
>
>
> Patch Set 5:
>
> seems the TSAN build doesn't like the std::random stuff (maybe it's not
> available in libc++?) We have our own RNG in util/random.h
>
> --
> 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: 5
> Gerrit-Project: kudu
> Gerrit-Branch: master
> Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
> Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
> Gerrit-Reviewer: Kudu Jenkins
> Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
> Gerrit-Reviewer: Tidy Bot
> Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
> Gerrit-HasComments: No
>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 5:
seems the TSAN build doesn't like the std::random stuff (maybe it's not available in libc++?) We have our own RNG in util/random.h
--
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 8:
(2 comments)
> Build Failed
>
> http://104.196.14.100/job/kudu-gerrit/6321/ : FAILURE
Will address these asap.
http://gerrit.cloudera.org:8080/#/c/5820/7/src/kudu/security/init.cc
File src/kudu/security/init.cc:
Line 108: return (d1.length == d2.length && (d1.length == 0 ||
> warning: 'data_eq' is a static definition in anonymous namespace; static is
Done
Line 115: !memcmp(d.data, s, d.length)));
> warning: 'data_eq_string' is a static definition in anonymous namespace; st
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP: KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5820
to look at the new patch set (#3).
Change subject: WIP: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
WIP: KUDU-1845: Kerberos client keytab should be periodically renewed
With the current kerberos support, if a ticket expires, all
communication on a secure cluster would stop as we do not attempt
to renew the ticket.
This patch adds a renewal thread which periodically wakes up and
either renews the Ticket Granting Ticket or acquires a new one if
the renewal window is closed. The renew interval is controlled by
a newly introduced flag called 'kerberos_reinit_interval'.
Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/security/init.cc
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
6 files changed, 193 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5820/3
--
To view, visit http://gerrit.cloudera.org:8080/5820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 10: Code-Review+2
--
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: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5820
to look at the new patch set (#4).
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
KUDU-1845: Kerberos client keytab should be periodically renewed
With the current kerberos support, if a ticket expires, all
communication on a secure cluster would stop as we do not attempt
to renew the ticket.
This patch adds a renewal thread which periodically wakes up and
either renews the Ticket Granting Ticket or acquires a new one if
the renewal window is closed. The renew interval is controlled by
a newly introduced flag called 'kerberos_reinit_interval'.
Testing: Added 2 tests, one for ticket renewal and one for ticket
reacquisition. I haven't added tests to confirm if disabling the
renew thread makes these tests fail as that would require quite
some plumbing which I think is unnecessary. Instead, I manually
tested that disabling the renew thread causes the tests to fail.
Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/security/init.cc
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
6 files changed, 212 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5820/4
--
To view, visit http://gerrit.cloudera.org:8080/5820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5820
to look at the new patch set (#8).
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
KUDU-1845: Kerberos client keytab should be periodically renewed
With the current kerberos support, if a ticket expires, all
communication on a secure cluster would stop as we do not attempt
to renew the ticket.
This patch adds a renewal thread which periodically wakes up and
either renews the Ticket Granting Ticket or acquires a new one if
the renewal window is closed. The renew interval is controlled by
a newly introduced flag called 'kerberos_reinit_interval'.
A reader-writer lock is used to avoid a race at the time of ticket
reinitializtaion, which can occur between the time the credential
cache is reinitialized and the time the new credentials are placed
in the cache. The reader lock is taken before any call to the
SASL library (if kerberos is enabled) and the writer lock is
taken before reinitializing the cache and placing the new creds
in it.
Testing: Added 2 tests, one for ticket renewal and one for ticket
reacquisition. I haven't added tests to confirm if disabling the
renew thread makes these tests fail as that would require quite
some plumbing which I think is unnecessary. Instead, I manually
tested that disabling the renew thread causes the tests to fail.
Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
8 files changed, 284 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5820/8
--
To view, visit http://gerrit.cloudera.org:8080/5820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 4:
> So the ASAN build fails because the renew thread is leaking all its allocations, since the thread runs forever and most of the objects are never freed.
I think the leak is actually that the scoped_cleanup for 'creds' should be inside the krb5_cc_next_cred(...) loop, since each iteration creates a new creds object, right?
BTW I'm also seeing a heap buffer overflow in the current rev of the patch
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5820/6/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS6, Line 189: if (krb5_init_context(&krb5_ctx_) != 0) {
: return Status::RuntimeError("could not initialize krb5 library");
> I'm not sure this is accomplishing what we want. Doesn't this just end up w
You're right. I looked at the krb5 code and this call just returns the handle to the old ccache.
As we spoke, the only known workaround to this is to not renew the tickets and just reacquire the tickets every time. I've removed the renewal code, and just left the reacquire code.
However, this race still exists when using Heimdal's krb5 and there's no workaround for that. I've left a comment explaining that.
Line 248: KRB5_RETURN_NOT_OK_PREPEND(krb5_cc_store_cred(krb5_ctx_, ccache_, &creds),
> isn't it possible that the renewal time changes when we renew the ticket? m
How so? The renewal time is based off the 'ticket_lifetime' which is a config value in krb5.conf.
So 'endtime - now' will always be the same assuming 'now' is the 'start_time' which it almost always will be.
Are you talking about the case where someone changes that value in the conf file midway?
--
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 1:
(2 comments)
Just passing through...
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 wondering: would it be possible to use the expiration of the Kerberos credentials to dictate how long to sleep? That is, if we know our tickets are expiring in 6 hours, we can sleep until then, rather than waking up periodically?
Ideally we wouldn't even need a thread, but schedule the future work as a task. Unfortunately, that facility is provided by the messenger in krpc, so using it might create a circular dependency.
Line 86: SleepFor(MonoDelta::FromSeconds(FLAGS_kerberos_reinit_interval * 60));
Shouldn't this thread participate in the rest of the server lifecycle though? That is, if the server is shutdown, shouldn't we explicitly stop this thread?
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5820/6/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS6, Line 189: KRB5_RETURN_NOT_OK_PREPEND(krb5_cc_default(krb5_ctx_, new_ccache),
: "unable to get default credentials cache");
I'm not sure this is accomplishing what we want. Doesn't this just end up with krb5_ccache pointing at the same ticket cache that the old one was, rather than creating a new one?
Line 248: int32_t interval = creds.times.endtime - time(nullptr);
isn't it possible that the renewal time changes when we renew the ticket? maybe we should recalculate our "next renewal time" every time we renew?
--
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5820
to look at the new patch set (#6).
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
KUDU-1845: Kerberos client keytab should be periodically renewed
With the current kerberos support, if a ticket expires, all
communication on a secure cluster would stop as we do not attempt
to renew the ticket.
This patch adds a renewal thread which periodically wakes up and
either renews the Ticket Granting Ticket or acquires a new one if
the renewal window is closed. The renew interval is controlled by
a newly introduced flag called 'kerberos_reinit_interval'.
Testing: Added 2 tests, one for ticket renewal and one for ticket
reacquisition. I haven't added tests to confirm if disabling the
renew thread makes these tests fail as that would require quite
some plumbing which I think is unnecessary. Instead, I manually
tested that disabling the renew thread causes the tests to fail.
Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/security/init.cc
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
6 files changed, 273 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5820/6
--
To view, visit http://gerrit.cloudera.org:8080/5820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5820/4/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS4, Line 125: strcmp(creds.server->data[1].data, principal_->realm.data) != 0 ||
: strcmp(creds.server->data[0].data, KRB5_TGS_NAME) != 0 ||
: strcmp(creds.server->realm.data, principal_->realm.data) != 0) {
:
the kerberos code uses a 'data_eq' helper here which also uses the length field of the data (and probably avoids the heap buffer overflow that ASAN is hitting)
Line 134: time_t renew_deadline = renew_till - 30;
we may want some jitter on this. As is, it seems like if you started up 100 servers, they'd all get their initial ticket at the same time. Then, they'd all hit this 30-second-before-expiration window at the same moment and cause a coordinated flood of load at the KDC at that moment. Some randomization like between 30 seconds and 5 minutes would probably be a good idea (potentailly also a random sleep before the first renewal attempt so that the checks themselves are offset)
PS4, Line 144: // Acquire a new ticket using the keytab. This ticket will automatically be put into the
: // credential cache.
:
this probably needs the same OSX workaround performed below in the initial kinit
PS4, Line 157: // 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");
:
does this leave open a race where a connection might be attempted in between the two calls, see no TGT, and fail? is there a way to atomically replace the cred? (what happens if you don't use krb5_cc_initialize to reset it first?)
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 4:
Thanks Todd. As Todd fixed in the patch, the issue was that the client had a separate Kinit path and there was no renewal for that path which meant that the client could not communicate with the master or tserver after the ticket expired.
The fix was to ensure communication between the master and the tserver worked well after the ticket expired.
I added another test to check if acquiring a new ticket works as well (as opposed to renewing the existing one).
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5820
to look at the new patch set (#7).
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
KUDU-1845: Kerberos client keytab should be periodically renewed
With the current kerberos support, if a ticket expires, all
communication on a secure cluster would stop as we do not attempt
to renew the ticket.
This patch adds a renewal thread which periodically wakes up and
acquires a new ticket. We don't attempt to renew the existing
ticket as there's a fundamental race in how ticket renewal works,
as explained in the comment above KinitContext::DoRenewal().
Testing: Added a test for ticket reacquisition.
I haven't added tests to confirm if disabling the renew thread
makes these tests fail as that would require quite some plumbing
which I think is unnecessary. Instead, I manually tested that
disabling the renew thread causes the tests to fail.
Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/security/init.cc
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
6 files changed, 225 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5820/7
--
To view, visit http://gerrit.cloudera.org:8080/5820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5820/1/src/kudu/security/init.cc
File src/kudu/security/init.cc:
Line 86: SleepFor(MonoDelta::FromSeconds(FLAGS_kerberos_reinit_interval * 60));
> Will this be OK in MiniCluster, where all the servers are in-proc and we ex
In MiniCluster I don't think any of this code gets called (because it deals with process-wide state). Though, maybe I'm wrong about that? Sailesh?
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5820/1/src/kudu/security/init.cc
File src/kudu/security/init.cc:
Line 86: SleepFor(MonoDelta::FromSeconds(FLAGS_kerberos_reinit_interval * 60));
> In MiniCluster I don't think any of this code gets called (because it deals
AFAICT it is called in MiniClusters. Here's a call stack:
1. MiniCluster::AddTabletServer()
2. MiniTabletServer::Start()
3. TabletServer::Init()
4. ServerBase::Init()
5. security::InitKerberosForServer()
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5820/1/src/kudu/security/init.cc
File src/kudu/security/init.cc:
Line 86: SleepFor(MonoDelta::FromSeconds(FLAGS_kerberos_reinit_interval * 60));
> I think this only runs in the "real daemon" context, in which case there is
Will this be OK in MiniCluster, where all the servers are in-proc and we explicitly shutdown to restart them? FWIW, in that context the log rotator thread does get shut down (see ServerBase::Shutdown), but the async logger thread does not.
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 4:
So the ASAN build fails because the renew thread is leaking all its allocations, since the thread runs forever and most of the objects are never freed.
I tried adding a security::Shutdown() which gets called from ServerBase::Shutdown(), but that seems to not be of any use in the case of the tests since an ExternalDaemon is killed and not shutdown properly. So CheckForLeaks() would still complain and fail the test.
The only option I see is to have a ScopedLeakCheckDisabler in the scope of the renew thread. However, that too makes me nervous since a lot of the structures can have nested allocations and future changes could introduce bugs easily.
What do you think is the right path of action 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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#6).
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
KUDU-1845: Kerberos client keytab should be periodically renewed
With the current kerberos support, if a ticket expires, all
communication on a secure cluster would stop as we do not attempt
to renew the ticket.
This patch adds a renewal thread which periodically wakes up and
either renews the Ticket Granting Ticket or acquires a new one if
the renewal window is closed. The renew interval is controlled by
a newly introduced flag called 'kerberos_reinit_interval'.
Testing: Added 2 tests, one for ticket renewal and one for ticket
reacquisition. I haven't added tests to confirm if disabling the
renew thread makes these tests fail as that would require quite
some plumbing which I think is unnecessary. Instead, I manually
tested that disabling the renew thread causes the tests to fail.
Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/security/init.cc
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
6 files changed, 273 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5820/6
--
To view, visit http://gerrit.cloudera.org:8080/5820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 10:
lgtm, i'll put this on a cluster and see how it goes.
--
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: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5820
to look at the new patch set (#2).
Change subject: WIP: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
WIP: KUDU-1845: Kerberos client keytab should be periodically renewed
With the current kerberos support, if a ticket expires, all
communication on a secure cluster would stop as we do not attempt
to renew the ticket.
This patch adds a renewal thread which periodically wakes up and
either renews the Ticket Granting Ticket or acquires a new one if
the renewal window is closed. The renew interval is controlled by
a newly introduced flag called 'kerberos_reinit_interval'.
Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/security/init.cc
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
6 files changed, 191 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5820/2
--
To view, visit http://gerrit.cloudera.org:8080/5820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5820
to look at the new patch set (#5).
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
KUDU-1845: Kerberos client keytab should be periodically renewed
With the current kerberos support, if a ticket expires, all
communication on a secure cluster would stop as we do not attempt
to renew the ticket.
This patch adds a renewal thread which periodically wakes up and
either renews the Ticket Granting Ticket or acquires a new one if
the renewal window is closed. The renew interval is controlled by
a newly introduced flag called 'kerberos_reinit_interval'.
Testing: Added 2 tests, one for ticket renewal and one for ticket
reacquisition. I haven't added tests to confirm if disabling the
renew thread makes these tests fail as that would require quite
some plumbing which I think is unnecessary. Instead, I manually
tested that disabling the renew thread causes the tests to fail.
Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/security/init.cc
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
6 files changed, 273 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5820/5
--
To view, visit http://gerrit.cloudera.org:8080/5820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5820
to look at the new patch set (#9).
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
KUDU-1845: Kerberos client keytab should be periodically renewed
With the current kerberos support, if a ticket expires, all
communication on a secure cluster would stop as we do not attempt
to renew the ticket.
This patch adds a renewal thread which periodically wakes up and
either renews the Ticket Granting Ticket or acquires a new one if
the renewal window is closed. The renew interval is controlled by
a newly introduced flag called 'kerberos_reinit_interval'.
A reader-writer lock is used to avoid a race at the time of ticket
reinitializtaion, which can occur between the time the credential
cache is reinitialized and the time the new credentials are placed
in the cache. The reader lock is taken before any call to the
SASL library (if kerberos is enabled) and the writer lock is
taken before reinitializing the cache and placing the new creds
in it.
Testing: Added 2 tests, one for ticket renewal and one for ticket
reacquisition. I haven't added tests to confirm if disabling the
renew thread makes these tests fail as that would require quite
some plumbing which I think is unnecessary. Instead, I manually
tested that disabling the renew thread causes the tests to fail.
Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
8 files changed, 289 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5820/9
--
To view, visit http://gerrit.cloudera.org:8080/5820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
KUDU-1845: Kerberos client keytab should be periodically renewed
With the current kerberos support, if a ticket expires, all
communication on a secure cluster would stop as we do not attempt
to renew the ticket.
This patch adds a renewal thread which periodically wakes up and
either renews the Ticket Granting Ticket or acquires a new one if
the renewal window is closed. The renew interval is controlled by
a newly introduced flag called 'kerberos_reinit_interval'.
A reader-writer lock is used to avoid a race at the time of ticket
reinitializtaion, which can occur between the time the credential
cache is reinitialized and the time the new credentials are placed
in the cache. The reader lock is taken before any call to the
SASL library (if kerberos is enabled) and the writer lock is
taken before reinitializing the cache and placing the new creds
in it.
Testing: Added 2 tests, one for ticket renewal and one for ticket
reacquisition. I haven't added tests to confirm if disabling the
renew thread makes these tests fail as that would require quite
some plumbing which I think is unnecessary. Instead, I manually
tested that disabling the renew thread causes the tests to fail.
Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Reviewed-on: http://gerrit.cloudera.org:8080/5820
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
8 files changed, 318 insertions(+), 43 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 10:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/5820/9/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS9, Line 69:
: // Acquires a new Ticket Granting Ticket (TGT).
> is this still accurate? If this method isn't renewing a ticket, but rather
Whoops sorry, I forgot to revert the comment. It does renewal again now.
Line 83: // Returns a value based on 'time_remaining' that increases exponentially with
> please rename to include units (sleep_interval_secs()) or have it return a
Renamed to sleep_interval_secs()
PS9, Line 95:
> nit: name g_kinit_ctx
Done
Line 100:
> I think non-POD global data is a no-no. Better to use a pointer and explici
Done
PS9, Line 120: entation
> I don't think the d.length check is necessary, since memcmp with 0 length i
Done
Line 176:
> did we determine that the underlying get_init_creds_keytab held the appropr
It doesn't use internal locks, but it looks like it retries once if it doesn't go through the first time but that won't be enough, so I'll wrap the lock around it.
PS9, Line 197: // This follows the same format as is_local_tgt() from krb5:src/clients/klist/klist.c
: if (creds.server->length != 2 ||
: data_eq(creds.server->data[1], principal_->realm) == 0 ||
> isn't this already set when we initialized opts_?
Oh yea, this is redundant. Removed it.
PS9, Line 203: }
:
: time_t now = time(nullptr);
: time_t ticket_expiry = creds.times.endtime;
: time_t renew_till = creds.times.renew_till;
: time_t renew_deadline = renew_till - 30;
:
: krb5_creds new_creds;
> wrap this in a block so that the log message below isn't holding the lock
Done
PS9, Line 252: KRB5_RETURN_NOT_OK_PREPEND(krb5_cc_store_cred(krb5_ctx_, ccache_, &new_creds),
: "Failed to store credentials in ccache");
: }
: LOG(INFO) << "Successfully renewed kerberos TGT";
: }
: ticket_end_timestamp_ = new_creds.times.endtime;
: break;
: }
> I'm worried that, since we're just setting the interval to wae up just befo
Added a backoff mechanism on failures, and also this is recalculated after every ticket renewal.
http://gerrit.cloudera.org:8080/#/c/5820/9/src/kudu/security/init.h
File src/kudu/security/init.h:
Line 20:
> can just use a forward decl
Done
Line 29: Status InitKerberosForServer();
> I think it's worth documenting this further in the header. Something like:
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: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 5:
(6 comments)
@Todd: Thanks, I got the ASAN build working. Aside from the code comments, I had a very silly bug in an intermediate patch that I got rid of.
http://gerrit.cloudera.org:8080/#/c/5820/4/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:
Line 35: #include "kudu/server/server_base.pb.h"
> error: 'kudu/master/master_rpc.h' file not found [clang-diagnostic-error]
Done
http://gerrit.cloudera.org:8080/#/c/5820/4/src/kudu/security/init.cc
File src/kudu/security/init.cc:
Line 81: krb5_context krb5_ctx_;
> warning: 'kinit_ctx' is a static definition in anonymous namespace; static
Done
PS4, Line 125: auto cleanup_cursor = MakeScopedCleanup([&]() {
: krb5_cc_end_seq_get(krb5_ctx_, *ccache_, &cursor); });
:
:
> the kerberos code uses a 'data_eq' helper here which also uses the length f
I ported in the data_eq functions (they're very small), and that did away with the heap-buffer overflow. There still is a leak though that I'm unable to track down.
Line 134: auto cleanup_creds = MakeScopedCleanup([&]() {
> we may want some jitter on this. As is, it seems like if you started up 100
You're right, but adding a jitter here wouldn't help, as this is just a deadline and if all 100 servers try to renew at the same point in time before this deadline, they would still flood the servers.
I've added the jitter while setting the sleep_interval_ in Kinit(), so that each server, with a high probability, wakes up at a different time than all or most other servers.
PS4, Line 144: continue;
: }
:
> this probably needs the same OSX workaround performed below in the initial
Done
PS4, Line 157: // If the ticket has already expired or if there's only a short period before which the
: // renew window closes, we acquire a new ticket.
: if (ticket_expiry < now || renew_deadline < now) {
: // Acquire a new ticket using the keytab. This ticket will automatically be put into the
: // credential cache.
: KRB5_RETURN_NOT_OK_PREPEND(krb5_get_init_creds_keytab(krb5_ctx_, &new_creds, principal_,
:
> does this leave open a race where a connection might be attempted in betwee
Good catch. To verify, I manually added a sleep between both the function calls and authentication of the peer failed.
I changed it so that we allocate a new ccache and store the credentials there and swap out the old cache for the new one, before closing and deleting the old ccache.
Odd how most other implementations of this don't take care of this case.
--
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 1:
I'm having a hard time testing it properly as I don't know how to set up a kerberos cluster manually.
I tested it with the MiniKdc manually by adding sleeps to the external mini cluster test and it seems to be working fine, but I still would like to run it in a real cluster to check if it works well. I'll try to have an update on that soon.
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[kudu-CR] KUDU-1845: Kerberos client keytab should be periodically renewed
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5820/1/src/kudu/security/init.cc
File src/kudu/security/init.cc:
Line 86: SleepFor(MonoDelta::FromSeconds(FLAGS_kerberos_reinit_interval * 60));
> AFAICT it is called in MiniClusters. Here's a call stack:
Yea, but we never currently configure security in miniclusters, AFAIK, because it would mix state (like env vars) between all the participants in the cluster and the client and thus not give us much benefit.
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes