duo_client_python icon indicating copy to clipboard operation
duo_client_python copied to clipboard

Adding support for auth_log iterator

Open csanders-git opened this issue 3 years ago • 2 comments
trafficstars

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 avatar Jan 04 '22 06:01 csanders-git

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.

csanders-git avatar Jan 20 '22 05:01 csanders-git

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.

AaronAtDuo avatar Apr 26 '22 21:04 AaronAtDuo