33e04cc · typeable/oauth2-proxy

git.iko.soy/typeable/oauth2-proxy

Merge pull request #690 from grnhse/gosec-findings-fixes

Address gosec findings

commit33e04cc52f301b6ee932e103b142c60e98851f99 authorNick Meves <nick.meves@greenhouse.io> dateSun 9 Aug 2020, 08:24 (6 years ago) parent7b21f53 a1358d2

22 files changed+266−136

compare with master

unifiedsplit

.golangci.yml+2

@@ -9,6 +9,7 @@ linters:

99 - deadcode

1010 - gofmt

1111 - goimports

12+ - gosec

1213 - gosimple

1314 - staticcheck

1415 - structcheck

@@ -33,6 +34,7 @@ issues:

3334 - bodyclose

3435 - unconvert

3536 - gocritic

37+ - gosec

3638 # If we have tests in shared test folders, these can be less strictly linted

3739 - path: tests/.*_tests\.go

3840 linters:

CHANGELOG.md+1

@@ -11,6 +11,7 @@

1111

1212## Changes since v6.0.0

1313

14+- [#690](https://github.com/oauth2-proxy/oauth2-proxy/pull/690) Address GoSec security findings & remediate (@NickMeves)

1415- [#689](https://github.com/oauth2-proxy/oauth2-proxy/pull/689) Fix finicky logging_handler_test from time drift (@NickMeves)

1516- [#699](https://github.com/oauth2-proxy/oauth2-proxy/pull/699) Align persistence ginkgo tests with conventions (@NickMeves)

1617- [#696](https://github.com/oauth2-proxy/oauth2-proxy/pull/696) Preserve query when building redirect

http.go+10−4

@@ -119,12 +119,18 @@ type tcpKeepAliveListener struct {

119119 *net.TCPListener

120120}

121121

122−func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) {

122+func (ln tcpKeepAliveListener) Accept() (net.Conn, error) {

123123 tc, err := ln.AcceptTCP()

124124 if err != nil {

125− return

125+ return nil, err

126+ }

127+ err = tc.SetKeepAlive(true)

128+ if err != nil {

129+ logger.Printf("Error setting Keep-Alive: %v", err)

130+ }

131+ err = tc.SetKeepAlivePeriod(3 * time.Minute)

132+ if err != nil {

133+ logger.Printf("Error setting Keep-Alive period: %v", err)

126134 }

127− tc.SetKeepAlive(true)

128− tc.SetKeepAlivePeriod(3 * time.Minute)

129135 return tc, nil

130136}

main.go+6−17

@@ -7,7 +7,6 @@ import (

77 "os"

88 "os/signal"

99 "runtime"

10− "strings"

1110 "syscall"

1211 "time"

1312

@@ -25,7 +24,11 @@ func main() {

2524 config := flagSet.String("config", "", "path to config file")

2625 showVersion := flagSet.Bool("version", false, "print version string")

2726

28− flagSet.Parse(os.Args[1:])

27+ err := flagSet.Parse(os.Args[1:])

28+ if err != nil {

29+ logger.Printf("ERROR: Failed to parse flags: %v", err)

30+ os.Exit(1)

31+ }

2932

3033 if *showVersion {

3134 fmt.Printf("oauth2-proxy %s (built with %s)\n", VERSION, runtime.Version())

@@ -33,7 +36,7 @@ func main() {

3336 }

3437

3538 legacyOpts := options.NewLegacyOptions()

36− err := options.Load(*config, flagSet, legacyOpts)

39+ err = options.Load(*config, flagSet, legacyOpts)

3740 if err != nil {

3841 logger.Printf("ERROR: Failed to load config: %v", err)

3942 os.Exit(1)

@@ -58,20 +61,6 @@ func main() {

5861 os.Exit(1)

5962 }

6063

61− if len(opts.Banner) >= 1 {

62− if opts.Banner == "-" {

63− oauthproxy.SignInMessage = ""

64− } else {

65− oauthproxy.SignInMessage = opts.Banner

66− }

67− } else if len(opts.EmailDomains) != 0 && opts.AuthenticatedEmailsFile == "" {

68− if len(opts.EmailDomains) > 1 {

69− oauthproxy.SignInMessage = fmt.Sprintf("Authenticate using one of the following domains: %v", strings.Join(opts.EmailDomains, ", "))

70− } else if opts.EmailDomains[0] != "*" {

71− oauthproxy.SignInMessage = fmt.Sprintf("Authenticate using %v", opts.EmailDomains[0])

72− }

73− }

74−

7564 rand.Seed(time.Now().UnixNano())

7665

7766 chain := alice.New()

oauthproxy.go+90−34

@@ -213,6 +213,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr

213213 trustedIPs: trustedIPs,

214214 Banner: opts.Banner,

215215 Footer: opts.Footer,

216+ SignInMessage: buildSignInMessage(opts),

216217

217218 basicAuthValidator: basicAuthValidator,

218219 displayHtpasswdForm: basicAuthValidator != nil,

@@ -255,6 +256,24 @@ func buildSessionChain(opts *options.Options, sessionStore sessionsapi.SessionSt

255256 return chain

256257}

257258

259+func buildSignInMessage(opts *options.Options) string {

260+ var msg string

261+ if len(opts.Banner) >= 1 {

262+ if opts.Banner == "-" {

263+ msg = ""

264+ } else {

265+ msg = opts.Banner

266+ }

267+ } else if len(opts.EmailDomains) != 0 && opts.AuthenticatedEmailsFile == "" {

268+ if len(opts.EmailDomains) > 1 {

269+ msg = fmt.Sprintf("Authenticate using one of the following domains: %v", strings.Join(opts.EmailDomains, ", "))

270+ } else if opts.EmailDomains[0] != "*" {

271+ msg = fmt.Sprintf("Authenticate using %v", opts.EmailDomains[0])

272+ }

273+ }

274+ return msg

275+}

276+

258277// GetRedirectURI returns the redirectURL that the upstream OAuth Provider will

259278// redirect clients to once authenticated

260279func (p *OAuthProxy) GetRedirectURI(host string) string {

@@ -363,8 +382,13 @@ func (p *OAuthProxy) SaveSession(rw http.ResponseWriter, req *http.Request, s *s

363382

364383// RobotsTxt disallows scraping pages from the OAuthProxy

365384func (p *OAuthProxy) RobotsTxt(rw http.ResponseWriter) {

385+ _, err := fmt.Fprintf(rw, "User-agent: *\nDisallow: /")

386+ if err != nil {

387+ logger.Printf("Error writing robots.txt: %v", err)

388+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

389+ return

390+ }

366391 rw.WriteHeader(http.StatusOK)

367− fmt.Fprintf(rw, "User-agent: *\nDisallow: /")

368392}

369393

370394// ErrorPage writes an error response

@@ -379,19 +403,28 @@ func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, code int, title string, m

379403 Message: message,

380404 ProxyPrefix: p.ProxyPrefix,

381405 }

382− p.templates.ExecuteTemplate(rw, "error.html", t)

406+ err := p.templates.ExecuteTemplate(rw, "error.html", t)

407+ if err != nil {

408+ logger.Printf("Error rendering error.html template: %v", err)

409+ http.Error(rw, "Internal Server Error", http.StatusInternalServerError)

410+ }

383411}

384412

385413// SignInPage writes the sing in template to the response

386414func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code int) {

387415 prepareNoCache(rw)

388− p.ClearSessionCookie(rw, req)

416+ err := p.ClearSessionCookie(rw, req)

417+ if err != nil {

418+ logger.Printf("Error clearing session cookie: %v", err)

419+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

420+ return

421+ }

389422 rw.WriteHeader(code)

390423

391424 redirectURL, err := p.GetRedirect(req)

392425 if err != nil {

393− logger.Printf("Error obtaining redirect: %s", err.Error())

394− p.ErrorPage(rw, 500, "Internal Error", err.Error())

426+ logger.Printf("Error obtaining redirect: %v", err)

427+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

395428 return

396429 }

397430

@@ -399,6 +432,8 @@ func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code

399432 redirectURL = "/"

400433 }

401434

435+ // We allow unescaped template.HTML since it is user configured options

436+ /* #nosec G203 */

402437 t := struct {

403438 ProviderName string

404439 SignInMessage template.HTML

@@ -419,11 +454,15 @@ func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code

419454 if p.providerNameOverride != "" {

420455 t.ProviderName = p.providerNameOverride

421456 }

422− p.templates.ExecuteTemplate(rw, "sign_in.html", t)

457+ err = p.templates.ExecuteTemplate(rw, "sign_in.html", t)

458+ if err != nil {

459+ logger.Printf("Error rendering sign_in.html template: %v", err)

460+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

461+ }

423462}

424463

425464// ManualSignIn handles basic auth logins to the proxy

426−func (p *OAuthProxy) ManualSignIn(rw http.ResponseWriter, req *http.Request) (string, bool) {

465+func (p *OAuthProxy) ManualSignIn(req *http.Request) (string, bool) {

427466 if req.Method != "POST" || p.basicAuthValidator == nil {

428467 return "", false

429468 }

@@ -627,15 +666,20 @@ func (p *OAuthProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {

627666func (p *OAuthProxy) SignIn(rw http.ResponseWriter, req *http.Request) {

628667 redirect, err := p.GetRedirect(req)

629668 if err != nil {

630− logger.Printf("Error obtaining redirect: %s", err.Error())

631− p.ErrorPage(rw, 500, "Internal Error", err.Error())

669+ logger.Printf("Error obtaining redirect: %v", err)

670+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

632671 return

633672 }

634673

635− user, ok := p.ManualSignIn(rw, req)

674+ user, ok := p.ManualSignIn(req)

636675 if ok {

637676 session := &sessionsapi.SessionState{User: user}

638− p.SaveSession(rw, req, session)

677+ err = p.SaveSession(rw, req, session)

678+ if err != nil {

679+ logger.Printf("Error saving session: %v", err)

680+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

681+ return

682+ }

639683 http.Redirect(rw, req, redirect, http.StatusFound)

640684 } else {

641685 if p.SkipProviderButton {

@@ -663,18 +707,27 @@ func (p *OAuthProxy) UserInfo(rw http.ResponseWriter, req *http.Request) {

663707 }

664708 rw.Header().Set("Content-Type", "application/json")

665709 rw.WriteHeader(http.StatusOK)

666− json.NewEncoder(rw).Encode(userInfo)

710+ err = json.NewEncoder(rw).Encode(userInfo)

711+ if err != nil {

712+ logger.Printf("Error encoding user info: %v", err)

713+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

714+ }

667715}

668716

669717// SignOut sends a response to clear the authentication cookie

670718func (p *OAuthProxy) SignOut(rw http.ResponseWriter, req *http.Request) {

671719 redirect, err := p.GetRedirect(req)

672720 if err != nil {

673− logger.Printf("Error obtaining redirect: %s", err.Error())

674− p.ErrorPage(rw, 500, "Internal Error", err.Error())

721+ logger.Printf("Error obtaining redirect: %v", err)

722+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

723+ return

724+ }

725+ err = p.ClearSessionCookie(rw, req)

726+ if err != nil {

727+ logger.Printf("Error clearing session cookie: %v", err)

728+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

675729 return

676730 }

677− p.ClearSessionCookie(rw, req)

678731 http.Redirect(rw, req, redirect, http.StatusFound)

679732}

680733

@@ -683,15 +736,15 @@ func (p *OAuthProxy) OAuthStart(rw http.ResponseWriter, req *http.Request) {

683736 prepareNoCache(rw)

684737 nonce, err := encryption.Nonce()

685738 if err != nil {

686− logger.Printf("Error obtaining nonce: %s", err.Error())

687− p.ErrorPage(rw, 500, "Internal Error", err.Error())

739+ logger.Printf("Error obtaining nonce: %v", err)

740+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

688741 return

689742 }

690743 p.SetCSRFCookie(rw, req, nonce)

691744 redirect, err := p.GetRedirect(req)

692745 if err != nil {

693− logger.Printf("Error obtaining redirect: %s", err.Error())

694− p.ErrorPage(rw, 500, "Internal Error", err.Error())

746+ logger.Printf("Error obtaining redirect: %v", err)

747+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

695748 return

696749 }

697750 redirectURI := p.GetRedirectURI(req.Host)

@@ -706,42 +759,42 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {

706759 // finish the oauth cycle

707760 err := req.ParseForm()

708761 if err != nil {

709− logger.Printf("Error while parsing OAuth2 callback: %s" + err.Error())

710− p.ErrorPage(rw, 500, "Internal Error", err.Error())

762+ logger.Printf("Error while parsing OAuth2 callback: %v", err)

763+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

711764 return

712765 }

713766 errorString := req.Form.Get("error")

714767 if errorString != "" {

715− logger.Printf("Error while parsing OAuth2 callback: %s ", errorString)

716− p.ErrorPage(rw, 403, "Permission Denied", errorString)

768+ logger.Printf("Error while parsing OAuth2 callback: %s", errorString)

769+ p.ErrorPage(rw, http.StatusForbidden, "Permission Denied", errorString)

717770 return

718771 }

719772

720773 session, err := p.redeemCode(req.Context(), req.Host, req.Form.Get("code"))

721774 if err != nil {

722− logger.Printf("Error redeeming code during OAuth2 callback: %s ", err.Error())

723− p.ErrorPage(rw, 500, "Internal Error", "Internal Error")

775+ logger.Printf("Error redeeming code during OAuth2 callback: %v", err)

776+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", "Internal Error")

724777 return

725778 }

726779

727780 s := strings.SplitN(req.Form.Get("state"), ":", 2)

728781 if len(s) != 2 {

729782 logger.Printf("Error while parsing OAuth2 state: invalid length")

730− p.ErrorPage(rw, 500, "Internal Error", "Invalid State")

783+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", "Invalid State")

731784 return

732785 }

733786 nonce := s[0]

734787 redirect := s[1]

735788 c, err := req.Cookie(p.CSRFCookieName)

736789 if err != nil {

737− logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unable too obtain CSRF cookie")

738− p.ErrorPage(rw, 403, "Permission Denied", err.Error())

790+ logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie")

791+ p.ErrorPage(rw, http.StatusForbidden, "Permission Denied", err.Error())

739792 return

740793 }

741794 p.ClearCSRFCookie(rw, req)

742795 if c.Value != nonce {

743− logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: csrf token mismatch, potential attack")

744− p.ErrorPage(rw, 403, "Permission Denied", "csrf failed")

796+ logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: CSRF token mismatch, potential attack")

797+ p.ErrorPage(rw, http.StatusForbidden, "Permission Denied", "CSRF Failed")

745798 return

746799 }

747800

@@ -754,14 +807,14 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {

754807 logger.PrintAuthf(session.Email, req, logger.AuthSuccess, "Authenticated via OAuth2: %s", session)

755808 err := p.SaveSession(rw, req, session)

756809 if err != nil {

757− logger.Printf("%s %s", remoteAddr, err)

758− p.ErrorPage(rw, 500, "Internal Error", "Internal Error")

810+ logger.Printf("Error saving session state for %s: %v", remoteAddr, err)

811+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error())

759812 return

760813 }

761814 http.Redirect(rw, req, redirect, http.StatusFound)

762815 } else {

763816 logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unauthorized")

764− p.ErrorPage(rw, 403, "Permission Denied", "Invalid Account")

817+ p.ErrorPage(rw, http.StatusForbidden, "Permission Denied", "Invalid Account")

765818 }

766819}

767820

@@ -837,7 +890,10 @@ func (p *OAuthProxy) getAuthenticatedSession(rw http.ResponseWriter, req *http.R

837890 if session != nil && session.Email != "" && !p.Validator(session.Email) {

838891 logger.Printf(session.Email, req, logger.AuthFailure, "Invalid authentication via session: removing session %s", session)

839892 // Invalid session, clear it

840− p.ClearSessionCookie(rw, req)

893+ err := p.ClearSessionCookie(rw, req)

894+ if err != nil {

895+ logger.Printf("Error clearing session cookie: %v", err)

896+ }

841897 return nil, ErrNeedsLogin

842898 }

843899

pkg/authentication/basic/htpasswd.go+19−8

@@ -1,7 +1,8 @@

11package basic

33import (

4− "crypto/sha1"

4+ // We support SHA1 & bcrypt in HTPasswd

5+ "crypto/sha1" // #nosec G505

56 "encoding/base64"

67 "encoding/csv"

78 "fmt"

@@ -29,11 +30,17 @@ type sha1Pass string

2930// NewHTPasswdValidator constructs an httpasswd based validator from the file

3031// at the path given.

3132func NewHTPasswdValidator(path string) (Validator, error) {

32− r, err := os.Open(path)

33+ // We allow HTPasswd location via config options

34+ r, err := os.Open(path) // #nosec G304

3335 if err != nil {

3436 return nil, fmt.Errorf("could not open htpasswd file: %v", err)

3537 }

36− defer r.Close()

38+ defer func(c io.Closer) {

39+ cerr := c.Close()

40+ if cerr != nil {

41+ logger.Fatalf("error closing the htpasswd file: %v", cerr)

42+ }

43+ }(r)

3744 return newHtpasswd(r)

3845}

3946

@@ -83,13 +90,17 @@ func (h *htpasswdMap) Validate(user string, password string) bool {

8390 return false

8491 }

8592

86− switch real := realPassword.(type) {

93+ switch rp := realPassword.(type) {

8794 case sha1Pass:

88− d := sha1.New()

89− d.Write([]byte(password))

90− return string(real) == base64.StdEncoding.EncodeToString(d.Sum(nil))

95+ // We support SHA1 HTPasswd entries

96+ d := sha1.New() // #nosec G401

97+ _, err := d.Write([]byte(password))

98+ if err != nil {

99+ return false

100+ }

101+ return string(rp) == base64.StdEncoding.EncodeToString(d.Sum(nil))

91102 case bcryptPass:

92− return bcrypt.CompareHashAndPassword([]byte(real), []byte(password)) == nil

103+ return bcrypt.CompareHashAndPassword([]byte(rp), []byte(password)) == nil

93104 default:

94105 return false

95106 }

pkg/encryption/utils.go+23−10

@@ -2,7 +2,8 @@ package encryption

33import (

44 "crypto/hmac"

5− "crypto/sha1"

5+ // TODO (@NickMeves): Remove SHA1 signed cookie support in V7

6+ "crypto/sha1" // #nosec G505

67 "crypto/sha256"

78 "encoding/base64"

89 "fmt"

@@ -65,32 +66,44 @@ func Validate(cookie *http.Cookie, seed string, expiration time.Duration) (value

6566}

6667

6768// SignedValue returns a cookie that is signed and can later be checked with Validate

68−func SignedValue(seed string, key string, value []byte, now time.Time) string {

69+func SignedValue(seed string, key string, value []byte, now time.Time) (string, error) {

6970 encodedValue := base64.URLEncoding.EncodeToString(value)

7071 timeStr := fmt.Sprintf("%d", now.Unix())

71− sig := cookieSignature(sha256.New, seed, key, encodedValue, timeStr)

72+ sig, err := cookieSignature(sha256.New, seed, key, encodedValue, timeStr)

73+ if err != nil {

74+ return "", err

75+ }

7276 cookieVal := fmt.Sprintf("%s|%s|%s", encodedValue, timeStr, sig)

73− return cookieVal

77+ return cookieVal, nil

7478}

7579

76−func cookieSignature(signer func() hash.Hash, args ...string) string {

80+func cookieSignature(signer func() hash.Hash, args ...string) (string, error) {

7781 h := hmac.New(signer, []byte(args[0]))

7882 for _, arg := range args[1:] {

79− h.Write([]byte(arg))

83+ _, err := h.Write([]byte(arg))

84+ if err != nil {

85+ return "", err

86+ }

8087 }

8188 var b []byte

8289 b = h.Sum(b)

83− return base64.URLEncoding.EncodeToString(b)

90+ return base64.URLEncoding.EncodeToString(b), nil

8491}

8592

8693func checkSignature(signature string, args ...string) bool {

87− checkSig := cookieSignature(sha256.New, args...)

94+ checkSig, err := cookieSignature(sha256.New, args...)

95+ if err != nil {

96+ return false

97+ }

8898 if checkHmac(signature, checkSig) {

8999 return true

90100 }

91101

92− // TODO: After appropriate rollout window, remove support for SHA1

93− legacySig := cookieSignature(sha1.New, args...)

102+ // TODO (@NickMeves): Remove SHA1 signed cookie support in V7

103+ legacySig, err := cookieSignature(sha1.New, args...)

104+ if err != nil {

105+ return false

106+ }

94107 return checkHmac(signature, legacySig)

95108}

96109

pkg/encryption/utils_test.go+4−2

@@ -88,8 +88,10 @@ func TestSignAndValidate(t *testing.T) {

8888 value := base64.URLEncoding.EncodeToString([]byte("I am soooo encoded"))

8989 epoch := "123456789"

9090

91− sha256sig := cookieSignature(sha256.New, seed, key, value, epoch)

92− sha1sig := cookieSignature(sha1.New, seed, key, value, epoch)

91+ sha256sig, err := cookieSignature(sha256.New, seed, key, value, epoch)

92+ assert.NoError(t, err)

93+ sha1sig, err := cookieSignature(sha1.New, seed, key, value, epoch)

94+ assert.NoError(t, err)

9395

9496 assert.True(t, checkSignature(sha256sig, seed, key, value, epoch))

9597 // This should be switched to False after fully deprecating SHA1

pkg/logger/logger.go+24−6

@@ -132,13 +132,19 @@ func (l *Logger) Output(calldepth int, message string) {

132132 l.mu.Lock()

133133 defer l.mu.Unlock()

134134

135− l.stdLogTemplate.Execute(l.writer, stdLogMessageData{

135+ err := l.stdLogTemplate.Execute(l.writer, stdLogMessageData{

136136 Timestamp: FormatTimestamp(now),

137137 File: file,

138138 Message: message,

139139 })

140+ if err != nil {

141+ panic(err)

142+ }

140143

141− l.writer.Write([]byte("\n"))

144+ _, err = l.writer.Write([]byte("\n"))

145+ if err != nil {

146+ panic(err)

147+ }

142148}

143149

144150// PrintAuthf writes auth info to the logger. Requires an http.Request to

@@ -160,7 +166,7 @@ func (l *Logger) PrintAuthf(username string, req *http.Request, status AuthStatu

160166 l.mu.Lock()

161167 defer l.mu.Unlock()

162168

163− l.authTemplate.Execute(l.writer, authLogMessageData{

169+ err := l.authTemplate.Execute(l.writer, authLogMessageData{

164170 Client: client,

165171 Host: req.Host,

166172 Protocol: req.Proto,

@@ -171,8 +177,14 @@ func (l *Logger) PrintAuthf(username string, req *http.Request, status AuthStatu

171177 Status: string(status),

172178 Message: fmt.Sprintf(format, a...),

173179 })

180+ if err != nil {

181+ panic(err)

182+ }

174183

175− l.writer.Write([]byte("\n"))

184+ _, err = l.writer.Write([]byte("\n"))

185+ if err != nil {

186+ panic(err)

187+ }

176188}

177189

178190// PrintReq writes request details to the Logger using the http.Request,

@@ -208,7 +220,7 @@ func (l *Logger) PrintReq(username, upstream string, req *http.Request, url url.

208220 l.mu.Lock()

209221 defer l.mu.Unlock()

210222

211− l.reqTemplate.Execute(l.writer, reqLogMessageData{

223+ err := l.reqTemplate.Execute(l.writer, reqLogMessageData{

212224 Client: client,

213225 Host: req.Host,

214226 Protocol: req.Proto,

@@ -222,8 +234,14 @@ func (l *Logger) PrintReq(username, upstream string, req *http.Request, url url.

222234 UserAgent: fmt.Sprintf("%q", req.UserAgent()),

223235 Username: username,

224236 })

237+ if err != nil {

238+ panic(err)

239+ }

225240

226− l.writer.Write([]byte("\n"))

241+ _, err = l.writer.Write([]byte("\n"))

242+ if err != nil {

243+ panic(err)

244+ }

227245}

228246

229247// GetFileLineString will find the caller file and line number

pkg/middleware/stored_session.go+5−2

@@ -55,7 +55,7 @@ type storedSessionLoader struct {

5555}

5656

5757// loadSession attempts to load a session as identified by the request cookies.

58−// If no session is found, the request will be passed to the nex handler.

58+// If no session is found, the request will be passed to the next handler.

5959// If a session was loader by a previous handler, it will not be replaced.

6060func (s *storedSessionLoader) loadSession(next http.Handler) http.Handler {

6161 return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {

@@ -73,7 +73,10 @@ func (s *storedSessionLoader) loadSession(next http.Handler) http.Handler {

7373 // In the case when there was an error loading the session,

7474 // we should clear the session

7575 logger.Printf("Error loading cookied session: %v, removing session", err)

76− s.store.Clear(rw, req)

76+ err = s.store.Clear(rw, req)

77+ if err != nil {

78+ logger.Printf("Error removing session: %v", err)

79+ }

7780 }

7881

7982 // Add the session to the scope if it was found

pkg/sessions/cookie/session_store.go+16−8

@@ -44,8 +44,7 @@ func (s *SessionStore) Save(rw http.ResponseWriter, req *http.Request, ss *sessi

4444 if err != nil {

4545 return err

4646 }

47− s.setSessionCookie(rw, req, value, *ss.CreatedAt)

48− return nil

47+ return s.setSessionCookie(rw, req, value, *ss.CreatedAt)

4948}

5049

5150// Load reads sessions.SessionState information from Cookies within the

@@ -114,24 +113,33 @@ func sessionFromCookie(v []byte, c encryption.Cipher) (s *sessions.SessionState,

114113}

115114

116115// setSessionCookie adds the user's session cookie to the response

117−func (s *SessionStore) setSessionCookie(rw http.ResponseWriter, req *http.Request, val []byte, created time.Time) {

118− for _, c := range s.makeSessionCookie(req, val, created) {

116+func (s *SessionStore) setSessionCookie(rw http.ResponseWriter, req *http.Request, val []byte, created time.Time) error {

117+ cookies, err := s.makeSessionCookie(req, val, created)

118+ if err != nil {

119+ return err

120+ }

121+ for _, c := range cookies {

119122 http.SetCookie(rw, c)

120123 }

124+ return nil

121125}

122126

123127// makeSessionCookie creates an http.Cookie containing the authenticated user's

124128// authentication details

125−func (s *SessionStore) makeSessionCookie(req *http.Request, value []byte, now time.Time) []*http.Cookie {

129+func (s *SessionStore) makeSessionCookie(req *http.Request, value []byte, now time.Time) ([]*http.Cookie, error) {

126130 strValue := string(value)

127131 if strValue != "" {

128− strValue = encryption.SignedValue(s.Cookie.Secret, s.Cookie.Name, value, now)

132+ var err error

133+ strValue, err = encryption.SignedValue(s.Cookie.Secret, s.Cookie.Name, value, now)

134+ if err != nil {

135+ return nil, err

136+ }

129137 }

130138 c := s.makeCookie(req, s.Cookie.Name, strValue, s.Cookie.Expire, now)

131139 if len(c.String()) > maxCookieLength {

132− return splitCookie(c)

140+ return splitCookie(c), nil

133141 }

134− return []*http.Cookie{c}

142+ return []*http.Cookie{c}, nil

135143}

136144

137145func (s *SessionStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration, now time.Time) *http.Cookie {

pkg/sessions/persistence/manager.go+1−2

@@ -48,9 +48,8 @@ func (m *Manager) Save(rw http.ResponseWriter, req *http.Request, s *sessions.Se

4848 if err != nil {

4949 return err

5050 }

51− tckt.setCookie(rw, req, s)

5251

53− return nil

52+ return tckt.setCookie(rw, req, s)

5453}

5554

5655// Load reads sessions.SessionState information from a session store. It will

pkg/sessions/persistence/ticket.go+17−8

@@ -148,33 +148,42 @@ func (t *ticket) clearSession(clearer clearFunc) error {

148148}

149149

150150// setCookie sets the encoded ticket as a cookie

151−func (t *ticket) setCookie(rw http.ResponseWriter, req *http.Request, s *sessions.SessionState) {

152− ticketCookie := t.makeCookie(

151+func (t *ticket) setCookie(rw http.ResponseWriter, req *http.Request, s *sessions.SessionState) error {

152+ ticketCookie, err := t.makeCookie(

153153 req,

154154 t.encodeTicket(),

155155 t.options.Expire,

156156 *s.CreatedAt,

157157 )

158+ if err != nil {

159+ return err

160+ }

158161

159162 http.SetCookie(rw, ticketCookie)

163+ return nil

160164}

161165

162166// clearCookie removes any cookies that would be where this ticket

163167// would set them

164168func (t *ticket) clearCookie(rw http.ResponseWriter, req *http.Request) {

165− clearCookie := t.makeCookie(

169+ http.SetCookie(rw, cookies.MakeCookieFromOptions(

166170 req,

171+ t.options.Name,

167172 "",

173+ t.options,

168174 time.Hour*-1,

169175 time.Now(),

170− )

171− http.SetCookie(rw, clearCookie)

176+ ))

172177}

173178

174179// makeCookie makes a cookie, signing the value if present

175−func (t *ticket) makeCookie(req *http.Request, value string, expires time.Duration, now time.Time) *http.Cookie {

180+func (t *ticket) makeCookie(req *http.Request, value string, expires time.Duration, now time.Time) (*http.Cookie, error) {

176181 if value != "" {

177− value = encryption.SignedValue(t.options.Secret, t.options.Name, []byte(value), now)

182+ var err error

183+ value, err = encryption.SignedValue(t.options.Secret, t.options.Name, []byte(value), now)

184+ if err != nil {

185+ return nil, err

186+ }

178187 }

179188 return cookies.MakeCookieFromOptions(

180189 req,

@@ -183,7 +192,7 @@ func (t *ticket) makeCookie(req *http.Request, value string, expires time.Durati

183192 t.options,

184193 expires,

185194 now,

186− )

195+ ), nil

187196}

188197

189198// makeCipher makes a AES-GCM cipher out of the ticket's secret

pkg/sessions/tests/session_store_tests.go+3−2

@@ -311,11 +311,12 @@ func SessionStoreInterfaceTests(in *testInput) {

311311 BeforeEach(func() {

312312 By("Using a valid cookie with a different providers session encoding")

313313 broken := "BrokenSessionFromADifferentSessionImplementation"

314− value := encryption.SignedValue(in.cookieOpts.Secret, in.cookieOpts.Name, []byte(broken), time.Now())

314+ value, err := encryption.SignedValue(in.cookieOpts.Secret, in.cookieOpts.Name, []byte(broken), time.Now())

315+ Expect(err).ToNot(HaveOccurred())

315316 cookie := cookiesapi.MakeCookieFromOptions(in.request, in.cookieOpts.Name, value, in.cookieOpts, in.cookieOpts.Expire, time.Now())

316317 in.request.AddCookie(cookie)

317318

318− err := in.ss().Save(in.response, in.request, in.session)

319+ err = in.ss().Save(in.response, in.request, in.session)

319320 Expect(err).ToNot(HaveOccurred())

320321 })

321322

pkg/upstream/http.go+3

@@ -103,6 +103,8 @@ func newReverseProxy(target *url.URL, upstream options.Upstream, errorHandler Pr

103103 proxy.FlushInterval = 1 * time.Second

104104 }

105105

106+ // InsecureSkipVerify is a configurable option we allow

107+ /* #nosec G402 */

106108 if upstream.InsecureSkipTLSVerify {

107109 proxy.Transport = &http.Transport{

108110 TLSClientConfig: &tls.Config{InsecureSkipVerify: true},

@@ -156,6 +158,7 @@ func newWebSocketReverseProxy(u *url.URL, skipTLSVerify bool) http.Handler {

156158 wsURL := &url.URL{Scheme: wsScheme, Host: u.Host}

157159

158160 wsProxy := wsutil.NewSingleHostReverseProxy(wsURL)

161+ /* #nosec G402 */

159162 if skipTLSVerify {

160163 wsProxy.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}

161164 }

pkg/upstream/proxy.go+4−1

@@ -85,6 +85,9 @@ func NewProxyErrorHandler(errorTemplate *template.Template, proxyPrefix string)

8585 Message: "Error proxying to upstream server",

8686 ProxyPrefix: proxyPrefix,

8787 }

88− errorTemplate.Execute(rw, data)

88+ err := errorTemplate.Execute(rw, data)

89+ if err != nil {

90+ http.Error(rw, "Internal Server Error", http.StatusInternalServerError)

91+ }

8992 }

9093}

pkg/util/util.go+2−1

@@ -12,7 +12,8 @@ func GetCertPool(paths []string) (*x509.CertPool, error) {

1212 }

1313 pool := x509.NewCertPool()

1414 for _, path := range paths {

15− data, err := ioutil.ReadFile(path)

15+ // Cert paths are a configurable option

16+ data, err := ioutil.ReadFile(path) // #nosec G304

1617 if err != nil {

1718 return nil, fmt.Errorf("certificate authority file (%s) could not be read - %s", path, err)

1819 }

pkg/validation/logging.go+5−2

@@ -13,13 +13,16 @@ func configureLogger(o options.Logging, msgs []string) []string {

1313 // Setup the log file

1414 if len(o.File.Filename) > 0 {

1515 // Validate that the file/dir can be written

16− file, err := os.OpenFile(o.File.Filename, os.O_WRONLY|os.O_CREATE, 0666)

16+ file, err := os.OpenFile(o.File.Filename, os.O_WRONLY|os.O_CREATE, 0600)

1717 if err != nil {

1818 if os.IsPermission(err) {

1919 return append(msgs, "unable to write to log file: "+o.File.Filename)

2020 }

2121 }

22− file.Close()

22+ err = file.Close()

23+ if err != nil {

24+ return append(msgs, "error closing the log file: "+o.File.Filename)

25+ }

2326

2427 logger.Printf("Redirecting logging to file: %s", o.File.Filename)

2528

pkg/validation/options.go+6−1

@@ -30,6 +30,8 @@ func Validate(o *options.Options) error {

3030 msgs = append(msgs, validateSessionCookieMinimal(o)...)

3131

3232 if o.SSLInsecureSkipVerify {

33+ // InsecureSkipVerify is a configurable option we allow

34+ /* #nosec G402 */

3335 insecureTransport := &http.Transport{

3436 TLSClientConfig: &tls.Config{InsecureSkipVerify: true},

3537 }

@@ -217,7 +219,10 @@ func Validate(o *options.Options) error {

217219 }

218220

219221 if len(o.TrustedIPs) > 0 && o.ReverseProxy {

220− fmt.Fprintln(os.Stderr, "WARNING: trusting of IPs with --reverse-proxy poses risks if a header spoofing attack is possible.")

222+ _, err := fmt.Fprintln(os.Stderr, "WARNING: trusting of IPs with --reverse-proxy poses risks if a header spoofing attack is possible.")

223+ if err != nil {

224+ panic(err)

225+ }

221226 }

222227

223228 for i, ipStr := range o.TrustedIPs {

providers/logingov.go+4−23

@@ -4,12 +4,9 @@ import (

44 "bytes"

55 "context"

66 "crypto/rsa"

7− "encoding/json"

87 "errors"

98 "fmt"

10− "io/ioutil"

119 "math/rand"

12− "net/http"

1310 "net/url"

1411 "time"

1512

@@ -106,28 +103,12 @@ type loginGovCustomClaims struct {

106103// checkNonce checks the nonce in the id_token

107104func checkNonce(idToken string, p *LoginGovProvider) (err error) {

108105 token, err := jwt.ParseWithClaims(idToken, &loginGovCustomClaims{}, func(token *jwt.Token) (interface{}, error) {

109− resp, myerr := http.Get(p.PubJWKURL.String())

110− if myerr != nil {

111− return nil, myerr

112− }

113− if resp.StatusCode != 200 {

114− myerr = fmt.Errorf("got %d from %q", resp.StatusCode, p.PubJWKURL.String())

115− return nil, myerr

116− }

117− body, myerr := ioutil.ReadAll(resp.Body)

118− resp.Body.Close()

119− if myerr != nil {

120− return nil, myerr

121− }

122−

123106 var pubkeys jose.JSONWebKeySet

124− myerr = json.Unmarshal(body, &pubkeys)

125− if myerr != nil {

126− return nil, myerr

107+ rerr := requests.New(p.PubJWKURL.String()).Do().UnmarshalInto(&pubkeys)

108+ if rerr != nil {

109+ return nil, rerr

127110 }

128− pubkey := pubkeys.Keys[0]

129−

130− return pubkey.Key, nil

111+ return pubkeys.Keys[0].Key, nil

131112 })

132113 if err != nil {

133114 return

validator.go+11−3

@@ -3,6 +3,7 @@ package main

33import (

44 "encoding/csv"

55 "fmt"

6+ "io"

67 "os"

78 "strings"

89 "sync/atomic"

@@ -18,10 +19,12 @@ type UserMap struct {

1819}

1920

2021// NewUserMap parses the authenticated emails file into a new UserMap

22+//

23+// TODO (@NickMeves): Audit usage of `unsafe.Pointer` and potentially refactor

2124func NewUserMap(usersFile string, done <-chan bool, onUpdate func()) *UserMap {

2225 um := &UserMap{usersFile: usersFile}

2326 m := make(map[string]bool)

24− atomic.StorePointer(&um.m, unsafe.Pointer(&m))

27+ atomic.StorePointer(&um.m, unsafe.Pointer(&m)) // #nosec G103

2528 if usersFile != "" {

2629 logger.Printf("using authenticated emails file %s", usersFile)

2730 WatchForUpdates(usersFile, done, func() {

@@ -47,7 +50,12 @@ func (um *UserMap) LoadAuthenticatedEmailsFile() {

4750 if err != nil {

4851 logger.Fatalf("failed opening authenticated-emails-file=%q, %s", um.usersFile, err)

4952 }

50− defer r.Close()

53+ defer func(c io.Closer) {

54+ cerr := c.Close()

55+ if cerr != nil {

56+ logger.Fatalf("Error closing authenticated emails file: %s", cerr)

57+ }

58+ }(r)

5159 csvReader := csv.NewReader(r)

5260 csvReader.Comma = ','

5361 csvReader.Comment = '#'

@@ -62,7 +70,7 @@ func (um *UserMap) LoadAuthenticatedEmailsFile() {

6270 address := strings.ToLower(strings.TrimSpace(r[0]))

6371 updated[address] = true

6472 }

65− atomic.StorePointer(&um.m, unsafe.Pointer(&updated))

73+ atomic.StorePointer(&um.m, unsafe.Pointer(&updated)) // #nosec G103

6674}

6775

6876func newValidatorImpl(domains []string, usersFile string,

watcher.go+10−2

@@ -41,7 +41,12 @@ func WatchForUpdates(filename string, done <-chan bool, action func()) {

4141 logger.Fatal("failed to create watcher for ", filename, ": ", err)

4242 }

4343 go func() {

44− defer watcher.Close()

44+ defer func(w *fsnotify.Watcher) {

45+ cerr := w.Close()

46+ if cerr != nil {

47+ logger.Fatalf("error closing watcher: %v", err)

48+ }

49+ }(watcher)

4550 for {

4651 select {

4752 case <-done:

@@ -55,7 +60,10 @@ func WatchForUpdates(filename string, done <-chan bool, action func()) {

5560 // can't be opened.

5661 if event.Op&(fsnotify.Remove|fsnotify.Rename|fsnotify.Chmod) != 0 {

5762 logger.Printf("watching interrupted on event: %s", event)

58− watcher.Remove(filename)

63+ err = watcher.Remove(filename)

64+ if err != nil {

65+ logger.Printf("error removing watcher on %s: %v", filename, err)

66+ }

5967 WaitForReplacement(filename, event.Op, watcher)

6068 }

6169 logger.Printf("reloading after event: %s", event)