Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add customizable header command#119

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
code-asher merged 8 commits intomainfromasher/credentials-process
Aug 18, 2023
Merged

Conversation

code-asher
Copy link
Member

@code-ashercode-asher commentedAug 4, 2023
edited
Loading

The command will be executed before requests and before configuring SSH to set custom headers.

Some notes:

  1. When I use--header I seem to get the value set twice. For example if I usecoder --header X-MY-CUSTOM-HEADER=hello-there then on the server end I seex-my-custom-header: hello-there, hello-there so we should look into that.
  2. I went with environment variables to pass data to the credentials process for now since I figured that would be easier than parsing command-line arguments but opinions are welcome here. OnlyCODER_URL right now. Are there others needed?
  3. More importantly, I am not sure about the config SSH portion. Since configuration might only happen on the first connect, the headers we hardcode into the SSH config could expire.

I have two main ideas for fixing the SSH config issue:

  1. Modify the CLI to call out to the same process to get the headers.--credentials-process or something like that. The CLI executes the process just as the extension does and sets the headers. We set--credentials-process in the SSH config on the plugin side and are off to the races.
  2. Add a second VS Code setting for a proxy command wrapper which gets passed the path to the Coder binary and is responsible for adding the appropriate--header flags and passing through any other flags. This way this credential process is called every time SSH is invoked. Or instead of a separate setting it could be an environment variable/flag and we reuse the same process; it can switch behavior depending on what task it needs to do (output JSON or execute the Coder binary).

An example of the second option:ProxyCommand my-wrapper --wrapper-flags -- --coder-flags wheremy-wrapper is essentiallyexec $CODER_BINARY --header my-custom-header "$@" except everything after-- should get passed through to the Coder binary and the rest can be flags to the wrapper itself in case we need them.

Thoughts? I am inclined to go the first route since the second requires more complexity on the user side but the second does at least have the advantage (or danger) of allowing more control over how the Coder binary executes.

I also considered substitution in the proxy command (something similar tocoder $(/path/to/credentials-process) ssh dev.dev --stdio) but I am not sure that is portable.

Sorted the above,--header-command has been added to the CLI although we will need the next release before this can be used in the plugin.

Closes#76 and supersedes#81.

ghuntley reacted with eyes emoji
@code-ashercode-asherforce-pushed theasher/credentials-process branch 3 times, most recently from8b4e951 to0423107CompareAugust 4, 2023 22:22
@kylecarbs
Copy link
Member

Option 1 seems correct to me as well.

For the naming, should we call thisproxy-credential-process or something else instead?credential-process sounds idiomatic, but could easily be confused for Coder credentials instead of middleware credentials.

@code-asher
Copy link
MemberAuthor

Ooo yeah good point, technically could you actually use this for Coder credentials by supplying the token header? But we might not want to encourage that type of use.

I thinkproxy-credential-process makes it sound like it only works for authenticating to proxies and although VPN is pretty much a proxy I wonder if folks will seeproxy and only think about reverse proxies. Or there could be a case where no proxy is involved but they have some required header.

Maybe since we have--header we can base it off that and have something like--header-process? It could be unfortunate if we want to support more than headers in the future but I am not sure what else we might want and if something else does come up we might want it to be a separate process in that case anyway.

@code-asher
Copy link
MemberAuthor

Or--config-process if we think we might need to be more generic.

@liorb-canva
Copy link

@code-asher option 1 seems fine as well for us

code-asher reacted with rocket emoji

@code-ashercode-asherforce-pushed theasher/credentials-process branch from0423107 to534be09CompareAugust 14, 2023 21:17
@code-ashercode-asher changed the titleAdd customizable credentials processAdd customizable header commandAug 14, 2023
This was causing issues with the header flag as those contain equalsigns.
@code-ashercode-asherforce-pushed theasher/credentials-process branch 6 times, most recently from36c842b to2625f25CompareAugust 14, 2023 23:37
@code-asher
Copy link
MemberAuthor

I am not sure if we need to check the Coder version before setting the new flag but since it only sets if you provide the value maybe it is OK to just rely on the user only adding it if their version supports the new flag?

This will be called before requests and added to the SSH config.
@code-ashercode-asherforce-pushed theasher/credentials-process branch from2625f25 to811673fCompareAugust 14, 2023 23:40
I got this once, not sure how to reproduce it now though.
Forgot escape adds its own wrapping quotes.
...config,
headers: {
...(await storage.getHeaders()),
...creds.headers,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

hey@code-asher this does not compile..
I've tried to run it locally and it failed

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah just realized I messed this up, fixing now.

liorb-canva reacted with heart emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Pushed a fix!

Copy link
MemberAuthor

@code-ashercode-asherAug 18, 2023
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Turns out I had another issue where the URL in storage was not set yet (for the login request) resulting in no headers. I think everything is working now, will test a bit more then merge this in. Let me know if you find anything else wrong!

First, I replaced the wrong line, needed to replace the second one.Secondly, seems you cannot actually spread like this becauseconfig.headers actually has a bunch of functions on it.
To avoid an error like "127.0.0.1:80 ECONNREFUSED".  Before we did notlog the error so this did not matter so much but now we do to catchheader issues.
At this stage the URL may not be set.  Or it could be set to theprevious URL.  We need to use the URL of the actual request.
@code-ashercode-asherforce-pushed theasher/credentials-process branch from2bb02f9 toa7f21beCompareAugust 18, 2023 06:58
@code-ashercode-asher merged commit3ab3aad intomainAug 18, 2023
@code-ashercode-asher deleted the asher/credentials-process branchAugust 18, 2023 07:03
@bpmctbpmct mentioned this pull requestAug 18, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@liorb-canvaliorb-canvaliorb-canva left review comments

@kylecarbskylecarbskylecarbs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pass headers to coder api
3 participants
@code-asher@kylecarbs@liorb-canva

[8]ページ先頭

©2009-2025 Movatter.jp