zyedidia / eget

Easily install prebuilt binaries from GitHub.
MIT License
866 stars 39 forks source link

Allow to read GitHub token from a file #82

Closed hhromic closed 10 months ago

hhromic commented 10 months ago

First of all, thanks for this tool! I have been planning to implement a similar tool for a while until I found eget which is exactly what I was looking for and more.

I have one feature request (that I also offer to implement myself if approved): the ability to read a GitHub token from a file. The use case is the storage of the token as a secret in a protected file. And also, to avoid exposing the token into the process environment.

My proposal is to recognize the @/path/to/file syntax when reading the GitHub token value, and in such case read the contents of the file pointed by /path/to/file to be used as the final value. For example:

GITHUB_TOKEN=@/path/to/token
EGET_GITHUB_TOKEN=@/path/to/token

I propose this syntax because it is extremely unlikely for a GitHub token to start with a @ character.

I believe the simplest manner to implement this feature would be to add the following logic in SetAuthHeader().

Something like the following:

func SetAuthHeader(req *http.Request) *http.Request {
    hasGithubTokenEnv := os.Getenv("GITHUB_TOKEN") != ""
    hasEgetGithubTokenEnv := os.Getenv("EGET_GITHUB_TOKEN") != ""
    hasTokenEnvVar := hasGithubTokenEnv || hasEgetGithubTokenEnv

    githubEnvToken := ""

    if hasEgetGithubTokenEnv && githubEnvToken == "" {
        githubEnvToken = os.Getenv("EGET_GITHUB_TOKEN")
    }

    if hasGithubTokenEnv && githubEnvToken == "" {
        githubEnvToken = os.Getenv("GITHUB_TOKEN")
    }

    // start of new logic for reading the token from a file if provided
    if hasGithubTokenEnv && strings.HasPrefix(githubEnvToken, "@") {
        if len(githubEnvToken) < 2) {
            fmt.Fprintln(os.Stderr, "error: cannot read GitHub token from file: no file specified")
            os.Exit(1)
        }
        githubTokenFile := githubEnvToken[1:]
        githubEnvToken, err := os.ReadFile(githubTokenFile)
        if err != nil {
            fmt.Fprintln(os.Stderr, "error: cannot read GitHub token from specified file: %s", githubTokenFile)
            os.Exit(1)
        }
    }
    // end of new logic for reading the token from a file if provided

    if req.URL.Scheme == "https" && req.Host == "api.github.com" && hasTokenEnvVar {
        if opts.DisableSSL {
            fmt.Fprintln(os.Stderr, "error: cannot use GitHub token if SSL verification is disabled")
            os.Exit(1)
        }
        req.Header.Set("Authorization", fmt.Sprintf("token %s", githubEnvToken))
    }

    return req
}

Let me know if you agree with this feature request and if you want me to implement it like proposed.

EDIT: Oh, I just realized that SetAuthHeader() is likely called multiple times in a single command run. Therefore reading the file every time turns quite inefficient. In this case I would propose to implement passing the GitHub token as a request context variable and initialize it only once earlier in the application.

EDIT2: After a closer look to the codebase, it looks like SetAuthHeader() is called from Get(), which is called from Download(), or GetRateLimit(), and these chains only are called once per application run form main(). Therefore my previous proposal should not re-read the token from disk multiple times. Nonetheless, I would still refactor SetAuthHeader() a bit so reading a token from disk is decoupled from setting the auth header.

zyedidia commented 10 months ago

It's already possible to read the GitHub token from the configuration file (~/.eget.toml). Is this insufficient for your use-case? (not secret enough?)

hhromic commented 10 months ago

Ah yes, forgot to mention that indeed. I am aware of that possibility and even was accounting to be able to use my proposal there as well:

[global]
github_token = "@/path/to/token"

The reason to be able to do this is that for me, eget is not the only tool needing access to a GitHub token and therefore is much more convenient to mantain it in a single file sourced by applications. Also, reading tokens from files would make eget extremely easy to use inside of Docker containers (another use-case I'm thinking on introducing eget). For example, during a Docker image build, it is possible to specify secrets for the build which are exposed as files inside of the container.

If you are interested on bringing this feature, I'm also happy to work on it. Let me know.

zyedidia commented 10 months ago

I see, that sounds reasonable, so I went ahead and implemented it. Sorry for jumping the gun -- I noticed the code in SetAuthHeader was a bit messy anyway and could be refactored so it was easier this way.

hhromic commented 10 months ago

No problem! Nice you got motivated to do some cleanups! :)