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

Feature Request: Move INTERNAL_TOKEN value out of app.ini #3246

Closed
1 of 7 tasks
minoru7 opened this issue Dec 20, 2017 · 44 comments · Fixed by #5812
Closed
1 of 7 tasks

Feature Request: Move INTERNAL_TOKEN value out of app.ini #3246

minoru7 opened this issue Dec 20, 2017 · 44 comments · Fixed by #5812
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/refactoring Existing code has been cleaned up. There should be no new functionality.
Milestone

Comments

@minoru7
Copy link

minoru7 commented Dec 20, 2017

  • Gitea version (or commit ref): latest
  • Git version: latest
  • Operating system: Linux
  • Database (use [x]):
    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite
  • Can you reproduce the bug at https://try.gitea.io:
    • Yes (provide example URL)
    • No
    • Not relevant
  • Log gist:

Description

I'd like to request that the INTERNAL_TOKEN value that is currently placed in the app.ini during Gitea startup be moved to a separate file/config than the user-defined app.ini. It should probably be placed in a lockfile, pid, or separate gitea-system-controlled config file, etc.

The reason for this is that a lot of organizations use automation tools such as Puppet, SaltStack, Ansible, etc. to deploy configurations to servers. If the app.ini (user-defined config file) is being managed by one of these, it will overwrite the existing app.ini that includes the INTERNAL_TOKEN. This is less than ideal, and obviously breaks the functioning of Gitea.

References:

@lafriks lafriks added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Dec 20, 2017
@danielfbm
Copy link

same problem using kubernetes and configmaps, config files should not be used to save any kind of state

@lunny lunny added this to the 1.x.x milestone Dec 28, 2017
@xoxys
Copy link
Contributor

xoxys commented Jan 11, 2018

Any chance to fix this in near future? It's annoying to use idempotent config management with this...

@strk
Copy link
Member

strk commented Jan 11, 2018 via email

@xoxys
Copy link
Contributor

xoxys commented Jan 11, 2018

👍 will have a look

@cdrage
Copy link

cdrage commented Mar 27, 2018

@xoxys @strk

Any updates? We are unfortunately blocked on this and we're unable to add Gitea as a Helm chart to Kubernetes since it's trying to write to app.ini... Despite app.ini only used for configuration, INTERNAL_TOKEN is being updated within in.

See errors: https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/charts/3408/pull-charts-e2e/7056/build-log.txt

Also our Helm chart PR here: helm/charts#3408

@cdrage
Copy link

cdrage commented Mar 27, 2018

ping @lafriks

@cdrage
Copy link

cdrage commented Mar 27, 2018

Specifically:

I0326 19:50:47.406] ---Logs from container gitea in pod gitea-7056-1-gitea-5f94847576-krj5q:---
I0326 19:50:47.658] Generating /data/ssh/ssh_host_ed25519_key...
I0326 19:50:47.659] chown: /data/gitea/conf/app.ini: Read-only file system
I0326 19:50:47.659] Mar 26 19:32:40 syslogd started: BusyBox v1.26.2
I0326 19:50:47.659] Generating /data/ssh/ssh_host_rsa_key...
I0326 19:50:47.659] Generating /data/ssh/ssh_host_dsa_key...
I0326 19:50:47.660] Generating /data/ssh/ssh_host_ecdsa_key...
I0326 19:50:47.660] /etc/ssh/sshd_config line 32: Deprecated option UsePrivilegeSeparation
I0326 19:50:47.660] Mar 26 19:32:40 sshd[14]: Server listening on :: port 22.
I0326 19:50:47.660] Mar 26 19:32:40 sshd[14]: Server listening on 0.0.0.0 port 22.
I0326 19:50:47.661] 2018/03/26 19:32:40 �[1;31m[...s/setting/setting.go:924 NewContext()] [E] Error saving generated JWT Secret to custom config: open /data/gitea/conf/app.ini: read-only file system�[0m
I0326 19:50:47.661] chown: /data/gitea/conf/app.ini: Read-only file system
I0326 19:50:47.661] 2018/03/26 19:32:41 �[1;31m[...s/setting/setting.go:924 NewContext()] [E] Error saving generated JWT Secret to custom config: open /data/gitea/conf/app.ini: read-only file system�[0m
I0326 19:50:47.662] chown: /data/gitea/conf/app.ini: Read-only file system
I0326 19:50:47.662] 2018/03/26 19:32:42 �[1;31m[...s/setting/setting.go:924 NewContext()] [E] Error saving generated JWT Secret to custom config: open /data/gitea/conf/app.ini: read-only file system�[0m
I0326 19:50:47.662] chown: /data/gitea/conf/app.ini: Read-only file system

Which is causing the issue (Gitea is trying to write to app.ini when it shouldn't..)

@strk
Copy link
Member

strk commented Mar 27, 2018 via email

@lafriks
Copy link
Member

lafriks commented Mar 27, 2018

@cdrage you can pregenerate token and other values using gitea cli command generate that was added in 1.4.0 (see https://docs.gitea.io/en-us/command-line/)

@cdrage
Copy link

cdrage commented Mar 28, 2018

@lafriks Yup. I saw from your other comments on the other issues. Yes, that helps with configuration and setting up your .ini file. However, this issue is regarding when INTERNAL_TOKEN isn't set, what gitea should do.

The problem is that app.ini or whatever configuration file you use, should be immutable when deploying (through Ansible, read-only file system, etc.)

My thinking:

If INTERNAL_TOKEN does not exist, the key should be automatically generated and set internally rather than being passed to the configuration file.

Reference:

cfgSave.Section("security").Key("INTERNAL_TOKEN").SetValue(InternalToken)

I'll most likely push a PR later this week once I have enough time!

@Nodraak
Copy link

Nodraak commented Mar 29, 2018

Hi @cdrage! I am very much interested in deploying Gitea with a k8s Chart. If you need any help (testing, review, code, ...), don't hesitate to ping me :)

@cdrage
Copy link

cdrage commented Mar 29, 2018

@Nodraak There's already a PR: helm/charts#3408 but we're blocked on this issue 👍

@lunny
Copy link
Member

lunny commented Mar 29, 2018

Maybe put that on a file like pid? @cdrage the INTERNAL_TOKEN token will be shared by gitea web and gitea hook processes. So I don't think a memory shared is enough. Any idea?

@cdrage
Copy link

cdrage commented Mar 29, 2018

@lunny Yeah, anything else preferably, pid would probably do. The code:

cfgSave.Section("security").Key("INTERNAL_TOKEN").SetValue(InternalToken)
shows that INTERNAL_TOKEN is actually the only variable that it set within the configuration, no other environment variables are set / modify the configuration file after launching.

Alternatively (as a hack, for now) we could set the internal token as part of the configMap within Kubernetes (before launching). But then again.. it would be nice to have it automatically generated instead.

Anyways! I'll try to push a PR this/next week.

@Nodraak
Copy link

Nodraak commented Mar 30, 2018

TL;DR: for internal gitea calls, why not call the function directly, instead of using an HTTP api?


Reading again this thread, I have some trouble understanding the utility of INTERNAL_TOKEN.

Correct me if I'm wrong, but from what I have understood:

  • If the config file contains INTERNAL_TOKEN, gitea works fine (when deployed bare metal or with kubernetes)
  • If the config file does not contain INTERNAL_TOKEN, gitea will try to write one to the config file, which will fail when deployed with kubernetes (why? does k8s changes the file permissions? or is the problem when k8s deploys a new version which overwrite the token?)
  • If the values of the token is changed, gitea is broken. (why? If we restart gitea, it should read the token again from the file, shouldnt it?)
  • This INTERNAL_TOKEN seems to be used only in this function:
    func CheckInternalToken(ctx *macaron.Context) {
    tokens := ctx.Req.Header.Get("Authorization")
    fields := strings.Fields(tokens)
    if len(fields) != 2 || fields[0] != "Bearer" || fields[1] != setting.InternalToken {
    ctx.Error(403)
    }
    }

    Which is used here:
    // RegisterRoutes registers all internal APIs routes to web application.
    // These APIs will be invoked by internal commands for example `gitea serv` and etc.
    func RegisterRoutes(m *macaron.Macaron) {
    m.Group("/", func() {
    m.Post("/ssh/:id/update", UpdatePublicKey)
    m.Post("/push/update", PushUpdate)
    m.Get("/protectedbranch/:pbid/:userid", CanUserPush)
    m.Get("/branch/:id/*", GetProtectedBranchBy)
    }, CheckInternalToken)
    }

    I guess the goal is to verify if the call comes from gitea itself or from outside. Then for internal gitea calls, why not call the function directly, instead of using an HTTP api?
    For example, the HTTP route /ssh/:id/update is used here:
    func UpdatePublicKeyUpdated(keyID int64) error {
    // Ask for running deliver hook and test pull request tasks.
    reqURL := setting.LocalURL + fmt.Sprintf("api/internal/ssh/%d/update", keyID)
    log.GitLogger.Trace("UpdatePublicKeyUpdated: %s", reqURL)
    resp, err := newInternalRequest(reqURL, "POST").Response()
    if err != nil {
    return err
    }
    defer resp.Body.Close()
    // All 2XX status codes are accepted and others will return an error
    if resp.StatusCode/100 != 2 {
    return fmt.Errorf("Failed to update public key: %s", decodeJSONError(resp).Err)
    }
    return nil
    }

    Then, instead of reqURL := setting.LocalURL + fmt.Sprintf("api/internal/ssh/%d/update", keyID) and newInternalRequest(reqURL, "POST"), why not do UpdatePublicKey(keyID)?

Nodraak added a commit to Nodraak/gitea that referenced this issue Mar 30, 2018
Nodraak added a commit to Nodraak/gitea that referenced this issue Mar 30, 2018
@Nodraak
Copy link

Nodraak commented Mar 30, 2018

I've played a little with the code, and managed to substitute the calls (replace HTTP requests by function calls) and to remove all references to INTERNAL_TOKEN. I have not tested extensively, but all the tests passes.

I won't submit a PR yet, but please have a look at my two commits (the most relevant is commit Nodraak@c20afe9 "Substitute calls") and if they solve properly this issue, I can open a PR. I am not aware of all the trade-off of gitea, so I probably missed something.


@ cdrage: I did have seen that you planned to work on a PR, sorry for "stealing" your work. I just tried to naively implement my idea :)

@lafriks
Copy link
Member

lafriks commented Mar 30, 2018

@Nodraak what you have removed whas specially made so that ssh process would not need to create database connection that update data as this totally break sqlite3 db backend (sqlite can have only single connection that updates data)

@Nodraak
Copy link

Nodraak commented Mar 30, 2018

Ahah, so that was what I missed. Actually, I'm stupid, I should have run a git blame and dig in the git history. Anyway.

So, if I understand correctly, concurrent write access and timeouts are handled by the HTTP protocol (I guess gitea has only one webserver, so there can be only one access to the DB), instead of directly by the database. I don't see why this is a better solution, IMO it adds a lot of complexity (due to indirect calls). Moreover, sqlite support concurrent write acces: the transaction will retry until the DB is unlock or the timeout expires. By default the timeout is 0 so any concurrent write will immediatly return Error: database is locked, but with _busy_timeout one can set an appropriate timeout, for instance a few seconds. (the parameter _busy_timeout is passed when opening the connection)

@lafriks
Copy link
Member

lafriks commented Mar 30, 2018

@Nodraak internal calls is planned to increase most probably if we want to support clustering of some kind

@lafriks
Copy link
Member

lafriks commented Mar 30, 2018

As for sqlite locking it is a bit more difficult because we use xorm and also there is GoLang layer not directly c calls

@lunny
Copy link
Member

lunny commented Mar 30, 2018

Two benefit for this change from both gitea web and gitea hook connected to mysql or sqlite. One is to resolve sqlite share write problem. Another is SSH could be deployed different machine from web in future.

@cdrage
Copy link

cdrage commented Jun 6, 2018

Did anyone else happen to start work on this recently? This is still blocking us from deploying Gitea to Kubernetes: helm/charts#3408

@lafriks @Nodraak

@lunny
Copy link
Member

lunny commented Jun 6, 2018

We could create a single file on data/internal/lock to share between multiple processes?

@techknowlogick
Copy link
Member

@cdrage Is this indeed blocking? There have been others who have deployed Gitea to kubernetes

@minoru7
Copy link
Author

minoru7 commented Jun 6, 2018

While I certainly have been able to work around the INTERNAL_TOKEN issue myself in Puppet and elsewhere in automation, the workarounds are less than ideal. It makes life difficult to manage Gitea settings while Gitea is modifying the settings file that I'm laying down through automation. The applications tend to fight each other. While this may not be critical, it is definitely a fairly large hiccup.

Thank you to anyone who has the time to look into this and place a PR! You would be my savior, that's for sure. ;-)

@cdrage
Copy link

cdrage commented Jun 6, 2018

@techknowlogick Yes. You're unable to create a ConfigMap for the app.ini settings. If you go by the links you posted, it's using volumes for configuration management, rather than the better way of using ConfigMaps (more Kubernetes-esque).

@minoru7 What was your work-around?

@minoru7
Copy link
Author

minoru7 commented Jun 6, 2018

@cdrage Well, for Puppet, I have the configuration file being laid down only if it doesn't already exist. Which, that's fine for a new system, but defeats the purpose of Puppet and continuous configuration. If I make a change to the configuration, which happens on occasion, I have to manually delete the app.ini to have it take effect. As for Kubernetes, I haven't had enough time yet to start digging into that, so unfortunately, I don't have any help for you there, sorry.

@cdrage
Copy link

cdrage commented Jun 6, 2018

@minoru7 Thanks!

That helps, since it's the exact same issue as when deploying to Kubernetes. Since app.ini should be immutable, it's difficult to have continuous configuration with INTERNAL_TOKEN making changes to the app.ini file.

So essentially it's also blocking any Puppet deployment too.

@lunny
Copy link
Member

lunny commented Jun 6, 2018

Could INTERNAL_TOKEN be set when first set up?

@minoru7
Copy link
Author

minoru7 commented Jun 6, 2018

@lunny I believe, last I remember, that it was linked to the daemon or somesuch. The INTERNAL_TOKEN value changes upon each restart of the service. So that's why my original request mentioned maybe adding this to a pid file or something instead.

@lunny
Copy link
Member

lunny commented Jun 6, 2018

The only one place to change the token is https://github.com/go-gitea/gitea/blob/master/modules/setting/setting.go#L918 when INTERNAL_TOKEN is empty. If it is changed each restart that maybe a bug.

@lunny
Copy link
Member

lunny commented Jun 6, 2018

And in fact we can generate it via

func runGenerateInternalToken(c *cli.Context) error {
gitea generate

@cdrage
Copy link

cdrage commented Jun 6, 2018

Yeah. The problem is that on each restart, a new token is generated. If you decide to set one yourself at start-up (providing INTERNAL_TOKEN to app.ini) the service will run into an error of trying to modify app.ini despite it already being set (see my above comments for the log)

@lunny
Copy link
Member

lunny commented Jun 6, 2018

OK. I will investigate it.

@bkcsoft
Copy link
Member

bkcsoft commented Jul 10, 2018

So, as far as I can see, the code should only generate this token if it does not exist. Is this still an issue?

@minoru7
Copy link
Author

minoru7 commented Jul 10, 2018

I'm using version 1.3.2. Last time I attempted it, I generated a token and inserted it into the ini file. When I restarted the service it overwrote my InternalToken with a newly generated one, which then threw Puppet into overwriting it, which restarts the service, which Puppet then overwrites again. It was a bad situation. Not only that, but if I'm rolling out a few of these servers, I want to be able to automate that Token generation in that case. Or otherwise, the best scenario would be to take the Token out of the config file that Puppet would need to manage. I have not attempted a newer version of Gitea, so maybe you guys have sorted out my original issue? I'll give it a try soon when time permits.

@stale
Copy link

stale bot commented Jan 19, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.

@stale stale bot added the issue/stale label Jan 19, 2019
@andreymal
Copy link

@​stale nope plz

@stale stale bot removed the issue/stale label Jan 19, 2019
@cdrage
Copy link

cdrage commented Jan 21, 2019

Yeah, this is still a major blocker getting Gitea on Kubernetes.

@pytimer
Copy link

pytimer commented Jan 23, 2019

@cdrage I follow your helm chart to deploy gitea on the Kubernetes 1.11.6, and i add INTERNAL_TOKEN in configmap.yaml, and it's works well. So can we add internalToken in gitea chart values.yaml first? If this feature completed, we can change.

Below is my configmap.yaml :

[security]
    INTERNAL_TOKEN = abcdef123456

@cdrage
Copy link

cdrage commented Jan 23, 2019

@pytimer The problem is that INTERNAL_TOKEN is the only value that's not hard-coded / doesn't change for Gitea. It's not "kubernetes like" to have to modify configmap.yaml afterwards. That's why we should move INTERNAL_TOKEN out of app.ini and somewhere else.

@techknowlogick techknowlogick added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Jan 23, 2019
@sapk
Copy link
Member

sapk commented Feb 19, 2019

I think the real problem is that internal token should not change and I can't find why it should in the code.
The PR #3531 should have introduced all the needed generator to provide a valid and stable configuration.

@lunny lunny modified the milestones: 1.x.x, 1.8.0 Mar 16, 2019
@lunny
Copy link
Member

lunny commented Mar 16, 2019

In fact I like @sapk's idea.

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet