-
Notifications
You must be signed in to change notification settings - Fork 264
[auth] Implement device flow auth #1114
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
Conversation
Questions on the UX and not the implementation:
|
|
), | ||
Domain: envir.GetValueOrDefault( | ||
"DEVBOX_AUTH_DOMAIN", | ||
"auth.jetpack.io", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean if we ship this cli version, we'd have to keep auth.jetpack.io
even if we choose a different domain later on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we ever change auth domains, we would need to maintain this one as long as users are on this version of the CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code won't follow redirects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
|
||
func getAuthFilePath() string { | ||
return xdg.StateSubpath(filepath.FromSlash("devbox/auth.json")) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this mostly taken from the jetpack
tool login code? what changes were made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this stuff is new (launchpad doesn't store auth in state XDG I don't think, it also has a custom struct instead of just storing the auth0 response). The device code polling is inspired by launchpad but heavily modified in order to be simpler and not require a lot of copy pasting or importing (i.e. protos from that repo)
func (u *user) String() string { | ||
return u.Email() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine for now, but eventually we'd need to move away from email to our own user-id as the true-identifier of a user (since a user may change their email)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is mostly for printing info, not meant to be a canonical ID or anything
I find myself feeling a little uneasy with this PR, because we are shipping it as part of a CLI binary release, it is hard to change once we shipped it (verses something like devbox.sh, where things are more flexible). Once we ship this, does that mean we are locked to use auth0 as the auth provider? Can we easily swap provider out if we want to? 🤔 In general, is there a way to make the CLI stay as generic as possible, so that any changes to auth scope, endpoint, provider, or identifier can be made without a release and/or stay backward compatible by default? Just thinking out loud here. |
Is this Devbox Cloud specific, or cloud agnostic? Edit, to clarify: is the idea I can, for example, push/pull config from a custom bucket my team owns, or just using Devbox Cloud? |
## Summary This is stacked on #1114 This implements cloud based global profiles using s3, our existing auth tokens, and identity federation. When user is logged in, we save an IDToken. Tokens are then used to assume an AWS role that allows the user to push and pull data to a path reserved to them. The two commands that do this right now are: ```bash devbox global push devbox global pull ``` (without a url/path) It also improves how we overwrite existing profiles by checking for a profile before downloading the new profile. ## How was it tested? ``` devbox global push devbox global pull ``` (inspected bucket). * Also tried pushing/pulling to path that user did not have access to and it failed. * If you push/pull without being logged in you get an error message.
@alvarogonzalez-packlink Great question. This feature has nothing to do with our "remote developer environments", it's a different cloud feature to upload and share devbox environments with others. To upload to a cloud store you need to be authenticated. The plan is to build it in a cloud agnostic way; similar to how docker can talk to many different container registries. We're iterating on the feature behind a feature flag, so that we can iron out how to improve it and continue to make it agnostic. Let me know if you have more thoughts or concerns: would love to hear them so we continue to improve the product! |
Sorry @loreto , I think I was filtering merged PRs and missed your question. I was checking this and #1128 , and being a bit confused by the "auth.jetpack.io" workflow, so that's why I asked if this was around Jetpack.io services only, or available for self-hosting. I haven't checked much the auth workflow, and I don't know much Go and my AWS is a bit rusty 🤣 so I don't know exactly who's doing the auth. By now (and I understand you're still testing) it feels like Jetpack is somehow giving permissions to users to certain buckets, and maybe through the web login process handing the client some authentication key, and then the client uses that key to download, directly from the AWS bucket, the profile. If I were doing simple self-hosting support, I would do a bunch of things:
So configuration would be a bit like when you define a state backend for Terraform :
You're using Terraform's managed service? You declare that in a file and then do terraform {
cloud {
organization = "example_corp"
## Required for Terraform Enterprise; Defaults to app.terraform.io for Terraform Cloud
hostname = "app.terraform.io"
workspaces {
tags = ["app"]
}
}
} You self-manage? You declare that bucket in a file, and then by yourself manage whatever permissions you need in that bucket: terraform {
backend "s3" {
bucket = "mybucket"
key = "path/to/my/key"
region = "us-east-1"
}
} |
Summary
This adds a new hidden top level command
devbox auth
. It adds a few other commands:This infrastructure will be used to power
devbox global push/pull
. It is customizable so that a user could specify own auth0 tenant.How was it tested?
Changed refresh token expiration to be 5 seconds so I could test out auto-refresh.