diff --git a/pkg/mcclient/auth/aksk.go b/pkg/mcclient/auth/aksk.go index b5563deb8d..73046544e3 100644 --- a/pkg/mcclient/auth/aksk.go +++ b/pkg/mcclient/auth/aksk.go @@ -16,6 +16,7 @@ package auth import ( "net/http" + "time" "yunion.io/x/pkg/errors" "yunion.io/x/pkg/util/cache" @@ -24,6 +25,41 @@ import ( "yunion.io/x/onecloud/pkg/mcclient" ) +// maxSignatureSkew is the maximal clock skew accepted for a signed request, +// the same window AWS uses: requests signed earlier can not be replayed +// indefinitely. +const maxSignatureSkew = 15 * time.Minute + +// checkRequestFreshness rejects replayed signatures: a signed request is only +// accepted within 15 minutes of its signing time. +func checkRequestFreshness(req http.Request) error { + dateStr := req.Header.Get("x-amz-date") + if len(dateStr) > 0 { + signTime, err := time.Parse("20060102T150405Z", dateStr) + if err != nil { + return errors.Wrap(err, "invalid x-amz-date") + } + return checkTimeSkew(signTime) + } + dateStr = req.Header.Get("Date") + if len(dateStr) > 0 { + signTime, err := http.ParseTime(dateStr) + if err != nil { + return errors.Wrap(err, "invalid Date header") + } + return checkTimeSkew(signTime) + } + return errors.Error("missing signing date") +} + +func checkTimeSkew(signTime time.Time) error { + skew := time.Since(signTime) + if skew > maxSignatureSkew || skew < -maxSignatureSkew { + return errors.Errorf("request signature expired, signed at %s", signTime) + } + return nil +} + type sAccessKeyCache struct { *cache.LRUCache } @@ -64,6 +100,9 @@ func (c *sAccessKeyCache) Verify(cli *mcclient.Client, req http.Request, virtual if err != nil { return nil, errors.Wrap(err, "s3auth.DecodeAccessKeyRequestV2") } + if err := checkRequestFreshness(req); err != nil { + return nil, errors.Wrap(err, "checkRequestFreshness") + } token, found := c.getToken(aksk.GetAccessKey()) if found { diff --git a/pkg/mcclient/auth/aksk_test.go b/pkg/mcclient/auth/aksk_test.go new file mode 100644 index 0000000000..3b60b7f1e1 --- /dev/null +++ b/pkg/mcclient/auth/aksk_test.go @@ -0,0 +1,78 @@ +// Copyright 2019 Yunion +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package auth + +import ( + "net/http" + "testing" + "time" +) + +func TestCheckRequestFreshness(t *testing.T) { + now := time.Now() + + cases := []struct { + name string + req http.Request + wantErr bool + }{ + { + name: "v4 recent x-amz-date", + req: http.Request{Header: http.Header{"X-Amz-Date": []string{now.UTC().Format("20060102T150405Z")}}}, + wantErr: false, + }, + { + name: "v4 expired x-amz-date", + req: http.Request{Header: http.Header{"X-Amz-Date": []string{now.Add(-time.Hour).UTC().Format("20060102T150405Z")}}}, + wantErr: true, + }, + { + name: "v4 future x-amz-date", + req: http.Request{Header: http.Header{"X-Amz-Date": []string{now.Add(time.Hour).UTC().Format("20060102T150405Z")}}}, + wantErr: true, + }, + { + name: "v4 malformed x-amz-date", + req: http.Request{Header: http.Header{"X-Amz-Date": []string{"not-a-date"}}}, + wantErr: true, + }, + { + name: "v2 recent Date header", + req: http.Request{Header: http.Header{"Date": []string{now.UTC().Format(http.TimeFormat)}}}, + wantErr: false, + }, + { + name: "v2 expired Date header", + req: http.Request{Header: http.Header{"Date": []string{now.Add(-time.Hour).UTC().Format(http.TimeFormat)}}}, + wantErr: true, + }, + { + name: "missing date", + req: http.Request{Header: http.Header{}}, + wantErr: true, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + err := checkRequestFreshness(c.req) + if c.wantErr && err == nil { + t.Fatalf("expected error, got nil") + } + if !c.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} diff --git a/pkg/s3gateway/handlers/handlers.go b/pkg/s3gateway/handlers/handlers.go index 6ad1aa21bc..4d39ed5679 100644 --- a/pkg/s3gateway/handlers/handlers.go +++ b/pkg/s3gateway/handlers/handlers.go @@ -17,7 +17,6 @@ package handlers import ( "context" "net/http" - "net/url" "strings" "time" @@ -115,11 +114,10 @@ func getObjectRequest(r *http.Request) (SObjectRequest, error) { } else { return o, errors.Error("invalid S3 request") } - var err error - o.Key, err = url.PathUnescape(o.Key) - if err != nil { - return o, errors.Wrap(err, "url.PathUnescape") - } + // r.URL.Path is already percent-decoded once by net/http, and the + // signature covers exactly this form. Decoding again would let a valid + // signature for one key be replayed against a different key + // (e.g. %252e%252e%252f would resolve to ../) return o, o.Validate() } diff --git a/pkg/s3gateway/handlers/handlers_test.go b/pkg/s3gateway/handlers/handlers_test.go new file mode 100644 index 0000000000..a691569d61 --- /dev/null +++ b/pkg/s3gateway/handlers/handlers_test.go @@ -0,0 +1,55 @@ +// Copyright 2019 Yunion +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package handlers + +import ( + "net/http" + "net/url" + "testing" +) + +// the object key must be percent-decoded exactly once (the form the +// signature covers); decoding twice would let a signature for one key be +// replayed against another +func TestGetObjectRequestSingleDecode(t *testing.T) { + cases := []struct { + name string + urlPath string // URL.Path as net/http sets it: raw path decoded once + wantKey string + }{ + // raw /bucket/a%20b -> Path /bucket/a b + {name: "simple space", urlPath: "/bucket/a b", wantKey: "a b"}, + // raw /bucket/a%252Fb -> Path /bucket/a%2Fb, must stay literal + {name: "literal percent", urlPath: "/bucket/a%2Fb", wantKey: "a%2Fb"}, + // raw /bucket/a%252e%252e%252fc -> Path /bucket/a%2e%2e%2fc, + // must not resolve to a/../c + {name: "no double dot resolution", urlPath: "/bucket/a%2e%2e%2fc", wantKey: "a%2e%2e%2fc"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + req := &http.Request{ + Host: "127.0.0.1", + URL: &url.URL{Path: c.urlPath}, + } + o, err := getObjectRequest(req) + if err != nil { + t.Fatalf("getObjectRequest: %v", err) + } + if o.Key != c.wantKey { + t.Fatalf("key = %q, want %q", o.Key, c.wantKey) + } + }) + } +}