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

fix(ngrok): Do not require 1Password authentication for certain (sub)commands #187

Conversation

arunsathiya
Copy link
Contributor

@arunsathiya arunsathiya commented Feb 16, 2023

Fixes #185

We currently require 1Password authentication for ngrok config's and ngrok update's subcommands, while it isn't necessary at all. Requiring authentication results in breakage for certain commands, as described here:

This PR fixes that issue by not requiring 1Password authentication for ngrok config, ngrok update and their subcommands.

@russorat
Copy link

other subcommands that do not require authtoken:

ngrok completion
ngrok credits
ngrok help
ngrok version

it's unclear how this would interact with ngrok service which will install ngrok as a background service

@arunsathiya
Copy link
Contributor Author

other subcommands that do not require authtoken:

Thanks Russ, I have updated this PR to reflect that behavior: 3c6fdb2

it's unclear how this would interact with ngrok service which will install ngrok as a background service

I have just had a look at the ngrok service docs (and tested), and it seems that the initial setup (ngrok service start --config path) will require knowing the config file's path in advance. That's not possible when 1Password wrapper/shell plugin is used. Even when this shell plugin is reworked to support environment variables-based provisioning, we cannot securely generate a config file and use its path. 1Password Shell Plugin's TempFile provisioner is not relevant is my understanding, because that's 1Password generating a secure config file at runtime and injecting that path into the command.

To sum things up, I am opting to not use 1Password shell plugin for ngrok service commands: de09071. The user will need to use a locally store config file for that command.

Open to others feedback/thoughts!

@arunsathiya arunsathiya changed the title Do not require 1Password authentication for config and update commands, and subcommands fix(ngrok): Do not require 1Password authentication for certain (sub)commands Feb 17, 2023
@SimonBarendse
Copy link
Member

SimonBarendse commented Feb 17, 2023

For ngrok config to be usable for the non-secret configuration, we also need to merge the on-disk configuration file with the in-memory configuration file we're provisioning, so that any changes you make to the on-disk file persist when the plugin is used for auth.

We could use ngrok's built-in merge functionality for this: https://ngrok.com/docs/ngrok-agent/config#how-config-files-get-merged.

@AndyTitu could you give this a look since you recently worked on a similar problem for another plugin?

@SimonBarendse
Copy link
Member

@russorat for ngrok service, does ngrok service start accept the NGROK_AUTHTOKEN environment variable? Or does that need to be in the configuration file that was used with ngrok service install?

@russorat
Copy link

@SimonBarendse from my testing, the ngrok service start command expects the authtoken to be included in the config file you passed via ngrok service install which requires a --config param.

@AndyTitu
Copy link
Contributor

AndyTitu commented Mar 6, 2023

Thanks @arunsathiya for starting this, I rebased off your branch and continued work for supporting --config in #194

@arunsathiya
Copy link
Contributor Author

I'll close this PR in favor of the ongoing work at #194.

@arunsathiya arunsathiya closed this Mar 7, 2023
@arunsathiya arunsathiya deleted the fix/auth-for-certain-ngrok-commands branch March 29, 2023 09:50
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.

1password wrapper breaks some of the ngrok cli commands that do not require authtoken
4 participants