Skip to content

Conversation

bartonip
Copy link
Contributor

@bartonip bartonip commented Sep 4, 2023

This is a relatively in-progress implementation of a callback based token system for cli-auth.

Right now, we are finding it rather cumbersome to have to copy and paste the token into the CLI as we often already have something in the clipboard we want to keep.

I have implemented functionality that completes this token passing through HTTP. It works as follows:

  1. Coder asks the kernel for an available port and binds the port to the loopback address (127.0.0.1). This is because we are able to assume that 127.0.0.1 is safe and not susceptible to MITM attacks where localhost is.
  2. Coder starts an http.ListenAndServe session in a goroutine so that the main thread is not blocked.
  3. A callback query param is appended onto the authURL and the user's browser is opened.
  4. When the /cli-auth is loaded, a check has been added to see if a callback query param has been supplied, if so a single button interface requiring the user to press to authorize the callback is shown. I have included demonstrative callback_url validation in this page, we should be able to change this however is required.
  5. When the button is pressed, the browser navigates to the callback URL which would end up being something like http://127.0.0.1:42069/auth?token=nevergonnagiveyouup
  6. At which point, the token is sent back through a channel and is run using the same validation function that the original CLI input used. This will call client.SetSessionToken as per the original implementation.

Todo

  • Write tests. I will be working on this a little more this afternoon and adding some tests. If you are able to help me get started that would be great
  • Auto close the browser after the token is parsed by the cli tool. I will have some Javascript returned by the endpoint which will close the tab/window it is open in.

Security Concerns

In terms of security, I do not think this implementation is any less secure than what is used now. For the key to be leaked to an MITM attacker, they would either need scripted mouse control or be able to read the source of the returned page. Both of these attack vectors work on the existing implementation anyway, so the attack surface area there is no different. This would be different if the authorisation page automatically redirected to the callback URI but that is not the case.

Another attack vector could be a bogus callback url being replaced in the browser at some point. However I believe if this is able to happen, the user's machine is compromised anyway.

Also I have not been able to end-to-end test this as I cannot get the dogfood container to work- so any assistance there would also be eggsellent

@cdr-bot cdr-bot bot added the community Pull Requests and issues created by the community. label Sep 4, 2023
@bartonip bartonip changed the title Implement callback authorisation for cli tokens feat: Implement callback authorisation for cli tokens Sep 4, 2023
@github-actions
Copy link

github-actions bot commented Sep 4, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@bartonip
Copy link
Contributor Author

bartonip commented Sep 4, 2023

I have read the CLA Document and I hereby sign the CLA

cdrcommunity added a commit to coder/cla that referenced this pull request Sep 4, 2023
@matifali matifali changed the title feat: Implement callback authorisation for cli tokens feat: implement callback authorisation for cli tokens Sep 4, 2023
@phorcys420
Copy link
Member

phorcys420 commented Sep 5, 2023

For reference, this is very similar to discord's handoff process (opens up a web browser when your desktop client is logged out).


Right now, we are finding it rather cumbersome to have to copy and paste the token into the CLI as we often already have something in the clipboard we want to keep.

For this, I use the clipboard history (Win+V on Windows), but I do think this is a valid concern.


Auto close the browser after the token is parsed by the cli tool. I will have some Javascript returned by the endpoint which will close the tab/window it is open in.

Please do not return raw javascript code to be evaluated by the web page, this should be avoided at all costs and will probably not be allowed by the Content Security Policy anyways.
You should return a value to be interpreted by a script that's already loaded by the web page.


Regarding security concerns, I'm not too fond of this.
Even though the only way to exploit this is when the end user's machine is compromised, it is facilitated when such a login mechanism is in place.

While you can already extract the token out of a browser's localstorage via some program, these are harder to read and often protected by system keystores. (EDIT: This is wrong, I mismatched it with the password store)

You could also inject an extension into the browser, but that requires user consent.
The only currently viable attack vector would likely be clipboard sniffing or screen OCR.

I think this would make it easier for a theoretical attacker to implement an auth stealer.

@phorcys420
Copy link
Member

phorcys420 commented Sep 5, 2023

Also, please run make fmt and make lint in your branch (@bartonip)

@bartonip
Copy link
Contributor Author

bartonip commented Sep 5, 2023

While you can already extract the token out of a browser's localstorage via some program, these are harder to read and often protected by system keystores.

I think this would make it easier for a theoretical attacker to implement an auth stealer.

I don't think this is correct.

Right now, the cli key can just be read straight out of the DOM, the attack vector is exactly the same. The only case where there is an additional attack vector is if the callback URL provided is malicious, however this can easily be avoided with validation on the URL and the user not randomly clicking auth links from unknown sources.

@phorcys420
Copy link
Member

phorcys420 commented Sep 5, 2023

Right now, the cli key can just be read straight out of the DOM, the attack vector is exactly the same.

Which would require an extension to be forced into the browsers, all major browsers prevent this behavior and need the user to accept the activation of the extension.

this can easily be avoided with validation on the URL

What kind of validation are you talking about exactly ?

the user not randomly clicking auth links from unknown sources.

Relying on the end user for security is a pretty poor practice in my opinion, I think we should always assume that the end user will do all kinds of bad things, but I also think this type of flaw can be avoided by design.


I think I might come off as aggressive, that's not what I want, I just want to let you know that I'm reviewing every possibility.

@bartonip
Copy link
Contributor Author

bartonip commented Sep 5, 2023

Which would require an extension to be forced into the browsers, all major browsers prevent this behavior and need the user to accept the activation of the extension.

Can you propose an attack that only the proposed method is vulnerable to?

@phorcys420
Copy link
Member

Can you propose an attack that only the proposed method is vulnerable to?

Yes, the use of that login mechanism by an unauthorized client on the user's machine, which introduces less complexity when it comes to the attack mechanism.

But now that I think about it, it's fine if you make the user accept on the page, the issue applies more to discord where this process is practically unattended.

So therefore, not that big of a deal.


I think a good way to mitigate the callback URL issue might be to do it like npm does.

The advantage is also that you don't need to listen to a port on the local machine.

npm does something like this:

  1. initiate login flow
  2. open login page in browser
  3. poll every N seconds asking npm if the request is done

I think this can be applied with websockets to be more efficient.

What do you think?

@JackOfMostTrades
Copy link

FWIW using the localhost listener for the browser callback is the RFC-8252 standard way to do this sort of thing for OAuth2 clients. That RFC specifically applies to a client trying to get an Oauth2 (or OIDC) access token, but it really applies just as well to any context where you're bridging a browser login to a native client (like a CLI). When used with a PKCE challenge in the flow, it does address most of the plausibly addressable security considerations. But the security considerations section of has pretty good coverage of things to consider if you want to make sure your bases are covered.

@github-actions github-actions bot added the stale This issue is like stale bread. label Sep 16, 2023
@github-actions github-actions bot closed this Sep 19, 2023
@kylecarbs kylecarbs reopened this Oct 19, 2023
@ammario
Copy link
Member

ammario commented Oct 19, 2023

Great UX improvement! I think we should always prompt stdin for the token because the coder login is often occurring in a remote terminal.

An idea to consider that would work with remote terminals:

sequenceDiagram
    participant b as Authenticated Browser
    participant c as CLI
    participant s as coder server
   
    c->>s: Open websocket at `/api/cli-login`
    s->>c: Return login session ID
    c->>b: Open /cli-login?id=${session_id}
    b->>s: Authenticate ${session_id}
    s->>c: Return authenticated session token
Loading

@github-actions github-actions bot removed the stale This issue is like stale bread. label Oct 20, 2023
@bartonip
Copy link
Contributor Author

bartonip commented Oct 20, 2023

@ammario great idea! However I do think a websocket is overkill for this design.

How the AWS CLI solves this is it just polls an endpoint every second or so to see if the session has been authenticated.

The eventual workflow will look like this.

sequenceDiagram
    participant b as Authenticated Browser
    participant c as CLI
    participant s as coder server
   
    c->>s: Request session ID: POST /api/sessions
    s->>c: Return login session ID
    Note over s,c: Display /cli-login?id=${session_id} in CLI in case browser cannot be opened.
    c->>b: Open /cli-login?id=${session_id}
    par CLI polls Server
    loop Every n seconds
        c->>s: GET /api/sessions/${session_id}.
        alt not authenticated
            s->>c: {"authenticated": false}
        else is authenticated
            s->>c: {"token": "nevergonnagiveyouup"}
        end
    end
    and Browser authenticates with Server
    b->>s: Authenticate ${session_id}
    end
Loading

What do you think?

@ammario
Copy link
Member

ammario commented Oct 20, 2023

  1. we like to avoid polling early in a design to minimize HTTP log-spam and other per-HTTP overhead
  2. websockets/SSE is pretty easy for us as we already use it in many places throughout the codebase
  3. websocket/SSE simplifies login session cleanup as we know exactly when the client disappears and can defer delete(). Accordingly, it mitigates the DoS attack value of the endpoint.

@bartonip bartonip closed this Oct 21, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Pull Requests and issues created by the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants