Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rgw: add time skew check in function parse_v4_auth_header #19476

Merged
merged 1 commit into from Jan 18, 2018

Conversation

Projects
None yet
7 participants
@qrGitHub
Copy link

commented Dec 13, 2017

  • In auth v4 http header request, RGW doesn't check time skew, while AWS
  • does.

Fixes: http://tracker.ceph.com/issues/22418
Signed-off-by: Bingyin Zhang zhangbingyin@cloudin.cn

@@ -408,6 +408,19 @@ static inline int parse_v4_auth_header(const req_info& info, /* in
}
date = d;

uint64_t req_sec = (uint64_t)internal_timegm(&t);

This comment has been minimized.

Copy link
@mattbenjamin

mattbenjamin Dec 13, 2017

Contributor

I think we should be using new ceph::time primitives here; @adamemerson?

This comment has been minimized.

Copy link
@adamemerson

adamemerson Dec 19, 2017

Contributor

Yes, we should.

This comment has been minimized.

Copy link
@qrGitHub

qrGitHub Dec 14, 2017

Author

hi @mattbenjamin, thanks for your comments.
I copied "internal_timegm" from function parse_v4_query_string. Would you tell me what "new ceph::time" is?

This comment has been minimized.

Copy link
@cbodley

cbodley Dec 19, 2017

Contributor

i think the internal_timegm() bit is fine. there is a common/ceph_time.h header that's based on the std::chrono library that could be used instead of the ceph_clock_now() part. @adamemerson, do you see any advantages to using that here?

This comment has been minimized.

Copy link
@mattbenjamin

mattbenjamin Dec 19, 2017

Contributor

@cbodley I'd prefer we use that going forward.

This comment has been minimized.

Copy link
@qrGitHub

qrGitHub Dec 20, 2017

Author

Hi @adamemerson @mattbenjamin @cbodley, do you mean change ceph_clock_now to ceph::real_clock::now?

@qrGitHub

This comment has been minimized.

Copy link
Author

commented Dec 16, 2017

hi @rzarzynski, @cbodley would you help to review this? Thanks.

@rzarzynski rzarzynski self-requested a review Dec 17, 2017

@adamemerson
Copy link
Contributor

left a comment

@cbodley All new code should use the new ceph_time.h stuff. Trying to migrate to it entirely is one of our Trello cards, I believe.

@@ -408,6 +408,19 @@ static inline int parse_v4_auth_header(const req_info& info, /* in
}
date = d;

uint64_t req_sec = (uint64_t)internal_timegm(&t);

This comment has been minimized.

Copy link
@adamemerson

adamemerson Dec 19, 2017

Contributor

Yes, we should.

@adamemerson

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2017

As opposed to utime_t/ceph_clock_now()

@cbodley

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

@cbodley All new code should use the new ceph_time.h stuff. Trying to migrate to it entirely is one of our Trello cards, I believe.

okay, no arguments there. just note that this block was copied from the v2 implementation, so we should probably move this logic into a helper function and fix both while we're at it. would you be willing to share some example code for @qrGitHub? since the resolution is currently in seconds, is ceph::coarse_real_clock the one you'd recommend?

@qrGitHub

This comment has been minimized.

Copy link
Author

commented Dec 23, 2017

Hi @adamemerson, would you be willing to share some example code? Thanks.

@adamemerson

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2018

Yes! Coarse real clock should be the one to use.

@adamemerson

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2018

As to timegm or internal_timegm, just use coarse_real_clock::from_time_t() to convert it to time point.

@qrGitHub qrGitHub force-pushed the qrGitHub:wip-rgw-lack-timeSkewCheck branch from 0707006 to 4a819ce Jan 8, 2018

@qrGitHub

This comment has been minimized.

Copy link
Author

commented Jan 8, 2018

Hi @cbodley @adamemerson would you help to review this? Thanks.

auto req_tp = ceph::coarse_real_clock::from_time_t(internal_timegm(&t));
auto cur_tp = ceph::coarse_real_clock::now();
auto skew = std::chrono::duration_cast<std::chrono::seconds>(cur_tp - req_tp).count();
if (skew < -60*RGW_AUTH_GRACE_MINS || skew > 60*RGW_AUTH_GRACE_MINS) {

This comment has been minimized.

Copy link
@cbodley

cbodley Jan 9, 2018

Contributor

there's a std::chrono::abs() in c++17 that can simplify this a bit:

  constexpr auto grace = std::chrono::minutes{RGW_AUTH_GRACE_MINS};
  if (std::chrono::abs(cur_tp - req_tp) > grace) {
if (skew < -60*RGW_AUTH_GRACE_MINS || skew > 60*RGW_AUTH_GRACE_MINS) {
dout(10) << "NOTICE: request time skew too big." << dendl;
dout(10) << "req_tp=" << ceph::coarse_real_clock::to_time_t(req_tp) <<
", cur_tp=" << ceph::coarse_real_clock::to_time_t(cur_tp) << dendl;

This comment has been minimized.

Copy link
@cbodley

cbodley Jan 9, 2018

Contributor

these duration types can be output directly with:

    using ceph::operator<<;
    dout(10) << "req_tp=" << req_tp << ", cur_tp=" << cur_tp << dendl;
Bingyin Zhang
rgw: add time skew check in function parse_v4_auth_header
* In auth v4 http header request, RGW doesn't check time skew, while AWS
* does.

Fixes: http://tracker.ceph.com/issues/22418
Signed-off-by: Bingyin Zhang <zhangbingyin@cloudin.cn>

@qrGitHub qrGitHub force-pushed the qrGitHub:wip-rgw-lack-timeSkewCheck branch from 4a819ce to b4e2f32 Jan 10, 2018

@qrGitHub

This comment has been minimized.

Copy link
Author

commented Jan 10, 2018

Hi @cbodley, would you help to review this?
Do you know how to enable C++17 support in Ubuntu 16.04? I didn't test "std::chrono::abs" because my build machine cannot support C++17 yet.

@mattbenjamin

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2018

@qrGitHub @tchaikov certainly does; I'm pretty sure we're sourcing gcc-7and/or clang from some repo in our builders to accomplish that, e.g., https://askubuntu.com/questions/859256/how-to-install-gcc-7-or-clang-4-0

@tchaikov

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2018

@qrGitHub please see https://github.com/ceph/ceph#build-prerequisites . install-deps.sh will install gcc-7 on your system.

@qrGitHub

This comment has been minimized.

Copy link
Author

commented Jan 10, 2018

Thanks @mattbenjamin @tchaikov, after install-deps.sh, my machine can build the latest code now.

@cbodley cbodley added the needs-qa label Jan 10, 2018

@qrGitHub

This comment has been minimized.

Copy link
Author

commented Jan 15, 2018

Hi @cbodley, can this be merged? Thanks.

@yuriw

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2018

@yuriw yuriw merged commit c329791 into ceph:master Jan 18, 2018

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.