gptel-gh: Change how tokens are managed

[marcus-lundgren]

Removed the file based persistent storage of the tokens, as they were stored in clear text. They will only be stored in variables.

The initial value of the github-token can now be set by providing a function that provides it. This enables this token to be securely stored outside of Emacs.

[marcus-lundgren]

@karthink - Do you have any input in how you imagine the workflow to be for this API regarding the GitHub token?

With the changes made just recently to present the login function, is it reasonable to have a workflow to perform that in order to get the token that can then be stored elsewhere?

[karthink]

@karthink - Do you have any input in how you imagine the workflow to be for this API regarding the GitHub token? With the changes made just recently to present the login function, is it reasonable to have a workflow to perform that in order to get the token that can then be stored elsewhere?

Sorry, I haven't looked at this PR yet. I also don't use Copilot so let's hear from @kiennq (the author of gptel-gh) on the proposed changes.

[marcus-lundgren]

@kiennq - A friendly reminder about this PR :)

[kiennq]

Sorry for the late reply. Can you add a default/example for github-token-init? At least it should be defaulted to the current workflow where the token is saved/restored from a file, else we're looking at reauth everytime gptel is reloaded

[marcus-lundgren]

Thanks for your reply, @kiennq!

This is what I am currently using:

(use-package gptel
  :defer t
  :config (setq
           gptel-backend (gptel-make-gh-copilot "Copilot"
                           :github-token-init (lambda () (read-passwd "Copilot token: "))))
  )

I store my token in a password manager, so I use read-passwd in order to enter it when calling gptel. I will only be prompted once per Emacs session for this, which is a very reasonable workflow for me as I rarely restart Emacs. As it takes just a function, it is trivial to make it read from a file instead.

With the current implementation in this PR, the login function is not able to persistently store the newly acquired token. The new token is only stored in a variable and I take the it from the message log, which is not an ideal scenario and hence my request for feedback earlier. Another approach could be to introduce load and save functions that can be given as parameters to gptel-make-gh-copilot. It would then allow the user to choose how it should be stored and saved.

Before making the changes in gptel itself, I used the code below as a patch to control how the token was stored. This also has the behavior of only requesting the token once. As you may notice, the logic for the chat message token was kept, but this was before me realising that that is also a thing to keep secret.

(use-package gptel
  :config
  (progn
    (defvar my/gh-token nil "Store the Copilot token for GPTel commands.")
    (defalias 'original-gptel--gh-save (symbol-function 'gptel--gh-save))
    (defalias 'original-gptel--gh-restore (symbol-function 'gptel--gh-restore))
    (defun gptel--gh-restore (file)
      (if (equal file gptel-gh-github-token-file)
          (if my/gh-token
              my/gh-token
            (progn 
              (setq my/gh-token
                    (let ((token (read-passwd "Copilot token: ")))
                      (if (string= "" token) nil token)))
              my/gh-token))
        (original-gptel--gh-restore file)))
    (defun gptel--gh-save (file obj)
      (message "New GH token: %s" obj)
      (if (equal file gptel-gh-github-token-file)
          (setq my/gh-token obj)
        (original-gptel--gh-save file obj)))))

[marcus-lundgren]

@kiennq - Any thoughts? :)

[marcus-lundgren]

@kiennq - I've now changed the implementation so that it defaults to use the file as before for the github-token.

What has been additionally added is the ability to create a custom save function for the github-token. This solves the question that I raised before, as it is now up to the one redefining the save function to decide how to handle an update.

A sample use-package snippet to override it:

(use-package gptel
  :defer t
  :config
  (defvar my/gh-token nil "Store the Copilot token for GPTel commands.")
  (setq
   gptel-gh-github-token-load (lambda ()
                                (if my/gh-token
                                    my/gh-token
                                  (progn 
                                    (setq my/gh-token
                                          (let ((token (read-passwd "Copilot token: ")))
                                            (if (string= "" token) nil token)))
                                    my/gh-token)))
   gptel-gh-github-token-save (lambda (token)
                                (setq my/gh-token token))
   gptel-backend (gptel-make-gh-copilot "Copilot"))
  )

[marcus-lundgren]

@kiennq - a friendly reminder concerning this. I think the current solution in this MR strikes a good balance between ease of use and choice of security. If you agree, then I can start working on changes to the README.

@karthink - do you have any thoughts regarding the latest proposal?

[marcus-lundgren]

@kiennq, @karthink - another reminder.

[karthink]

@marcus-lundgren just dropping a note to say this PR remains on my radar -- I just need a longer chunk of time to review it than other PRs since I'm not the author of the token management code. I'll do a general code review but I trust @kiennq's judgment on the design.

[marcus-lundgren]

@karthink - thank you for the status update! As you may have noticed, I am rebasing the branch continuously to fix any conflicts (as happened a few days ago).

[marcus-lundgren]

Updated config example with the builtin cache functionality:

(use-package gptel
  :defer t
  :config
  (setq
   gptel-gh-github-token-load-function (lambda () (read-passwd "Copilot token: "))
   gptel-gh-github-token-save-function (lambda (token) (message "Token saved: %s" token))
   gptel-backend (gptel-make-gh-copilot "Copilot"))
  )

[kiennq]

I left one comment, else it LGTM

[marcus-lundgren]

@kiennq, @karthink - I resolved the review comments you had after fixing them. If the workflow is that the reviewer should do that, then please let me know.

I've now also updated the README file with information about the customizable variables.

Some suitable information for the NEWS file can be found below. Do you want me to add it?

• Breaking change: Removed the customizable variable gptel-gh-token-file, as the chat token is no longer persistently stored.
• Added the customizable variables gptel-gh-github-token-load-function and gptel-gh-github-token-save-function. This enables the GitHub OAuth token to be loaded from and saved to arbitrary places.

[karthink]

If I understand the changes correctly, you can no longer have more than one Github copilot backend defined, since the authentication is now shared across all gh backends? Is this correct?

[marcus-lundgren]

@karthink - Yes, you are correct, but I would argue that while it was technically true before, it wasn't properly implemented as such. Simply due to the fact that one could only persistently store one token and there was no official way to set the GitHub token manually.

Calling the login function will overwrite the token stored in the file:

https://github.com/karthink/gptel/blob/master/gptel-gh.el#L267

It would also fall back to the persistently stored token on failure to renew the chat token:

https://github.com/karthink/gptel/blob/master/gptel-gh.el#L287 followed by
https://github.com/karthink/gptel/blob/master/gptel-gh.el#L298

With the changes I've made as a basis, one could:

• Reintroduce the tokens to the backend to be used as a cache variable
• Add the load and save functions as parameters to the gptel-make-gh-copilot function and add them as references to the backend. This would enable several backends to use the same overloaded functions.

But I think that might be suitable for a follow up PR to make it proper.

Any thoughts, @kiennq? Do you agree with my understanding of the logic and that it is suitable for another PR?

[kiennq]

Any thoughts, @kiennq? Do you agree with my understanding of the logic and that it is suitable for another PR?

Although we only store one token to file, we can have multiple backends and each backend can ask for a github token by using the login function. So, we can still use multiple github accounts/backends.

[kiennq]

Can you keep this function the same signature as before, i.e., taking a file param.
I'm working on a change to automatically fetch the model list and will use this to store the cached model list as well

[marcus-lundgren]

I've added the file param to the function again. Do you need a similar change to the equivalent save function?

[kiennq]

Yes. Can we remove the -from-file suffix as well? It takes a parameter of file so it's quite obvious that this is restoring from file

[marcus-lundgren]

Any thoughts, @kiennq? Do you agree with my understanding of the logic and that it is suitable for another PR?

Although we only store one token to file, we can have multiple backends and each backend can ask for a github token by using the login function. So, we can still use multiple github accounts/backends.

As @karthink identified in his comment, that will no longer work in the current implementation in this PR.

As the GitHub account used for a backend can change behind the scenes as described earlier, I would like to suggest adding proper support for token handling. I am thinking something along the lines of adding a parameter to the gptel-make-gh-copilot function which contains the desired GitHub username. That would allow the user to explicitly state which account to use, which in turn makes it possible to store the token in account specific files (e.g. github_token_karthink). Then we will avoid the following scenario that I fall into:

1. Have one backend for a personal account using free tier and one backend for a work account using a paid subscription
2. The currently stored token in the file is for the personal account as that was logged in last
3. We receive an error in the following logic, triggering the work account to read and make use of the personal account's token from the file:

It would also fall back to the persistently stored token on failure to renew the chat token:

https://github.com/karthink/gptel/blob/master/gptel-gh.el#L287 followed by
https://github.com/karthink/gptel/blob/master/gptel-gh.el#L298

4. The backend meant for the work account will now burn through the free tier's limited usage limit.

gptel-gh-github-token-load-function and gptel-gh-github-token-save-function should then take the GitHub username as a parameter, so that the implementation can deduce how to save/load the account specific token. With this, it would probably be best to replace the gptel-gh-github-token-file custom variable with a variable that points to a directory, as the filename will have the username in it. What do you think, @kiennq?

[kiennq]

How about making the gptel-gh-github-token-save/load-function as part of the gptel--gh backend? Same with the chat token cache. That would allow the user to be able to create multiple backends that target different github copilot backends.