fix: close the findings of the final pre-release review

.github/workflows/go.yml

@@ -41,8 +41,11 @@ jobs:
 
       - name: Build on ${{ matrix.os }}
         if: matrix.os == 'windows-latest'
+        # bash, not pwsh: pwsh only propagates the last command's exit code,
+        # and %GOPATH% is a cmd.exe form that pwsh leaves unexpanded.
+        shell: bash
         run: |
-          go build --ldflags="-s -w" -o %GOPATH%\bin\mc.exe
+          go build --ldflags="-s -w" -o "$(go env GOPATH)/bin/mc.exe"
           go test -v -race --timeout 30m ./...
       - name: Build on ${{ matrix.os }}
         if: matrix.os == 'macos-latest'

buildscripts/check-release-commit.sh

@@ -57,9 +57,10 @@ if [ -z "${fixture}" ]; then
 
   error_file="$(mktemp)"
   trap 'rm -f "${error_file}"' EXIT
-  # exclude_pull_requests: a pull_request run checks out GitHub's synthetic
-  # merge ref, so a green run proves the merge result was good, not that this
-  # commit was ever built on its own.
+  # A pull_request run checks out GitHub's synthetic merge ref, so a green
+  # run proves the merge result was good, not that this commit was ever built
+  # on its own. exclude_pull_requests only trims the pull_requests field of
+  # each run; the event/head_branch filter below is what drops those runs.
   if ! runs_json="$(
     gh api --paginate \
       "repos/${repository}/actions/runs?head_sha=${release_commit}&exclude_pull_requests=true&per_page=100" \

cmd/admin-trace.go

@@ -700,21 +700,29 @@ func shortTrace(ti madmin.ServiceTraceInfo) shortTraceMsg {
 	s := shortTraceMsg{}
 	t := ti.Trace
 
+	// The server supplies every text field verbatim for other clients'
+	// requests; the short rendering must withhold credentials exactly like
+	// the verbose one. Work on redacted copies - the event is shared.
+	var eventSecrets []string
+	if t.HTTP != nil {
+		eventSecrets = traceEventSecrets(t.HTTP.ReqInfo.Headers, t.HTTP.RespInfo.Headers)
+	}
+
 	s.trcType = t.TraceType
 	s.Type = t.TraceType.String()
 	s.FuncName = t.FuncName
 	s.Time = t.Time
 	s.Path = t.Path
-	s.Error = t.Error
+	s.Error = redactTraceText(t.Error, eventSecrets)
 	s.Host = t.NodeName
 	s.Duration = t.Duration
-	s.StatusMsg = t.Message
-	s.Extra = t.Custom
+	s.StatusMsg = redactTraceText(t.Message, eventSecrets)
+	s.Extra = redactTraceCustom(t.Custom, eventSecrets)
 	s.Size = t.Bytes
 
 	switch t.TraceType {
 	case madmin.TraceS3, madmin.TraceInternal:
-		s.Query = t.HTTP.ReqInfo.RawQuery
+		s.Query = redactTraceQuery(t.HTTP.ReqInfo.RawQuery, eventSecrets)
 		s.StatusCode = t.HTTP.RespInfo.StatusCode
 		s.StatusMsg = http.StatusText(t.HTTP.RespInfo.StatusCode)
 		s.Client = t.HTTP.ReqInfo.Client
@@ -819,6 +827,10 @@ func colorizedNodeName(nodeName string) string {
 }
 
 func (t traceMessage) JSON() string {
+	var eventSecrets []string
+	if t.Trace.HTTP != nil {
+		eventSecrets = traceEventSecrets(t.Trace.HTTP.ReqInfo.Headers, t.Trace.HTTP.RespInfo.Headers)
+	}
 	trc := verboseTrace{
 		trcType:    t.Trace.TraceType,
 		Type:       t.Trace.TraceType.String(),
@@ -827,10 +839,10 @@ func (t traceMessage) JSON() string {
 		Time:       t.Trace.Time,
 		Duration:   t.Trace.Duration,
 		Path:       t.Trace.Path,
-		Error:      t.Trace.Error,
+		Error:      redactTraceText(t.Trace.Error, eventSecrets),
 		HealResult: t.Trace.HealResult,
-		Message:    t.Trace.Message,
-		Extra:      t.Trace.Custom,
+		Message:    redactTraceText(t.Trace.Message, eventSecrets),
+		Extra:      redactTraceCustom(t.Trace.Custom, eventSecrets),
 	}
 
 	if t.Trace.HTTP != nil {
@@ -850,14 +862,13 @@ func (t traceMessage) JSON() string {
 		for k, v := range redactHeaderMap(rs.Headers) {
 			rspHdrs[k] = strings.Join(v, " ")
 		}
-		eventSecrets := traceEventSecrets(rq.Headers, rs.Headers)
 
 		trc.RequestInfo = &requestInfo{
 			Time:     rq.Time,
 			Proto:    rq.Proto,
 			Method:   rq.Method,
 			Path:     rq.Path,
-			RawQuery: redactTraceText(rq.RawQuery, eventSecrets),
+			RawQuery: redactTraceQuery(rq.RawQuery, eventSecrets),
 			Body:     redactTraceText(string(rq.Body), eventSecrets),
 			Headers:  rqHdrs,
 		}
@@ -889,13 +900,23 @@ func (t traceMessage) String() string {
 	var nodeNameStr string
 	b := &strings.Builder{}
 
+	// Render a redacted copy: the server supplies the error, message and
+	// annotations verbatim for other clients' requests, and the event is
+	// shared with the JSON path.
 	trc := t.Trace
+	var eventSecrets []string
+	if trc.HTTP != nil {
+		eventSecrets = traceEventSecrets(trc.HTTP.ReqInfo.Headers, trc.HTTP.RespInfo.Headers)
+	}
+	trc.Error = redactTraceText(trc.Error, eventSecrets)
+	trc.Message = redactTraceText(trc.Message, eventSecrets)
+	trc.Custom = redactTraceCustom(trc.Custom, eventSecrets)
 	if trc.NodeName != "" {
 		nodeNameStr = fmt.Sprintf("%s ", colorizedNodeName(trc.NodeName))
 	}
 	extra := ""
-	if len(t.Trace.Custom) > 0 {
-		for k, v := range t.Trace.Custom {
+	if len(trc.Custom) > 0 {
+		for k, v := range trc.Custom {
 			extra = fmt.Sprintf("%s %s=%s", extra, k, v)
 		}
 		extra = console.Colorize("Extra", extra)
@@ -927,12 +948,11 @@ func (t traceMessage) String() string {
 	// is shared with the JSON path and must not be mutated.
 	reqHeaders := redactHeaderMap(ri.Headers)
 	respHeaders := redactHeaderMap(rs.Headers)
-	eventSecrets := traceEventSecrets(ri.Headers, rs.Headers)
 	fmt.Fprintf(b, "%s%s", nodeNameStr, console.Colorize("Request", fmt.Sprintf("[REQUEST %s] ", trc.FuncName)))
 	fmt.Fprintf(b, "[%s] %s\n", ri.Time.Local().Format(traceTimeFormat), console.Colorize("Host", fmt.Sprintf("[Client IP: %s]", ri.Client)))
 	fmt.Fprintf(b, "%s%s", nodeNameStr, console.Colorize("Method", fmt.Sprintf("%s %s", ri.Method, ri.Path)))
 	if ri.RawQuery != "" {
-		fmt.Fprintf(b, "?%s", redactTraceText(ri.RawQuery, eventSecrets))
+		fmt.Fprintf(b, "?%s", redactTraceQuery(ri.RawQuery, eventSecrets))
 	}
 	fmt.Fprint(b, "\n")
 	fmt.Fprintf(b, "%s%s", nodeNameStr, console.Colorize("Method", fmt.Sprintf("Proto: %s\n", ri.Proto)))
@@ -1075,3 +1095,27 @@ func redactTraceText(text string, eventSecrets []string) string {
 	text = redactSecretValues(text, eventSecrets)
 	return scrubKnownSecrets(text)
 }
+
+// redactTraceQuery redacts a raw query string. The query shape keys on the
+// "?" or "&" in front of a parameter name, so the "?" the trace strips is put
+// back for the scan; a credential in the first parameter is then caught too.
+func redactTraceQuery(rawQuery string, eventSecrets []string) string {
+	if rawQuery == "" {
+		return ""
+	}
+	redacted := redactTraceText("?"+rawQuery, eventSecrets)
+	return strings.TrimPrefix(redacted, "?")
+}
+
+// redactTraceCustom returns a redacted copy of an event's custom annotations;
+// the event is shared with the other rendering and is left untouched.
+func redactTraceCustom(custom map[string]string, eventSecrets []string) map[string]string {
+	if len(custom) == 0 {
+		return custom
+	}
+	redacted := make(map[string]string, len(custom))
+	for key, value := range custom {
+		redacted[key] = redactTraceText(value, eventSecrets)
+	}
+	return redacted
+}

cmd/admin-user-svcacct-set.go

@@ -113,11 +113,11 @@ func mainAdminUserSvcAcctSet(ctx *cli.Context) error {
 		buf, e = os.ReadFile(policyPath)
 		fatalIf(probe.NewError(e), "Unable to open the policy document.")
 
-		p, e := parsePolicyForWrite(buf)
+		// An empty document is the one way to clear an inline policy and
+		// return the account to its inherited one: the server treats
+		// {"Statement":[]} as a reset. Validate the shape, allow emptiness.
+		_, e = parsePolicyForWrite(buf)
 		fatalIf(probe.NewError(e), "Unable to parse the policy document.")
-		if p.IsEmpty() {
-			fatalIf(errInvalidArgument(), "empty policies are not allowed")
-		}
 	}
 
 	var expiryTime time.Time

cmd/checksum-verify.go

@@ -101,7 +101,7 @@ var checksumVerifyFlags = []cli.Flag{
 	},
 	cli.StringFlag{
 		Name:  "max-size",
-		Usage: "skip objects larger than this size (e.g. 10GiB)",
+		Usage: "skip objects larger than this size (e.g. 10GiB); 0 or empty means no limit",
 	},
 	cli.StringFlag{
 		Name:  "manifest",
@@ -400,7 +400,7 @@ func parseChecksumVerifyMaximumSize(value string) (int64, error) {
 	}
 	size, err := humanize.ParseBytes(value)
 	if err != nil {
-		return 0, err
+		return 0, fmt.Errorf("%q is not a size; use a value such as 10GiB, or 0 for no limit", value)
 	}
 	if size > uint64(^uint64(0)>>1) {
 		return 0, fmt.Errorf("maximum size is too large")
@@ -596,11 +596,13 @@ func checksumVerifyResultFor(candidate checksumVerifyCandidate) checksumVerifyRe
 	}
 }
 
-func checksumVerifyErrorResult(candidate checksumVerifyCandidate, err error) checksumVerifyResult {
-	return applyChecksumVerifyError(checksumVerifyResultFor(candidate), err)
+func checksumVerifyErrorResult(candidate checksumVerifyCandidate, err error, sseSupplied bool) checksumVerifyResult {
+	return applyChecksumVerifyError(checksumVerifyResultFor(candidate), err, sseSupplied)
 }
 
-func applyChecksumVerifyError(result checksumVerifyResult, err error) checksumVerifyResult {
+// sseSupplied tells an SSE-C rejection with a key from one without: with a
+// key the server's complaint is about the request, not a missing key.
+func applyChecksumVerifyError(result checksumVerifyResult, err error, sseSupplied bool) checksumVerifyResult {
 	response := minio.ToErrorResponse(err)
 	result.ErrorCode = response.Code
 	// Server-controlled text, printed and written to --report: an endpoint
@@ -618,10 +620,10 @@ func applyChecksumVerifyError(result checksumVerifyResult, err error) checksumVe
 		result.Result = checksumResultUnknownObjectChanged
 	case strings.Contains(code, "kms") || strings.Contains(message, "kms"):
 		result.Result = checksumResultUnknownKMSError
-	case strings.Contains(code, "sse") ||
+	case !sseSupplied && (strings.Contains(code, "sse") ||
 		strings.Contains(message, "server side encryption") ||
 		strings.Contains(message, "customer key") ||
-		strings.Contains(message, "sse-c"):
+		strings.Contains(message, "sse-c")):
 		result.Result = checksumResultUnknownSSECKeyMissing
 	default:
 		result.Result = checksumResultUnknownReadError
@@ -675,7 +677,7 @@ func verifyChecksumCandidate(ctx context.Context, backend checksumVerifyBackend,
 	sse := getSSE(resource, opts.Encryption[candidate.Alias])
 	info, err := backend.statObjectForChecksumVerify(ctx, candidate.Bucket, candidate.Key, candidate.VersionID, sse)
 	if err != nil {
-		return checksumVerifyErrorResult(candidate, err)
+		return checksumVerifyErrorResult(candidate, err, sse != nil)
 	}
 	result.Size = info.Size
 	result.ETag = info.ETag
@@ -745,19 +747,19 @@ func verifyChecksumCandidate(ctx context.Context, backend checksumVerifyBackend,
 	}
 	reader, err := backend.getObjectForChecksumVerify(ctx, candidate.Bucket, candidate.Key, readVersionID, ifMatch, sse)
 	if err != nil {
-		return applyChecksumVerifyError(result, err)
+		return applyChecksumVerifyError(result, err, sse != nil)
 	}
 	read, readErr := io.Copy(io.MultiWriter(writers...), reader)
 	closeErr := reader.Close()
 	result.BytesRead = read
 	if readErr != nil {
 		result.Result = checksumResultUnknownReadError
-		result.ErrorMessage = readErr.Error()
+		result.ErrorMessage = scrubSecretsFromOutput(readErr.Error())
 		return result
 	}
 	if closeErr != nil {
 		result.Result = checksumResultUnknownReadError
-		result.ErrorMessage = closeErr.Error()
+		result.ErrorMessage = scrubSecretsFromOutput(closeErr.Error())
 		return result
 	}
 	if read != info.Size {
@@ -769,7 +771,7 @@ func verifyChecksumCandidate(ctx context.Context, backend checksumVerifyBackend,
 	if mutableVersion {
 		after, statErr := backend.statObjectForChecksumVerify(ctx, candidate.Bucket, candidate.Key, readVersionID, sse)
 		if statErr != nil {
-			return applyChecksumVerifyError(result, statErr)
+			return applyChecksumVerifyError(result, statErr, sse != nil)
 		}
 		if checksumVerifyObjectChanged(info, after) {
 			result.Result = checksumResultUnknownObjectChanged

cmd/checksum-verify_test.go

@@ -529,9 +529,10 @@ func TestChecksumVerifySummaryFailOn(t *testing.T) {
 func TestApplyChecksumVerifyError(t *testing.T) {
 	base := checksumVerifyResult{SchemaVersion: 1, Type: "object", Bucket: "archive", Key: "object", Size: 42}
 	tests := []struct {
-		name string
-		err  error
-		want string
+		name        string
+		err         error
+		sseSupplied bool
+		want        string
 	}{
 		{
 			name: "access denied",
@@ -567,10 +568,22 @@ func TestApplyChecksumVerifyError(t *testing.T) {
 			},
 			want: checksumResultUnknownSSECKeyMissing,
 		},
+		{
+			// A key was supplied; the server refused the request itself
+			// (SSE-C over plain HTTP). That is not a missing key.
+			name: "SSE-C request refused although a key was supplied",
+			err: minio.ErrorResponse{
+				Code:       "InvalidRequest",
+				StatusCode: http.StatusBadRequest,
+				Message:    "Requests specifying Server Side Encryption with Customer provided keys must be made over a secure connection.",
+			},
+			sseSupplied: true,
+			want:        checksumResultUnknownReadError,
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			got := applyChecksumVerifyError(base, tc.err)
+			got := applyChecksumVerifyError(base, tc.err, tc.sseSupplied)
 			if got.Result != tc.want {
 				t.Fatalf("result %q, want %q", got.Result, tc.want)
 			}
@@ -1230,3 +1243,39 @@ func TestChecksumVerifyCLIFailOnExitStatus(t *testing.T) {
 		})
 	}
 }
+
+// An SSE-C refusal is a missing key only when no key was sent for the
+// object. With a matching --enc-c prefix the server's complaint is about the
+// request itself - SSE-C over plain HTTP - and the result is a read error
+// that carries its message.
+func TestVerifyChecksumCandidateSSECRefusalDependsOnKey(t *testing.T) {
+	refusal := minio.ErrorResponse{
+		Code:       "InvalidRequest",
+		StatusCode: http.StatusBadRequest,
+		Message:    "Requests specifying Server Side Encryption with Customer provided keys must be made over a secure connection.",
+	}
+	key, err := encrypt.NewSSEC([]byte("0123456789abcdef0123456789abcdef"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	candidate := checksumVerifyCandidate{Alias: "play", Bucket: "archive", Key: "object"}
+	for name, tc := range map[string]struct {
+		encryption map[string][]prefixSSEPair
+		want       string
+	}{
+		"no key sent":            {map[string][]prefixSSEPair{}, checksumResultUnknownSSECKeyMissing},
+		"key for another prefix": {map[string][]prefixSSEPair{"play": {{Prefix: "play/other/", SSE: key}}}, checksumResultUnknownSSECKeyMissing},
+		"key sent":               {map[string][]prefixSSEPair{"play": {{Prefix: "play/archive/", SSE: key}}}, checksumResultUnknownReadError},
+	} {
+		t.Run(name, func(t *testing.T) {
+			backend := &checksumVerifyFakeBackend{statErr: refusal}
+			result := verifyChecksumCandidate(context.Background(), backend, candidate, checksumVerifyOptions{Encryption: tc.encryption})
+			if result.Result != tc.want {
+				t.Fatalf("result %q, want %q: %+v", result.Result, tc.want, result)
+			}
+			if !strings.Contains(result.ErrorMessage, "secure connection") {
+				t.Fatalf("server message was dropped: %+v", result)
+			}
+		})
+	}
+}