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

Add internal routes for ssh hook comands #1471

Merged
merged 7 commits into from
Apr 19, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cmd/serv.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/private"
"code.gitea.io/gitea/modules/setting"
"github.com/Unknwon/com"
"github.com/dgrijalva/jwt-go"
Expand Down Expand Up @@ -316,7 +317,7 @@ func runServ(c *cli.Context) error {

// Update user key activity.
if keyID > 0 {
if err = models.UpdatePublicKeyUpdated(keyID); err != nil {
if err = private.UpdatePublicKeyUpdated(keyID); err != nil {
fail("Internal error", "UpdatePublicKey: %v", err)
}
}
Expand Down
6 changes: 6 additions & 0 deletions cmd/web.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
apiv1 "code.gitea.io/gitea/routers/api/v1"
"code.gitea.io/gitea/routers/dev"
"code.gitea.io/gitea/routers/org"
"code.gitea.io/gitea/routers/private"
"code.gitea.io/gitea/routers/repo"
"code.gitea.io/gitea/routers/user"

Expand Down Expand Up @@ -659,6 +660,11 @@ func runWeb(ctx *cli.Context) error {
apiv1.RegisterRoutes(m)
}, ignSignIn)

m.Group("/internal", func() {
Copy link
Member

Choose a reason for hiding this comment

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

This one should go in /api/internal

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

// package name internal is ideal but Golang is not allowed, so we use private as package name.
private.RegisterRoutes(m)
})

// robots.txt
m.Get("/robots.txt", func(ctx *context.Context) {
if setting.HasRobotsTxt {
Expand Down
6 changes: 4 additions & 2 deletions models/ssh_key.go
Original file line number Diff line number Diff line change
Expand Up @@ -502,8 +502,10 @@ func UpdatePublicKey(key *PublicKey) error {

// UpdatePublicKeyUpdated updates public key use time.
func UpdatePublicKeyUpdated(id int64) error {
cnt, err := x.ID(id).Cols("updated").Update(&PublicKey{
Updated: time.Now(),
now := time.Now()
cnt, err := x.ID(id).Cols("updated_unix").Update(&PublicKey{
Updated: now,
UpdatedUnix: now.Unix(),
})
if err != nil {
return err
Expand Down
5 changes: 5 additions & 0 deletions modules/httplib/httplib.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ func newRequest(url, method string) *Request {
return &Request{url, &req, map[string]string{}, map[string]string{}, defaultSetting, &resp, nil}
}

// NewRequest returns *Request with specific method
func NewRequest(url, method string) *Request {
return newRequest(url, method)
}

// Get returns *Request with GET method.
func Get(url string) *Request {
return newRequest(url, "GET")
Expand Down
51 changes: 51 additions & 0 deletions modules/private/internal.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package private
Copy link
Member

Choose a reason for hiding this comment

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

Why is it called private when then endpoint is /internal ? Pick one and stay with it 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

because Go don't allow a package named internal

Copy link
Member

Choose a reason for hiding this comment

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

ooh right, internal is "special" 😒

Copy link
Member

Choose a reason for hiding this comment

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

How about using /api/private then ? It's good to be consistent :)

Copy link
Member

Choose a reason for hiding this comment

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

And private.go, for that same reason...

Copy link
Member Author

Choose a reason for hiding this comment

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

@strk /api/private is inaccurate than /api/internal I think.


import (
"crypto/tls"
"encoding/json"
"fmt"
"net/http"

"code.gitea.io/gitea/modules/httplib"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
)

func newRequest(url, method string) *httplib.Request {
return httplib.NewRequest(url, method).Header("Authorization",
fmt.Sprintf("Bearer %s", setting.InternalToken))
}

// Response internal request response
type Response struct {
Err string `json:"err"`
}

func decodeJSONError(resp *http.Response) *Response {
var res Response
err := json.NewDecoder(resp.Body).Decode(&res)
if err != nil {
res.Err = err.Error()
}
return &res
}

// UpdatePublicKeyUpdated update publick key updates
func UpdatePublicKeyUpdated(keyID int64) error {
// Ask for running deliver hook and test pull request tasks.
reqURL := setting.LocalURL + fmt.Sprintf("internal/ssh/%d/update", keyID)
log.GitLogger.Trace("UpdatePublicKeyUpdated: %s", reqURL)

resp, err := newRequest(reqURL, "POST").SetTLSClientConfig(&tls.Config{
InsecureSkipVerify: true,
}).Response()
if err != nil {
return err
}

resp.Body.Close()
Copy link
Member

Choose a reason for hiding this comment

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

using defer?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

if resp.StatusCode/100 != 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Valid if the StatusCode is in 200...299 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, got it.

Copy link
Member

Choose a reason for hiding this comment

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

@lunny Please add a comment above as it's not clear why this check is here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

return fmt.Errorf("Failed to update public key: %s", decodeJSONError(resp).Err)
}
return nil
}
56 changes: 47 additions & 9 deletions modules/setting/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"code.gitea.io/gitea/modules/user"

"github.com/Unknwon/com"
"github.com/dgrijalva/jwt-go"
_ "github.com/go-macaron/cache/memcache" // memcache plugin for cache
_ "github.com/go-macaron/cache/redis"
"github.com/go-macaron/session"
Expand Down Expand Up @@ -443,14 +444,15 @@ var (
ShowFooterTemplateLoadTime bool

// Global setting objects
Cfg *ini.File
CustomPath string // Custom directory path
CustomConf string
CustomPID string
ProdMode bool
RunUser string
IsWindows bool
HasRobotsTxt bool
Cfg *ini.File
CustomPath string // Custom directory path
CustomConf string
CustomPID string
ProdMode bool
RunUser string
IsWindows bool
HasRobotsTxt bool
InternalToken string // internal access token
)

// DateLang transforms standard language locale name to corresponding value in datetime plugin.
Expand Down Expand Up @@ -765,6 +767,43 @@ please consider changing to GITEA_CUSTOM`)
ReverseProxyAuthUser = sec.Key("REVERSE_PROXY_AUTHENTICATION_USER").MustString("X-WEBAUTH-USER")
MinPasswordLength = sec.Key("MIN_PASSWORD_LENGTH").MustInt(6)
ImportLocalPaths = sec.Key("IMPORT_LOCAL_PATHS").MustBool(false)
InternalToken = sec.Key("INTERNAL_TOKEN").String()
if len(InternalToken) == 0 {
secretBytes := make([]byte, 32)
_, err := io.ReadFull(rand.Reader, secretBytes)
if err != nil {
log.Fatal(4, "Error reading random bytes: %v", err)
}

secretKey := base64.RawURLEncoding.EncodeToString(secretBytes)

now := time.Now()
InternalToken, err = jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
"nbf": now.Unix(),
}).SignedString([]byte(secretKey))

if err != nil {
log.Fatal(4, "Error generate internal token: %v", err)
}

// Save secret
cfgSave := ini.Empty()
if com.IsFile(CustomConf) {
// Keeps custom settings if there is already something.
if err := cfgSave.Append(CustomConf); err != nil {
log.Error(4, "Failed to load custom conf '%s': %v", CustomConf, err)
}
}

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

if err := os.MkdirAll(filepath.Dir(CustomConf), os.ModePerm); err != nil {
log.Fatal(4, "Failed to create '%s': %v", CustomConf, err)
}
if err := cfgSave.SaveTo(CustomConf); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Either use a token with expiration, OR save it in the ini-file, not both. It makes no sense to persist ephemeral values.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a reason to keep that token in the Ini file either, just have it randomly generated upon start and let it die/regenerate on restart

Copy link
Member Author

Choose a reason for hiding this comment

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

because we have to share the token between gitea web and gitea serv and gitea hook. And gitea web should be run firstly, then it will save token to ini if there is no token. After that, all other commands could read it.

Copy link
Member

Choose a reason for hiding this comment

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

@strk serv still has to get it from somewhere. This still makes sense :)

Copy link
Member

Choose a reason for hiding this comment

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

got it, thanks

log.Fatal(4, "Error saving generated JWT Secret to custom config: %v", err)
}
}

sec = Cfg.Section("attachment")
AttachmentPath = sec.Key("PATH").MustString(path.Join(AppDataPath, "attachments"))
Expand Down Expand Up @@ -935,7 +974,6 @@ var Service struct {
EnableOpenIDSignUp bool
OpenIDWhitelist []*regexp.Regexp
OpenIDBlacklist []*regexp.Regexp

}

func newService() {
Expand Down
44 changes: 44 additions & 0 deletions routers/private/internal.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright 2017 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

// Package private includes all internal routes. The package name internal is ideal but Golang is not allowed, so we use private as package name instead.
package private
Copy link
Member

Choose a reason for hiding this comment

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

same as above, internal or private doesn't matter to me, but the package name should reflect the endpoint name 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

So I will change all internal to private.

Copy link
Member

Choose a reason for hiding this comment

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

NVM actually, /private makes less sense than /internal. Just add a comment above package private explaining why it's called private instead of internal 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can just call it /ssh rather than /internal/ssh if there is no conflict.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just realized there already is modules/ssh. Does it make any sense to combine the two?

Copy link
Member Author

Choose a reason for hiding this comment

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

@typeless it's not only for ssh, it's also for hooks commands. The hooks commands will be fired by both SSH and HTTPS. So I think /ssh is not suitable. modules/ssh is for builtin SSH server, it's not related with this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bkcsoft done.


import (
"strings"

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/setting"
macaron "gopkg.in/macaron.v1"
)

// CheckInternalToken check internal token is set
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)
}
}

// UpdatePublicKey update publick key updates
func UpdatePublicKey(ctx *macaron.Context) {
keyID := ctx.ParamsInt64(":id")
if err := models.UpdatePublicKeyUpdated(keyID); err != nil {
ctx.JSON(500, map[string]interface{}{
"err": err.Error(),
})
return
}

ctx.PlainText(200, []byte("success"))
}

// 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) {
Copy link
Member

Choose a reason for hiding this comment

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

We really need to start doing this in more places ❤️

m.Group("/", func() {
m.Post("/ssh/:id/update", UpdatePublicKey)
}, CheckInternalToken)
}