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

[LIBCLOUD-835] Fix caching of Google auth tokens #844

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
4 participants
@paultiplady
Contributor

paultiplady commented Jul 25, 2016

Fix corruption bug in Google auth token caching

Description

The GoogleOAuth2Credential. _write_token_to_file() method writes a copy of the latest OAuth token to disk. Prior to this fix, the token was being written to disk without truncating the file first, which is fine in the case where the new token has the same number of characters (or more) as the old one. However, in some situations Google OAuth returns a shorter token string, which was causing the library to crash when loading the corrupted token.

Status

Fixed, needs tests.

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)

_write_token_to_file was not zeroing the file before writing
a new token, causing corruption.

FIXES: LIBCLOUD-835

[LIBCLOUD-835] Fix caching of Google auth tokens
_write_token_to_file was not zeroing the file before writing
a new token, causing corruption.

FIXES: LIBCLOUD-835
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 25, 2016

Member

Thanks.

The change looks good to me. /cc @erjohnso

It's also worth noting that there is still a possibility of a race condition (short window) if multiple instances of script which uses the driver run on the same host. Sadly there is not much we can (and probably should) do.

One option would be to use file based locking, but that's not totally trivial doing it cross-platform if we want to avoid additional dependencies and it's something we should probably outsource to the user (there will always be some possible edge cases and race conditions and sadly we can't take care of all of it).

Member

Kami commented Jul 25, 2016

Thanks.

The change looks good to me. /cc @erjohnso

It's also worth noting that there is still a possibility of a race condition (short window) if multiple instances of script which uses the driver run on the same host. Sadly there is not much we can (and probably should) do.

One option would be to use file based locking, but that's not totally trivial doing it cross-platform if we want to avoid additional dependencies and it's something we should probably outsource to the user (there will always be some possible edge cases and race conditions and sadly we can't take care of all of it).

@erjohnso

This comment has been minimized.

Show comment
Hide comment
@erjohnso

erjohnso Jul 25, 2016

Member

/cc @supertom - thanks for this @paultiplady!

Member

erjohnso commented Jul 25, 2016

/cc @supertom - thanks for this @paultiplady!

@paultiplady

This comment has been minimized.

Show comment
Hide comment
@paultiplady

paultiplady Jul 25, 2016

Contributor

I think this code could also be made more robust; I believe we're just writing the token as an optimization to speed up the next startup, so failure here shouldn't really tank the app.

Could probably defensively catch all exceptions in the _get_token_from_file() method, instead of just IOErrors; that way if multiple writers clobber the file, the driver will fallback to fetching a new auth token from the API.

Contributor

paultiplady commented Jul 25, 2016

I think this code could also be made more robust; I believe we're just writing the token as an optimization to speed up the next startup, so failure here shouldn't really tank the app.

Could probably defensively catch all exceptions in the _get_token_from_file() method, instead of just IOErrors; that way if multiple writers clobber the file, the driver will fallback to fetching a new auth token from the API.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 25, 2016

Member

I think this code could also be made more robust; I believe we're just writing the token as an optimization to speed up the next startup, so failure here shouldn't really tank the app.

I agree.

It should just degrade and re-authenticate each time instead of exploding like it does right now.

If possible - it would also be great if you can add a test case for it :)

And again thanks and a good catch.

Member

Kami commented Jul 25, 2016

I think this code could also be made more robust; I believe we're just writing the token as an optimization to speed up the next startup, so failure here shouldn't really tank the app.

I agree.

It should just degrade and re-authenticate each time instead of exploding like it does right now.

If possible - it would also be great if you can add a test case for it :)

And again thanks and a good catch.

@supertom

This comment has been minimized.

Show comment
Hide comment
@supertom

supertom Jul 25, 2016

Contributor

LGTM. I agree, we should be more robust in this area. No reason why we can't be defensive here.

Thanks @paultiplady for the fix!

Contributor

supertom commented Jul 25, 2016

LGTM. I agree, we should be more robust in this area. No reason why we can't be defensive here.

Thanks @paultiplady for the fix!

@paultiplady

This comment has been minimized.

Show comment
Hide comment
@paultiplady

paultiplady Jul 25, 2016

Contributor

I've started looking at tests, but I'm afraid I probably don't have time to
finish today. Not sure how you're prioritizing this, but if the merge can
wait a day or two then I should be able to come up with a UT in that
timeframe.

On Mon, Jul 25, 2016 at 11:31 AM, Tom Melendez notifications@github.com
wrote:

LGTM. I agree, we should be more robust in this area. No reason why we
can't be defensive here.

Thanks @paultiplady https://github.com/paultiplady for the fix!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#844 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ARTsikWGUtM8Xg8-q2DEumeLlVhMo_nQks5qZQEZgaJpZM4JUW3o
.

Contributor

paultiplady commented Jul 25, 2016

I've started looking at tests, but I'm afraid I probably don't have time to
finish today. Not sure how you're prioritizing this, but if the merge can
wait a day or two then I should be able to come up with a UT in that
timeframe.

On Mon, Jul 25, 2016 at 11:31 AM, Tom Melendez notifications@github.com
wrote:

LGTM. I agree, we should be more robust in this area. No reason why we
can't be defensive here.

Thanks @paultiplady https://github.com/paultiplady for the fix!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#844 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ARTsikWGUtM8Xg8-q2DEumeLlVhMo_nQks5qZQEZgaJpZM4JUW3o
.

@asfgit asfgit closed this in 78df34c Jul 30, 2016

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 30, 2016

Member

I've merged PR into trunk and also made changes so failure on writing and reading token to / from file is not fatal - 2f02aec, 25c414e.

I also added logging statement on failure, but we don't really have any common logging standards and practices established in Libcloud yet so that's something we need to improve in the future (among other things, we need to make sure that when LIBCLOUD_DEBUG is set that those messages are written to sys.stderr, etc.).

Thanks again!

Member

Kami commented Jul 30, 2016

I've merged PR into trunk and also made changes so failure on writing and reading token to / from file is not fatal - 2f02aec, 25c414e.

I also added logging statement on failure, but we don't really have any common logging standards and practices established in Libcloud yet so that's something we need to improve in the future (among other things, we need to make sure that when LIBCLOUD_DEBUG is set that those messages are written to sys.stderr, etc.).

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment