-
Notifications
You must be signed in to change notification settings - Fork 31
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
Fix authenticated flag in a transfer adapter response #159
Conversation
The Identity.id field is used in the JWTAuthenticator._generate_token_for_action as one of the fields in the preauth JWT. The human-readable GitHub login is better suitable for this place than an opaque number internally used by GitHub (which they also happen to call an 'id'). Additionally, add some comments to the code I managed to forget how it works :)
The LFS batch response always included authenticated = true, even when the default "PRE_AUTHORIZED_ACTION_PROVIDER" was disabled (= null). This led the git client to forever retry the download/upload attempts without realizing it should try the credentials from its creds store. This comes in handy with the GitHub auth provider which can handle both the usual authentication and the same for the streaming transfer adapter.
The only package that's being packaged here is 'giftless', any other "rogue" directory confuses the packager.
Finally fixed all lints and tests. Also here's a little story on how I got here. In my setup, I'm using a storage backend on a private network (while the API is public), so I naturally ended up using the basic streaming transfer adapter. With this, the default JWT After some time trying to figure out what's wrong, I found there's a handful of successfully downloaded objects, but then just an endless string of 401s. All due to the JWTs being expired. The lfs client obviously has a very stupid retry logic that - without any backoff - retries any 401 immediately and forever. The only thing that made it stop retrying a particular file was ironically a 503 (server temporarily unavailable, usually to be retried) from the poor proxy I have on the front... But let's focus now on why it had to retry in the first place. The default lifetime for those JWTs is 60s, but it's obviously possible that the downloading won't start at once for all the files due to some sane connection limits in the client. Therefore I'd expect the JWT's lifetime would span at least the lifetime of the Looking at the decoded JWTs eventually made me do this mostly cosmetic 40eee64. Finally understanding what's most of this preauth logic about (it took a while 🙄), I realized I could use my github auth provider to serve the same purpose (the streaming storage API URL neatly overlaps with the batch API URL, so the auth provider still know which org/repo it is) without dragging in the the JWT preauth logic. That's what I tried by disabling the The last change 5575147 just tells setuptools to package only the |
I dunno if I should tag you, dear @athornton (whoops, I did it again 😇), as you previously said you're not really part of the Datopian pack (still you were the most fruitful partner in getting things merged). |
Sorry! Way behind on my email. I'll take a look at this stuff in a bit. |
Packaging just giftless is fine, and, yeah, I got really lost trying to go through the preauth stuff too several months back. This looks OK to merge to me, but I want to build an image from it and run that in testing for a little while to make sure that my sort-of-different-s3-via-Google-Workload-Identity doesn't break under it. From what I'm reading it shouldn't, but I should make sure, especially since I have a tough time following the thread through all the abstraction layers. |
The failure was when I restarted it with the new image. I'm going to merge, and then I'll get the dependabot PRs. |
sweet! Thank you! I'm polishing some further patches for github (including docs), so stay tuned :) |
The
authenticated
flag in the/batch
endpoint response is alwaystrue
, which surely is not the case when thePRE_AUTHORIZED_ACTION_PROVIDER
isnull
in the startup configuration.This prevented me from using the github auth provider, because with
authenticated: true
the git client doesn't try the stored credentials previously used to authenticate against thebatch
endpoint.There's couple loosely related tweaks included, see the details in the particular commits.