-
Notifications
You must be signed in to change notification settings - Fork 172
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
Add support for tunnelto with config file importer and file-based provisioning #171
base: main
Are you sure you want to change the base?
Conversation
I thought about this a bit more. It makes sense to disable 1Password auth if the command contains
In either case, there's no requirement to use 1Password auth for |
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.
Looks good to me, thank you for this contribution! Only one question.
return schema.CredentialType{ | ||
Name: credname.APIKey, | ||
ManagementURL: sdk.URL("https://dashboard.tunnelto.dev/"), | ||
Fields: []schema.CredentialField{ |
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 provisioning of host, port, scheme and subdomain also available via envvars? If so, I'd propose we add these as well as optional fields.
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.
a few of them are: https://github.com/agrinman/tunnelto/blob/06428f13c638180dd349a4c42a17b569ab51a25f/tunnelto/src/config.rs#L6. This would be a nice improvement indeed.
plugins/tunnelto/api_key.go
Outdated
}, | ||
DefaultProvisioner: provision.TempFile( | ||
provision.FieldAsFile(fieldname.APIKey), | ||
provision.AtFixedPath("~/.tunnelto/key.token"), |
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.
Sadly they do not offer support for overriding the fixed path :(, so users of this plugin will have to move a potentially already existing file elsewhere. I opened an issue on their repo for this: agrinman/tunnelto#78
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.
lgtm. @hculea 's comment should be addressed before merging.
Thanks for reviewing, Andi and Horia! Just wanted to note that this PR is on my radar, but it may be another two or three days before I can get back. 🙏🏼 |
Hi @hculea and @AndyTitu, thank you for your patience! Do I understand correctly that a shell plugin can have only one provisioner? In our current format, we provision a TempFile but if we want to support envvar-based provisioning, I think I'd have to remove the TempFile provisioner. 🤔 I don't have much experience with rust code, but I am slowly exploring the tunnelto codebase (starting with the config.rs file) to understand how we can configure host, port, scheme and subdomain in the configuration file, if that's an option at all. |
Hey there! Indeed, plugins can have at most one provisioner. For now, we (mostly I 😂 ) have a preference towards envvar provisioners over file provisioners, because of some limitations that are inherent to the latter. Whenever both are an option, I advise to proceed with envvars. From the looks of it, this tool offers envvar support as well, so let's explore that! I'm not very familiar with Rust myself, but let us know if we can help in any way 😄 |
Definitely happy to explore envvar-based provisioning! But I haven't exactly located the actual envvar that we'd use for the API Key. Only the config directory and config file names are available as envvars: I have reached out to the author of tunnelto, @agrinman, on email for advise. I'll followup as soon as I know more. |
I took a quick look around, and based on this file: https://github.com/agrinman/tunnelto/blob/06428f13c638180dd349a4c42a17b569ab51a25f/tunnelto_server/src/config.rs it doesn't look like they currently support storing the API Key in an environment variable. I look forward to seeing what they come back to you with! |
What is blocking us here? Last time I checked it was about neither the file provisioning or env var provisioning working. There are some changes which need to be done in their Rust codebase for either to work right? |
@AndyTitu, the current implementation is using a file provisioner. We are trying to move to environment variables-based provisioner so that we can use host, port, scheme and subdomain as well. I talked to Alex (author of TunnelTo) and they pointed out that while they don't support envvars, they do accept a command like argument "--key". So, I started working on a arguments provisioner last night. It's a work in progress (pushing what I have working so far) but has a couple of things pending:
It may be a while before I wrap up all of this, so I'll be sure to update the PR status to "ready to review" when it's done! |
773868e
to
0a836fb
Compare
This PR adds support for tunnelto dev. Fixes #157.
Key points
tunnelto set-auth -k key
ortunnelto set-auth --key key
is the command to set the API Key for authentication.~/.tunnelto/key.token
path.Notes
Environment variable not supported
I am not seeing an environment variable to use for authentication. I reached out @agrinman on email to understand if it's possible. Until that's clear, I suggest we move forward with file-based provisioning.
1Password authentication needed for arg-less usage
If an user runs just
tunnelto
, 1Password authentication should be used because that command basically runstunnelto --port 8000
Not sure how to handle
set-auth
flowWhen someone sets up tunnelto after setting up the shell plugin, running
tunnelto set-auth -k key
ortunnelto set-auth --key key
results in the API Key being stored to~/.tunnelto/key.token
on the computer.And running tunnelto commands after that results in failure because 1Password tries to provision the credential to that path, but a file already exists at that path:
I have already skipped 1Password authentication for
-k
and--key
.