smartnews / yoya-thumber Goto Github PK

#23 (comment)

func isWEBP(bytes []byte) bool {
	// 0x52 = R, 0x49 = I, 0x46 = F
	if bytes[0] != 0x52 || bytes[1] != 0x49 || bytes[2] != 0x46 || bytes[2] != 0x46 {
		return false
	}

WEBP has duplicated bytes[2] != 0x46

X bytes[0] != 0x52 || bytes[1] != 0x49 || bytes[2] != 0x46 || bytes[2] != 0x46
O bytes[0] != 0x52 || bytes[1] != 0x49 || bytes[2] != 0x46 || bytes[3] != 0x46

Honestly, we don't use FastCGI mode. FastCGI mode was broken for months...

If possible, I want to drop FastCGI mode and drop -local option.

odd size HEIC image output, it's truncated by 1 pixel.

ex) w=99,h=99,fo=heic => 98x98 HEIC image output.

Currently, yoya-thumber returns http.StatusBadGateway when it fails to download image from origin. From the point of view from client, it is hard to identify the root cause of error. Yoya-thumber needs to honor origin's HTTP status code.

func myClientImageGet(imageUrl string, referer string, userAgent string) (*http.Response, error) {
    .....
    srcReader, err = client.Do(req)
    if err != nil {
        glog.Warning("imageUrl not find " + imageUrl)
        return nil, err
    }
    // 200 以外はエラーにする (302 とかはどうしよう？)
    if srcReader.StatusCode != http.StatusOK {
        srcReader.Body.Close()
        return nil, errors.New("upstream status:" + srcReader.Status)
    }
    return srcReader, nil
}

.....

    srcReader, err := myClientImageGet(params.ImageUrl, r.Referer(), config.Http.UserAgent)
    if err != nil {
        message := "Upstream failed\tpath:" + path + "\treferer:" + r.Referer() + "\terror:" + err.Error()
        glog.Error(message, http.StatusBadGateway)
        http.Error(w, message, http.StatusBadGateway)
        atomic.AddInt64(&http_stats.upstream_error, 1)
        return
    }

Currently, thumberd.go uses glog as a logging library, while thumbnail_magick.go sometimes uses log.Printf.

IMO using glog is enough, thus we can omit log.Printf from yoya-thumber.

ImageMagick has HEIF support using libheif in 6.9.9-44, released in May 2018.
I also want to support HEIF in yoya-thumber.

I think it better to set a version on this product.

• https://github.com/smartnews/yoya-thumber/releases

for example.

• v1.0
• v1.0.0
  or
• 1.0
• 1.0.0
  etc...

• Guest OS Ubuntu 16.04 is too old. (Maintenance updates are coming to an end soon.)
  • 16.04 (2016/4/21) to 18.04 (2018/4/26)
• ImageMagick 6.9.9-15 is also too old. (There are a lot of fixes in between the latest.)
  • 6.9.9-15 (2017/9/23) to 6.9.11-6 (2020/4/7)
• Golang 1.9.1 is also too.
  • 1.9.1 (2017/10/04) to 1.14.1 (2020/03/19)

#19

Now we're being plagued by a weird performance regression issue on our production environment. Some long-running yoya-thumber process slow down.

• Slow process still can process requests, but the throughput is low
• CPU usage doesn't be maximized. Normal process utilization sometimes goes around 200%, but the problematic process doesn't use possible maximum CPU. On the other hand, the problematic process always use around 100% CPU resource (precisely, 103% to 110%).

I presume that there is a resource contention, but there's no way for further investigation.

I want to introduce net/http/pprof package to the yoya-thumber to resolve this issue.

yoya-thumber prohibits loopback address, but it seems that there is Time Of Check to Time Of Use (TOCTOU) attacks issue.

In yoya-thumber/thumberd/thumberd.go

	u, err = url.Parse(imageUrl)

        // CHECK
	// these codes are referencing net/http/transport.go useProxy method.
	if err != nil {
		return nil, err
	}
	if u.Host == "localhost" {
		return nil, errors.New("localhost is prohibited.")
	}
	if ip := net.ParseIP(u.Host); ip != nil {
		if ip.IsLoopback() {
			return nil, errors.New("loopback address is prohibited.")
		}
	}

        // Attacker may change DNS record

        // USE
	req, err := http.NewRequest("GET", imageUrl, nil)

SEE ALSO

• Goによるプライベートネットワークへのアクセスを禁止するHTTPクライアントの実装(written in Japanese)
• hakobe/paranoidhttp#1