mirror of
https://gitea.com/gitea/act_runner.git
synced 2026-08-26 01:37:22 +00:00
fix: mask secrets on every path they leave a job (#1188)
A secret in a matrix value reached the log in the clear:
```yaml
strategy:
matrix:
include: "${{ github.token }}"
```
Chasing that one route is pointless, so this masks every sink a secret leaves a job by: the uploaded log rows and the on-disk `job.log`, both through one choke point in `appendLogRow`; the runner's own log, which is where planning errors like that one land with no job logger in reach; the job logger's stdout under debug logging; job summaries; job outputs; and the job name that becomes a container name.
Values the runner knows but the job never declared, the proxy password and the task token, are hidden the same way. Masks apply longest first, since `strings.Replacer` matches in argument order and one secret prefixing another would otherwise mask the prefix and print the rest.
### What changes for users
An output whose value carries a secret is skipped with a warning instead of sent, matching GitHub. Output that showed a secret now shows `***`. `ACTIONS_STEP_DEBUG` and `ACTIONS_RUNNER_DEBUG` are never masked, also matching GitHub, so an output of `true` still reaches the jobs that need it.
Each fix has a test that fails without it.
Reviewed-on: https://gitea.com/gitea/runner/pulls/1188
Reviewed-by: bircni <bircni@icloud.com>
Co-authored-by: silverwind <me@silverwind.io>
This commit is contained in:
@@ -24,6 +24,7 @@ import (
|
||||
"gitea.com/gitea/runner/internal/pkg/labels"
|
||||
"gitea.com/gitea/runner/internal/pkg/lock"
|
||||
"gitea.com/gitea/runner/internal/pkg/metrics"
|
||||
"gitea.com/gitea/runner/internal/pkg/report"
|
||||
"gitea.com/gitea/runner/internal/pkg/ver"
|
||||
|
||||
"connectrpc.com/connect"
|
||||
@@ -283,7 +284,7 @@ func initLogging(cfg *config.Config) {
|
||||
FullTimestamp: true,
|
||||
CallerPrettyfier: callPrettyfier,
|
||||
}
|
||||
log.SetFormatter(format)
|
||||
log.SetFormatter(report.MaskingFormatter(format))
|
||||
|
||||
l := cfg.Log.Level
|
||||
if l == "" {
|
||||
|
||||
@@ -7,6 +7,7 @@ import (
|
||||
"testing"
|
||||
|
||||
"gitea.com/gitea/runner/internal/pkg/config"
|
||||
"gitea.com/gitea/runner/internal/pkg/report"
|
||||
|
||||
log "github.com/sirupsen/logrus"
|
||||
"github.com/stretchr/testify/require"
|
||||
@@ -57,10 +58,15 @@ func TestInitLoggingSetsLevelAndCaller(t *testing.T) {
|
||||
log.SetReportCaller(oldReportCaller)
|
||||
})
|
||||
|
||||
oldFormatter := log.StandardLogger().Formatter
|
||||
t.Cleanup(func() { log.SetFormatter(oldFormatter) })
|
||||
|
||||
cfg := &config.Config{}
|
||||
cfg.Log.Level = "debug"
|
||||
initLogging(cfg)
|
||||
|
||||
require.Equal(t, log.DebugLevel, log.GetLevel())
|
||||
require.True(t, log.StandardLogger().ReportCaller)
|
||||
// act plans a job on this logger, so a live task's secrets have to be masked out of it
|
||||
require.IsType(t, report.MaskingFormatter(nil), log.StandardLogger().Formatter)
|
||||
}
|
||||
|
||||
@@ -519,6 +519,7 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report.
|
||||
Env: envs,
|
||||
ProxyEnv: proxyEnv,
|
||||
Secrets: task.Secrets,
|
||||
ExtraMasks: append(proxyPasswords(), preset.Token),
|
||||
GitHubInstance: strings.TrimSuffix(r.client.Address(), "/"),
|
||||
NoSkipCheckout: true,
|
||||
DisableActEnv: r.cfg.Runner.SetActEnv != nil && !*r.cfg.Runner.SetActEnv,
|
||||
|
||||
@@ -0,0 +1,67 @@
|
||||
// Copyright 2026 The Gitea Authors. All rights reserved.
|
||||
// SPDX-License-Identifier: MIT
|
||||
|
||||
package report
|
||||
|
||||
import (
|
||||
"strings"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
|
||||
"gitea.com/gitea/runner/act/runner"
|
||||
|
||||
log "github.com/sirupsen/logrus"
|
||||
)
|
||||
|
||||
// A job's plan runs before its job logger exists and logs through the process-wide one, which
|
||||
// several tasks share, so that one holds the union of every live task's starting secrets.
|
||||
var (
|
||||
globalMu sync.Mutex
|
||||
globalMasks = map[*Reporter][]string{}
|
||||
globalReplacer atomic.Pointer[strings.Replacer] // nil while nothing is registered
|
||||
)
|
||||
|
||||
func registerGlobalMasks(r *Reporter) {
|
||||
globalMu.Lock()
|
||||
defer globalMu.Unlock()
|
||||
globalMasks[r] = r.oldnew
|
||||
rebuildGlobalReplacer()
|
||||
}
|
||||
|
||||
func deregisterGlobalMasks(r *Reporter) {
|
||||
globalMu.Lock()
|
||||
defer globalMu.Unlock()
|
||||
delete(globalMasks, r)
|
||||
rebuildGlobalReplacer()
|
||||
}
|
||||
|
||||
func rebuildGlobalReplacer() { // caller holds globalMu
|
||||
var oldnew []string
|
||||
for _, masks := range globalMasks {
|
||||
oldnew = append(oldnew, masks...)
|
||||
}
|
||||
if len(oldnew) == 0 {
|
||||
globalReplacer.Store(nil)
|
||||
return
|
||||
}
|
||||
globalReplacer.Store(runner.NewSecretReplacer(oldnew))
|
||||
}
|
||||
|
||||
// MaskingFormatter wraps f so a registered value cannot reach the process-wide log, fields included.
|
||||
func MaskingFormatter(f log.Formatter) log.Formatter {
|
||||
return &maskingFormatter{inner: f}
|
||||
}
|
||||
|
||||
type maskingFormatter struct{ inner log.Formatter }
|
||||
|
||||
func (m *maskingFormatter) Format(entry *log.Entry) ([]byte, error) {
|
||||
line, err := m.inner.Format(entry)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
replacer := globalReplacer.Load()
|
||||
if replacer == nil { // nothing to hide, so an idle daemon pays no copy
|
||||
return line, nil
|
||||
}
|
||||
return []byte(replacer.Replace(string(line))), nil
|
||||
}
|
||||
@@ -0,0 +1,51 @@
|
||||
// Copyright 2026 The Gitea Authors. All rights reserved.
|
||||
// SPDX-License-Identifier: MIT
|
||||
|
||||
package report
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
log "github.com/sirupsen/logrus"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestGlobalMasks(t *testing.T) {
|
||||
formatter := MaskingFormatter(&log.TextFormatter{DisableTimestamp: true})
|
||||
format := func(job string) string {
|
||||
line, err := formatter.Format(log.WithField("job", job))
|
||||
require.NoError(t, err)
|
||||
return string(line)
|
||||
}
|
||||
register := func(oldnew ...string) *Reporter {
|
||||
r := &Reporter{oldnew: oldnew}
|
||||
registerGlobalMasks(r)
|
||||
t.Cleanup(func() { deregisterGlobalMasks(r) })
|
||||
return r
|
||||
}
|
||||
|
||||
assert.Contains(t, format("build s3cr3t"), "s3cr3t")
|
||||
|
||||
first := register("s3cr3t", "***")
|
||||
second := register("other", "***", "otherlonger", "***")
|
||||
|
||||
line := format("build s3cr3t and other")
|
||||
assert.NotContains(t, line, "s3cr3t") // masked though it rode a field, not the message
|
||||
assert.NotContains(t, line, "other")
|
||||
assert.NotContains(t, format("otherlonger"), "longer") // longest first, so not "***longer"
|
||||
|
||||
deregisterGlobalMasks(first)
|
||||
line = format("build s3cr3t and other")
|
||||
assert.Contains(t, line, "s3cr3t")
|
||||
assert.NotContains(t, line, "other") // the task still running keeps its own
|
||||
|
||||
deregisterGlobalMasks(second)
|
||||
assert.Nil(t, globalReplacer.Load())
|
||||
|
||||
// A workflow could otherwise mask "error" here and rewrite every other task's log.
|
||||
third := register("s3cr3t", "***")
|
||||
third.addMask("runtime-secret")
|
||||
assert.Contains(t, format("saw runtime-secret"), "runtime-secret")
|
||||
assert.NotContains(t, third.mask("saw runtime-secret"), "runtime-secret")
|
||||
}
|
||||
@@ -111,16 +111,17 @@ func NewReporter(ctx context.Context, cancel context.CancelFunc, client client.C
|
||||
if v := task.Context.Fields["gitea_runtime_token"].GetStringValue(); v != "" {
|
||||
oldnew = runner.AppendSecretMasker(oldnew, v)
|
||||
}
|
||||
for _, v := range task.Secrets {
|
||||
if v := task.Context.Fields["actions_id_token_request_token"].GetStringValue(); v != "" {
|
||||
oldnew = runner.AppendSecretMasker(oldnew, v)
|
||||
}
|
||||
oldnew = runner.AppendSecretMaskers(oldnew, task.Secrets)
|
||||
|
||||
rv := &Reporter{
|
||||
ctx: ctx,
|
||||
cancel: cancel,
|
||||
client: client,
|
||||
oldnew: oldnew,
|
||||
logReplacer: strings.NewReplacer(oldnew...),
|
||||
logReplacer: runner.NewSecretReplacer(oldnew),
|
||||
logReportInterval: cfg.Runner.LogReportInterval,
|
||||
logReportMaxLatency: cfg.Runner.LogReportMaxLatency,
|
||||
logBatchSize: cfg.Runner.LogReportBatchSize,
|
||||
@@ -139,6 +140,8 @@ func NewReporter(ctx context.Context, cancel context.CancelFunc, client client.C
|
||||
|
||||
rv.daemonWait = 6 * rv.effectiveCloseTimeout()
|
||||
|
||||
registerGlobalMasks(rv)
|
||||
|
||||
if task.Secrets["ACTIONS_STEP_DEBUG"] == "true" {
|
||||
rv.debugOutputEnabled = true
|
||||
}
|
||||
@@ -167,12 +170,13 @@ func (r *Reporter) Levels() []log.Level {
|
||||
return log.AllLevels
|
||||
}
|
||||
|
||||
// appendLogRow buffers a row for the uploader and mirrors it into job.log. A nil row is one
|
||||
// the command handler dropped, such as ::add-mask::. Caller holds stateMu.
|
||||
// appendLogRow masks a row before buffering it for Gitea and the local job.log, the one point
|
||||
// feeding both. A nil row is one the command handler dropped. Caller holds stateMu.
|
||||
func (r *Reporter) appendLogRow(row *runnerv1.LogRow) {
|
||||
if row == nil {
|
||||
return
|
||||
}
|
||||
row.Content = r.mask(row.Content)
|
||||
r.logRows = append(r.logRows, row)
|
||||
r.jobLog.write(row.Time.AsTime(), row.Content)
|
||||
}
|
||||
@@ -223,7 +227,7 @@ func (r *Reporter) Fire(entry *log.Entry) error {
|
||||
r.stateChanged = true
|
||||
|
||||
if log.IsLevelEnabled(log.TraceLevel) {
|
||||
log.WithFields(entry.Data).Trace(entry.Message)
|
||||
log.WithFields(entry.Data).Trace(r.mask(entry.Message)) // the process masker has no ::add-mask:: value
|
||||
}
|
||||
|
||||
timestamp := entry.Time
|
||||
@@ -445,7 +449,7 @@ func (r *Reporter) logf(format string, a ...any) {
|
||||
if !r.duringSteps() {
|
||||
// Masked like any other row: these bypass parseLogRow, but a caller can still
|
||||
// interpolate a secret, such as a configured URL carrying credentials.
|
||||
r.appendLogRow(r.newLogRow(timestamppb.Now(), fmt.Sprintf(format, a...)))
|
||||
r.appendLogRow(&runnerv1.LogRow{Time: timestamppb.Now(), Content: fmt.Sprintf(format, a...)})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -469,6 +473,11 @@ func (r *Reporter) SetOutputs(outputs map[string]string) {
|
||||
r.logf("ignore output %q because the value is too long: %d > %d", k, l, maxOutputValueLen)
|
||||
continue
|
||||
}
|
||||
if r.logReplacer.Replace(v) != v { // GitHub skips an output that may carry a secret rather than masking it
|
||||
log.Warnf("ignore output %q because it may contain a secret", k)
|
||||
r.logf("ignore output %q because it may contain a secret", k)
|
||||
continue
|
||||
}
|
||||
if _, ok := r.outputs[k]; !ok {
|
||||
r.outputs[k] = jobOutput{value: v}
|
||||
}
|
||||
@@ -476,6 +485,7 @@ func (r *Reporter) SetOutputs(outputs map[string]string) {
|
||||
}
|
||||
|
||||
func (r *Reporter) Close(lastWords string) error {
|
||||
defer deregisterGlobalMasks(r) // deferred so a panic below cannot strand this task's masks
|
||||
r.stateMu.Lock()
|
||||
r.closed = true
|
||||
if r.state.Result == runnerv1.Result_RESULT_UNSPECIFIED {
|
||||
@@ -880,22 +890,18 @@ func (r *Reporter) parseLogRow(entry *log.Entry) *runnerv1.LogRow {
|
||||
}
|
||||
}
|
||||
|
||||
return r.newLogRow(timestamppb.New(entry.Time), content)
|
||||
}
|
||||
|
||||
// newLogRow applies the masking and validation every row must carry, whatever built it.
|
||||
func (r *Reporter) newLogRow(t *timestamppb.Timestamp, content string) *runnerv1.LogRow {
|
||||
return &runnerv1.LogRow{
|
||||
Time: t,
|
||||
Content: r.mask(content),
|
||||
}
|
||||
return &runnerv1.LogRow{Time: timestamppb.New(entry.Time), Content: content}
|
||||
}
|
||||
|
||||
// mask repairs the content first, so a secret the repair itself spells out is still caught.
|
||||
func (r *Reporter) mask(content string) string {
|
||||
return strings.ToValidUTF8(r.logReplacer.Replace(content), "?")
|
||||
return r.logReplacer.Replace(strings.ToValidUTF8(content, "?"))
|
||||
}
|
||||
|
||||
// addMask deliberately leaves the process-wide masker alone. Its entries come from every live
|
||||
// task at once, so a workflow could otherwise mask "error" there and rewrite the runner's own
|
||||
// log, and every other task's, for the rest of the job.
|
||||
func (r *Reporter) addMask(msg string) {
|
||||
r.oldnew = runner.AppendSecretMasker(r.oldnew, msg)
|
||||
r.logReplacer = strings.NewReplacer(r.oldnew...)
|
||||
r.logReplacer = runner.NewSecretReplacer(r.oldnew)
|
||||
}
|
||||
|
||||
@@ -209,7 +209,7 @@ func TestReporter_parseLogRow(t *testing.T) {
|
||||
got := "<nil>"
|
||||
|
||||
if rv != nil {
|
||||
got = rv.Content
|
||||
got = r.mask(rv.Content)
|
||||
}
|
||||
|
||||
assert.Equal(t, tt.want[idx], got)
|
||||
@@ -226,8 +226,7 @@ func TestReporter_parseLogRowAddMask(t *testing.T) {
|
||||
|
||||
assert.Nil(t, r.parseLogRow(&log.Entry{Message: line}), line)
|
||||
|
||||
row := r.parseLogRow(&log.Entry{Message: "using supersecret now"})
|
||||
assert.Equal(t, "using *** now", row.Content, line)
|
||||
assert.Equal(t, "using *** now", r.mask("using supersecret now"), line)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1042,12 +1041,16 @@ func TestReporter_StopHeartbeats(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestAppendLogRow(t *testing.T) {
|
||||
r := &Reporter{}
|
||||
row := &runnerv1.LogRow{Time: timestamppb.Now(), Content: "hello"}
|
||||
r := &Reporter{logReplacer: strings.NewReplacer("supersecret", "***")}
|
||||
r.appendLogRow(nil)
|
||||
r.appendLogRow(row)
|
||||
r.appendLogRow(nil)
|
||||
assert.Equal(t, []*runnerv1.LogRow{row}, r.logRows)
|
||||
r.appendLogRow(&runnerv1.LogRow{Time: timestamppb.Now(), Content: "hello supersecret"})
|
||||
require.Len(t, r.logRows, 1)
|
||||
assert.Equal(t, "hello ***", r.logRows[0].Content)
|
||||
|
||||
// repairing the invalid byte spells out the secret, so the repair has to come first
|
||||
r = &Reporter{logReplacer: strings.NewReplacer("a?b", "***")}
|
||||
r.appendLogRow(&runnerv1.LogRow{Time: timestamppb.Now(), Content: "a\xffb"})
|
||||
assert.Equal(t, "***", r.logRows[0].Content)
|
||||
}
|
||||
|
||||
func TestReporter_Levels(t *testing.T) {
|
||||
@@ -1060,7 +1063,7 @@ func TestReporter_Result(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestReporter_SetOutputs(t *testing.T) {
|
||||
r := &Reporter{state: &runnerv1.TaskState{}, logReplacer: strings.NewReplacer()}
|
||||
r := &Reporter{state: &runnerv1.TaskState{}, logReplacer: strings.NewReplacer("s3cr3t", "***")}
|
||||
|
||||
r.SetOutputs(map[string]string{"foo": "bar"})
|
||||
got, ok := r.outputs["foo"]
|
||||
@@ -1083,6 +1086,17 @@ func TestReporter_SetOutputs(t *testing.T) {
|
||||
_, ok = r.outputs["big"]
|
||||
assert.False(t, ok)
|
||||
|
||||
// a value carrying a secret is skipped, as GitHub does, rather than sent masked
|
||||
r.SetOutputs(map[string]string{"leaky": "has s3cr3t in it"})
|
||||
_, ok = r.outputs["leaky"]
|
||||
assert.False(t, ok)
|
||||
|
||||
// invalid UTF-8 is not a secret, so the value is kept as it is rather than dropped
|
||||
r.SetOutputs(map[string]string{"binary": "caf\xff"})
|
||||
got, ok = r.outputs["binary"]
|
||||
require.True(t, ok)
|
||||
assert.Equal(t, "caf\xff", got.value)
|
||||
|
||||
// a value at exactly the limit is still stored
|
||||
maxValue := strings.Repeat("v", maxOutputValueLen)
|
||||
r.SetOutputs(map[string]string{"atlimit": maxValue})
|
||||
@@ -1091,6 +1105,26 @@ func TestReporter_SetOutputs(t *testing.T) {
|
||||
assert.Len(t, got.value, maxOutputValueLen)
|
||||
}
|
||||
|
||||
// Gitea delivers ACTIONS_STEP_DEBUG as a secret, so masking "true" would drop any job output
|
||||
// saying it. GitHub skips the same two keys.
|
||||
func TestReporter_DebugSettingsAreNotMasked(t *testing.T) {
|
||||
taskCtx, err := structpb.NewStruct(map[string]any{})
|
||||
require.NoError(t, err)
|
||||
|
||||
reporter := NewReporter(context.Background(), nil, nil, &runnerv1.Task{
|
||||
Context: taskCtx,
|
||||
Secrets: map[string]string{"ACTIONS_STEP_DEBUG": "true", "ACTIONS_RUNNER_DEBUG": "true", "TOKEN": "s3cr3t"},
|
||||
}, &config.Config{})
|
||||
defer deregisterGlobalMasks(reporter)
|
||||
|
||||
assert.True(t, reporter.debugOutputEnabled)
|
||||
assert.Equal(t, "debug is true", reporter.mask("debug is true"))
|
||||
assert.Equal(t, "***", reporter.mask("s3cr3t"))
|
||||
|
||||
reporter.SetOutputs(map[string]string{"changed": "true"})
|
||||
assert.Equal(t, "true", reporter.outputs["changed"].value) // needs.<job>.outputs.changed == 'true' still works
|
||||
}
|
||||
|
||||
// An output the server acknowledged is not reported again.
|
||||
func TestReporter_OutputsSentOnce(t *testing.T) {
|
||||
client := mocks.NewClient(t)
|
||||
@@ -1160,11 +1194,10 @@ func TestReporter_masksEncodedSecrets(t *testing.T) {
|
||||
"basic " + base64.StdEncoding.EncodeToString([]byte(secret)),
|
||||
"https://example.com/?token=" + url.QueryEscape(secret),
|
||||
} {
|
||||
row := r.parseLogRow(&log.Entry{Message: line})
|
||||
require.NotNil(t, row)
|
||||
assert.Contains(t, row.Content, "***")
|
||||
assert.NotContains(t, row.Content, secret)
|
||||
assert.NotContains(t, row.Content, base64.StdEncoding.EncodeToString([]byte(secret)))
|
||||
masked := r.mask(line)
|
||||
assert.Contains(t, masked, "***")
|
||||
assert.NotContains(t, masked, secret)
|
||||
assert.NotContains(t, masked, base64.StdEncoding.EncodeToString([]byte(secret)))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1297,6 +1330,45 @@ func TestReporter_NoteReport(t *testing.T) {
|
||||
assert.Contains(t, hook.LastEntry().Message, "reconnected")
|
||||
}
|
||||
|
||||
// A job's final error can carry a secret, e.g. one interpolated into a failing
|
||||
// expression, so Close must mask it like every other row.
|
||||
func TestReporter_CloseMasksLastWords(t *testing.T) {
|
||||
const secret = "supersecret"
|
||||
var rows []*runnerv1.LogRow
|
||||
|
||||
client := mocks.NewClient(t)
|
||||
client.On("UpdateLog", mock.Anything, mock.Anything).Return(
|
||||
func(_ context.Context, req *connect_go.Request[runnerv1.UpdateLogRequest]) (*connect_go.Response[runnerv1.UpdateLogResponse], error) {
|
||||
rows = append(rows, req.Msg.Rows...)
|
||||
return connect_go.NewResponse(&runnerv1.UpdateLogResponse{
|
||||
AckIndex: req.Msg.Index + int64(len(req.Msg.Rows)),
|
||||
}), nil
|
||||
},
|
||||
)
|
||||
client.On("UpdateTask", mock.Anything, mock.Anything).Return(
|
||||
func(_ context.Context, _ *connect_go.Request[runnerv1.UpdateTaskRequest]) (*connect_go.Response[runnerv1.UpdateTaskResponse], error) {
|
||||
return connect_go.NewResponse(&runnerv1.UpdateTaskResponse{}), nil
|
||||
},
|
||||
)
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
const idToken = "id-token-request-secret"
|
||||
taskCtx, err := structpb.NewStruct(map[string]any{"actions_id_token_request_token": idToken})
|
||||
require.NoError(t, err)
|
||||
cfg, _ := config.LoadDefault("")
|
||||
reporter := NewReporter(ctx, cancel, client, &runnerv1.Task{
|
||||
Context: taskCtx,
|
||||
Secrets: map[string]string{"TOKEN": secret},
|
||||
}, cfg)
|
||||
close(reporter.daemon)
|
||||
|
||||
require.NoError(t, reporter.Close("could not get job matrix: "+secret+" "+idToken))
|
||||
|
||||
require.Len(t, rows, 1)
|
||||
assert.Equal(t, "could not get job matrix: *** ***", rows[0].Content)
|
||||
}
|
||||
|
||||
// giteaLogModel mirrors how Gitea stores a task log: UpdateLog appends rows to one
|
||||
// stream, and UpdateTask overwrites the per-step ranges the web UI slices that stream by
|
||||
// (modules/actions/task_state.go, FullSteps).
|
||||
|
||||
Reference in New Issue
Block a user