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

Adding support for auth_log iterator #156

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

csanders-git
Copy link

@csanders-git csanders-git commented Jan 4, 2022

auth_log can take a number of complex parameters that also have very specific API limits. This PR uses the documentation to better emulate authentication log collection.

This addresses issues outlined in #153 #154 and #155. It also correctly identifies the max GET request length of the Duo servers.

@csanders-git
Copy link
Author

csanders-git commented Jan 20, 2022

The most recent change above moved the rate limited checking from _connect to api_call. The reason this is needed is because the rate limit lockout on certain API calls (auth_logs) can be variable, i've seen as much as 240 seconds needed between requests. This is larger than the +/- 1 minute API timestamp skew allowed (the difference in times here 1 minute to > 1 minute is not a great design BTW), so each subsequent retry request needs to be resigned.

The code is a tad uglier as a result, but what can you do.

@AaronAtDuo
Copy link
Contributor

Hi @csanders-git,

Thanks for the PR and we apologize for taking so long to get to it.

We have a few concerns about the PR as it stands:

  • The biggest concern is that it appears to be changing the signature of the get_authentication_log in a backwards-incompatible way. Is there any way to accomplish what you want while retaining backwards compatibility? If not, we can figure out how we want to address the incompatibilities.
  • It looks like you're trying to solve multiple problems at once, and it's making the changes a bit hard to follow.
  • We'd would love to see some test cases added. Changes of this magnitude are scary without some testing.

In an ideal world, we'd like to see the three issues you mention (153, 154, and 155) solved in separate PRs, with new tests added in each, if at all possible.

Also, since you submitted the PR, there's been some changes to fix the sig v4 support and remove the sig v3 support, so I fear you'll have some merge conflicts to resolve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants