(fix): gosec, staticcheck, errcheck fixes

[sampras343]

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]

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

[Hayden-IO]

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

[sampras343]

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

Thanks for your thoughts @haydentherapper.

[sampras343]

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?

[sampras343]

@wlynch Just checking on these changes. Any thoughts on the above discussions?

[adityasaky]

FWIW, I think we've used //nolint:errcheck and similar elsewhere in the codebase, so perhaps that for consistency?