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/08/22 05:15:22 UTC
[kudu-CR] Adjust kerberos renewal logic to avoid tickets with NULL 'renew till' timestamp
Sailesh Mukil has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7770
Change subject: Adjust kerberos renewal logic to avoid tickets with NULL 'renew_till' timestamp
......................................................................
Adjust kerberos renewal logic to avoid tickets with NULL 'renew_till' timestamp
It was found that if we use a file based credential cache that is
shared between the C++ side and the java side of a process, and we
encounter the specific edge case where we renew a ticket that has
less than 'ticket_lifetime' left before its 'renew_lifetime' expires,
the ticket is set to have a NULL 'renew_till' timestamp.
Eg:
ticket_lifetime = 10m
renew_lifetime = 100m
[current ticket being renewed] at '15:30:00'
endtime = '15:30:30'
renew_till = '15:31:00'
This ticket will be renewed and the renewed ticket will have the
following values:
endtime = '15:31:00'
renew_till = null
The Java krb5 library does not like this and doesn't handle these
kinds of tickets properly, causing unexpected failures. We work around
this by reqacquiring a new ticket instead of renewing the existing
ticket if there is less that 'ticket_lifetime' left between now and
the 'renew_till' deadline.
Change-Id: I59194af94838f680df4ce121a8dee526a876e369
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/security/init.cc
2 files changed, 6 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/7770/1
--
To view, visit http://gerrit.cloudera.org:8080/7770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[kudu-CR] Adjust kerberos renewal logic to avoid tickets with NULL 'renew till' timestamp
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Adjust kerberos renewal logic to avoid tickets with NULL 'renew_till' timestamp
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7770/1//COMMIT_MSG
Commit Message:
PS1, Line 28: does not like this
maybe be slightly more specific and say that the Java library refuses to read a ticket which has the RENEWABLE flag set, but no renew_till set.
PS1, Line 30: reqacquiring
typo
http://gerrit.cloudera.org:8080/#/c/7770/1/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS1, Line 290: difftime
am surprised to see 'difftime'. Never seen that before. Why not just cast to (signed) int64_ts?
PS1, Line 290: creds.times.starttime
according to http://web.mit.edu/kerberos/krb5-current/doc/appldev/refs/types/krb5_ticket_times.html#krb5_ticket_times the starttime may be missing, in which case we would have to fall back to 'authtime' instead. I've never seen it, but I think we should probably look whether that happens and at least make sure we do something sane in that case
PS1, Line 298: (now + ticket_lifetime) > renew_till
we probably want a little bit of slop here just like we had with 'renew_deadline'. Otherwise if we are exactly at the threshold where things break, we might still do a renewal and hit the issue, right?
--
To view, visit http://gerrit.cloudera.org:8080/7770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Adjust kerberos renewal logic to avoid tickets with NULL 'renew till' timestamp
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Adjust kerberos renewal logic to avoid tickets with NULL 'renew_till' timestamp
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7770/2/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS2, Line 295: ticket_lifetime
don't you still need a little slop here in addition to ticket_lifetime?
eg if your lifetime is 30s, and it's 12:00:29, and renew_till is 12:01:00, you only have 1 second left. So if the renewal takes more than a second you'll be back in the danger zone.
--
To view, visit http://gerrit.cloudera.org:8080/7770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 2
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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Adjust kerberos renewal logic to avoid tickets with NULL 'renew till' timestamp
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#2).
Change subject: Adjust kerberos renewal logic to avoid tickets with NULL 'renew_till' timestamp
......................................................................
Adjust kerberos renewal logic to avoid tickets with NULL 'renew_till' timestamp
It was found that if we use a file based credential cache that is
shared between the C++ side and the java side of a process, and we
encounter the specific edge case where we renew a ticket that has
less than 'ticket_lifetime' left before its 'renew_lifetime' expires,
the ticket is set to have a NULL 'renew_till' timestamp.
Eg:
ticket_lifetime = 10m
renew_lifetime = 100m
[current ticket being renewed] at '15:30:00'
endtime = '15:30:30'
renew_till = '15:31:00'
This ticket will be renewed and the renewed ticket will have the
following values:
endtime = '15:31:00'
renew_till = null
The Java krb5 library refuses to read these kinds of tickets which
have the RENEWABLE flag set but no 'renew_till' set, causing
unexpected failures. We work around this by reacquiring a new ticket
instead of renewing the existing ticket if there is less that
'ticket_lifetime' left between now and the 'renew_till' deadline.
Change-Id: I59194af94838f680df4ce121a8dee526a876e369
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/security/init.cc
2 files changed, 12 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/7770/2
--
To view, visit http://gerrit.cloudera.org:8080/7770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Adjust kerberos renewal logic to avoid tickets with NULL 'renew till' timestamp
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: Adjust kerberos renewal logic to avoid tickets with NULL 'renew_till' timestamp
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7770/1//COMMIT_MSG
Commit Message:
PS1, Line 28: refuses to read th
> maybe be slightly more specific and say that the Java library refuses to re
Done
PS1, Line 30: ed failures.
> typo
Done
http://gerrit.cloudera.org:8080/#/c/7770/1/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS1, Line 290:
> am surprised to see 'difftime'. Never seen that before. Why not just cast t
http://en.cppreference.com/w/c/chrono/difftime
"On POSIX systems, time_t is measured in seconds, and difftime is equivalent to arithmetic subtraction, but C and C++ allow fractional units for time_t."
Also, just looking at posts online generally, people discourage using '-' to subtract two time_t objects.
PS1, Line 290:
> according to http://web.mit.edu/kerberos/krb5-current/doc/appldev/refs/type
Good catch. I added some logic to handle that case.
PS1, Line 298:
> we probably want a little bit of slop here just like we had with 'renew_dea
Oops, you're right. I added the renew_deadline back.
--
To view, visit http://gerrit.cloudera.org:8080/7770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 2
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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Adjust kerberos renewal logic to avoid tickets with NULL 'renew till' timestamp
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Adjust kerberos renewal logic to avoid tickets with NULL 'renew_till' timestamp
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7770/2//COMMIT_MSG
Commit Message:
PS2, Line 7: Adjust kerberos renewal logic to avoid tickets with NULL 'renew_till' timestamp
nit: could you shorten it to fit about 50 symbols, like
[security] avoid tickets with NULL 'renew_till'
?
http://gerrit.cloudera.org:8080/#/c/7770/2/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS2, Line 295: ticket_lifetime
> Yes, that's true, but how low of a ticket_lifetime do we want to adjust for
Maybe, then compute renew_deadline as
renew_deadline = renew_till - 30.0
?
PS2, Line 295: std::min(static_cast<double>(30), ticket_lifetime)
nit: consider using
std::min(30.0, ticket_lifetime)
or
std::min<double>(30, ticket_lifetime)
for brevity.
--
To view, visit http://gerrit.cloudera.org:8080/7770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] avoid tickets with NULL 'renew till'
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] avoid tickets with NULL 'renew_till'
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7770/2/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS2, Line 295:
> If we just do renew_deadline = (renew_till - 30), tickets with renew lifet
I might be missing some crucial point here, but if renew_deadline is negative, that's fine with the if (ticket_expiry < now || (now + ticket_lifetime) > renew_deadline) condition, right? So, a ticket with lifetime of less than 30 seconds would be re-acquired here instead of renewing. Would be that a problem?
--
To view, visit http://gerrit.cloudera.org:8080/7770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] avoid tickets with NULL 'renew till'
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/7770
to look at the new patch set (#3).
Change subject: [security] avoid tickets with NULL 'renew_till'
......................................................................
[security] avoid tickets with NULL 'renew_till'
It was found that if we use a file based credential cache that is
shared between the C++ side and the java side of a process, and we
encounter the specific edge case where we renew a ticket that has
less than 'ticket_lifetime' left before its 'renew_lifetime' expires,
the ticket is set to have a NULL 'renew_till' timestamp.
Eg:
ticket_lifetime = 10m
renew_lifetime = 100m
[current ticket being renewed] at '15:30:00'
endtime = '15:30:30'
renew_till = '15:31:00'
This ticket will be renewed and the renewed ticket will have the
following values:
endtime = '15:31:00'
renew_till = null
The Java krb5 library refuses to read these kinds of tickets which
have the RENEWABLE flag set but no 'renew_till' set, causing
unexpected failures. We work around this by reacquiring a new ticket
instead of renewing the existing ticket if there is less that
'ticket_lifetime' left between now and the 'renew_till' deadline.
Tracked on the Java side by:
http://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8186576
Change-Id: I59194af94838f680df4ce121a8dee526a876e369
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/security/init.cc
2 files changed, 12 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/7770/3
--
To view, visit http://gerrit.cloudera.org:8080/7770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] avoid tickets with NULL 'renew till'
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has abandoned this change.
Change subject: [security] avoid tickets with NULL 'renew_till'
......................................................................
Abandoned
Turns out that this patch makes the issue mentioned in the commit message less severe, but still hits it occasionally.
The reason is that if the Kudu side renew tickets, the Java side (from hadoop-common in my testing) could also have its own renewal thread working on the same credential cache. And even if we're careful with our renewals, the java side could renew outside the ideal window that we're trying to maintain.
The only other way to work around this is to always reacquire instead of renew, thereby not giving a chance for the Java side renewal thread to renew outside this ideal window, as our reacquisition of the ticket would always reset the window for the new ticket in the credential cache.
Also, we don't have anything to gain in terms of performance or security from renewing vs. reacquiring when we login from a keytab.
I will put out another patch which removes the renewal code and only always reacquires.
--
To view, visit http://gerrit.cloudera.org:8080/7770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] avoid tickets with NULL 'renew till'
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: [security] avoid tickets with NULL 'renew_till'
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7770/2/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS2, Line 295:
> I might be missing some crucial point here, but if renew_deadline is negati
You're right. That was my bad. In any case, I have to abandon this patch. After quite a lot of testing, it shows that this still doesn't work around the problem as I previously thought.
I've added a top level review comment as to why it doesn't work.
--
To view, visit http://gerrit.cloudera.org:8080/7770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] avoid tickets with NULL 'renew till'
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: [security] avoid tickets with NULL 'renew_till'
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7770/2//COMMIT_MSG
Commit Message:
PS2, Line 7: Adjust kerberos renewal logic to avoid tickets with NULL 'renew_till' timestamp
> nit: could you shorten it to fit about 50 symbols, like
Done
http://gerrit.cloudera.org:8080/#/c/7770/2/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS2, Line 295: std::min(static_cast<double>(30), ticket_lifetime)
> nit: consider using
Done
PS2, Line 295: ticket_lifetime
> Maybe, then compute renew_deadline as
If we just do renew_deadline = (renew_till - 30), tickets with renew lifetime less than 30s will cause the renew_deadline to have a negative value.
The idea is to add some slop for tickets that have renew lifetimes > 30s. For tickets that have < 30s renew lifetimes, we just end up renewing at the time we hit the deadline.
--
To view, visit http://gerrit.cloudera.org:8080/7770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Adjust kerberos renewal logic to avoid tickets with NULL 'renew till' timestamp
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: Adjust kerberos renewal logic to avoid tickets with NULL 'renew_till' timestamp
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7770/2/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS2, Line 295: ticket_lifetime
> don't you still need a little slop here in addition to ticket_lifetime?
Yes, that's true, but how low of a ticket_lifetime do we want to adjust for? Technically, the user could set a 1s ticket lifetime too, for which the above argument still holds and for which we really can't do anything.
--
To view, visit http://gerrit.cloudera.org:8080/7770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 2
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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes