Skip to content

Conversation

@sampras343
Copy link

Summary

This PR mitigates the issues flagged by gosec, errcheck and staticcheck

Fixed Issues:

  • Errcheck
cmd/gitsign-credential-cache/main.go:84:13:	os.Remove(path)
  • Staticcheck
internal/fork/ietf-cms/timestamp/timestamp.go:102:19: error strings should not be capitalized (ST1005)

Raised Issues:

  • Errcheck
    cmd/gitsign-credential-cache/main.go:84:13:	os.Remove(path)
    internal/attest/attest.go:87:15:	defer f.Close()
    internal/commands/root/root.go:57:28:	cmd.Flags().MarkDeprecated("detached-sign", "--detached-sign has been deprecated in favor of --detach-sign to match the interface of other signing tools") //nolint:errcheck
    internal/commands/root/root.go:70:17:	defer s.Close()
    internal/commands/root/sign.go:62:17:	defer f2.Close()
    internal/commands/root/sign.go:99:14:	fmt.Fprintf(s.TTYOut, "tlog entry created with index: %d\n", *tlog.LogIndex)
    internal/commands/root/verify.go:86:14:	fmt.Fprintln(s.Err, "WARNING: git verify-commit does not verify cert claims. Prefer using `gitsign verify` instead.")
    internal/commands/root/verify.go:106:17:	defer f2.Close()
    internal/commands/root/verify.go:126:21:	defer sigFile.Close()
    internal/commands/root/verify.go:141:17:	defer f2.Close()
    internal/commands/verify-tag/verify_tag.go:84:15:	defer r.Close()
    internal/commands/verify/verify.go:82:15:	defer r.Close()
    internal/commands/verify/verify.go:105:14:	fmt.Fprintln(w, "tlog index:", *summary.LogEntry.LogIndex)
    internal/commands/verify/verify.go:106:13:	fmt.Fprintf(w, "gitsign: Signature made using certificate ID 0x%s | %v\n", fpr, summary.Cert.Issuer)
    internal/commands/verify/verify.go:109:13:	fmt.Fprintf(w, "gitsign: Good signature from %v(%s)\n", cryptoutils.GetSubjectAlternateNames(summary.Cert), ce.GetIssuer())
    internal/commands/verify/verify.go:112:14:	fmt.Fprintf(w, "%s: %t\n", string(c.Key), c.Value)
    internal/fulcio/identity.go:88:14:	fmt.Fprintf(out, "error getting cached creds: %v\n", err)
    internal/fulcio/identity.go:102:15:	fmt.Fprintf(out, "error storing identity in cache: %v\n", err)
    internal/fulcio/identity.go:230:15:	fmt.Fprintln(f.out, "using token provider", cfg.TokenProvider)
    internal/fulcio/identity.go:243:16:	fmt.Fprintln(f.out, "error getting id token:", err)
    internal/fulcio/identity.go:268:15:	fmt.Fprintln(f.out, "error getting signer:", err)
    internal/gitsign/gitsign.go:58:16:	defer f.Close()
    internal/gpg/status.go:168:14:	fmt.Fprintln(w.w, prefix+string(s))
    internal/gpg/status.go:172:12:	fmt.Fprint(w.w, prefix)
    internal/gpg/status.go:173:12:	fmt.Fprint(w.w, string(s))
    internal/gpg/status.go:174:13:	fmt.Fprintf(w.w, " "+format+"\n", args...)
    internal/io/streams.go:75:16:	fmt.Fprintln(s.TTYOut, r, string(debug.Stack()))
    internal/io/streams.go:80:15:	fmt.Fprintln(s.TTYOut, err)
    
  • Gosec
    Results:
    
    [�[30;43m/home/sacm/Documents/Sigstore/gitsign/internal/io/streams.go:50�[0m] - G304 (CWE-22): Potential file inclusion via variable (Confidence: HIGH, Severity: MEDIUM)
        49: 		// a temp file.
      > 50: 		if f, err := os.Create(logPath); err == nil {
        51: 			s.close = append(s.close, f.Close)
    
    Autofix: Consider using os.Root to scope file access under a fixed root (Go >=1.24). Prefer root.Open/root.Stat over os.Open/os.Stat to prevent directory traversal.
    
    [�[30;43m/home/sacm/Documents/Sigstore/gitsign/internal/gitsign/gitsign.go:54�[0m] - G304 (CWE-22): Potential file inclusion via variable (Confidence: HIGH, Severity: MEDIUM)
        53: 	if path := cfg.TimestampCert; path != "" {
      > 54: 		f, err := os.Open(path)
        55: 		if err != nil {
    
    Autofix: Consider using os.Root to scope file access under a fixed root (Go >=1.24). Prefer root.Open/root.Stat over os.Open/os.Stat to prevent directory traversal.
    
    [�[30;43m/home/sacm/Documents/Sigstore/gitsign/internal/git/gittest/testing.go:27�[0m] - G304 (CWE-22): Potential file inclusion via variable (Confidence: HIGH, Severity: MEDIUM)
        26: func ParseCommit(t *testing.T, path string) *object.Commit {
      > 27: 	raw, err := os.ReadFile(path)
        28: 	if err != nil {
    
    Autofix: Consider using os.Root to scope file access under a fixed root (Go >=1.24). Prefer root.Open/root.Stat over os.Open/os.Stat to prevent directory traversal.
    
    [�[30;43m/home/sacm/Documents/Sigstore/gitsign/internal/fulcio/fulcioroots/fulcioroots.go:113�[0m] - G304 (CWE-22): Potential file inclusion via variable (Confidence: HIGH, Severity: MEDIUM)
        112: 	return func() ([]*x509.Certificate, error) {
      > 113: 		b, err := os.ReadFile(path)
        114: 		if err != nil {
    
    Autofix: Consider using os.Root to scope file access under a fixed root (Go >=1.24). Prefer root.Open/root.Stat over os.Open/os.Stat to prevent directory traversal.
    
    [�[30;43m/home/sacm/Documents/Sigstore/gitsign/internal/attest/attest.go:83�[0m] - G304 (CWE-22): Potential file inclusion via variable (Confidence: HIGH, Severity: MEDIUM)
        82: func (a *Attestor) WriteFile(ctx context.Context, refName string, sha plumbing.Hash, path, attType string) (plumbing.Hash, error) {
      > 83: 	f, err := os.Open(path)
        84: 	if err != nil {
    
    Autofix: Consider using os.Root to scope file access under a fixed root (Go >=1.24). Prefer root.Open/root.Stat over os.Open/os.Stat to prevent directory traversal.
    
    [�[30;43m/home/sacm/Documents/Sigstore/gitsign/cmd/gitsign-credential-cache/main.go:98�[0m] - G302 (CWE-276): Expect file permissions to be 0600 or less (Confidence: HIGH, Severity: MEDIUM)
        97: 		// Also see https://github.com/golang/go/issues/11822
      > 98: 		if err := os.Chmod(path, 0700); err != nil {
        99: 			log.Fatalf("error setting socket permissions: %v", err)
    
    Autofix: 
    
    [�[37;40m/home/sacm/Documents/Sigstore/gitsign/internal/commands/root/root.go:57�[0m] - G104 (CWE-703): Errors unhandled (Confidence: HIGH, Severity: LOW)
        56: 
      > 57: 	cmd.Flags().MarkDeprecated("detached-sign", "--detached-sign has been deprecated in favor of --detach-sign to match the interface of other signing tools") //nolint:errcheck
        58: }
    
    Autofix: 
    
    �[1;36mSummary:�[0m
      Gosec  : dev
      Files  : 46
      Lines  : 5856
      Nosec  : 1
      Issues : �[1;31m8�[0m
    
  • Staticcheck
    internal/fork/ietf-cms/timestamp/timestamp.go:102:19: error strings should not be capitalized (ST1005)
    

Release Note

NONE

Documentation

NONE

@sampras343
Copy link
Author

sampras343 commented Nov 11, 2025

The possible fixes for fmt.Fprintf related errcheck issues can be fixed in two ways. Below are some example:

Method 1 (Ignore the possibility of errors):

_, _ = fmt.Fprintf(s.TTYOut, "tlog entry created with index: %d\n", *tlog.LogIndex)

Method 2:

if _, err := fmt.Fprintf(s.TTYOut, "tlog entry created with index: %d\n", *tlog.LogIndex); err != nil {
	// don't fail signing though TTY write failed
	log.Printf("warning: failed to write tlog info: %v", err)
}

Similarly, possible fix for defer f.Close() is:

defer func() { _ = f.Close() }()

Alternatively, we can just add a //nolint comment and move on for these flags.

Any guidance here is appreciated, I will make the changes accordingly.
Thanks

@haydentherapper

@haydentherapper
Copy link
Contributor

ping @wlynch as the maintainer (though my 2c, method 2, though in practice method 1 is probably fine)

@sampras343
Copy link
Author

ping @wlynch as the maintainer (though my 2c, method 2, though in practice method 1 is probably fine)

Thanks for your thoughts @haydentherapper.

@sampras343
Copy link
Author

The possible fixes for fmt.Fprintf related errcheck issues can be fixed in two ways. Below are some example:

Method 1 (Ignore the possibility of errors):

_, _ = fmt.Fprintf(s.TTYOut, "tlog entry created with index: %d\n", *tlog.LogIndex)

Method 2:

if _, err := fmt.Fprintf(s.TTYOut, "tlog entry created with index: %d\n", *tlog.LogIndex); err != nil {
	// don't fail signing though TTY write failed
	log.Printf("warning: failed to write tlog info: %v", err)
}

Similarly, possible fix for defer f.Close() is:

defer func() { _ = f.Close() }()

Alternatively, we can just add a //nolint comment and move on for these flags.

Any guidance here is appreciated, I will make the changes accordingly. Thanks

@wlynch Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants