Repository: oauth2-proxy/oauth2-proxy
Pull request #575 (2020-05-24 18:08:00)

Stop accepting legacy SHA1 signed cookies
Getting this in the wings so we don't forget 😄 

Related to https://github.com/oauth2-proxy/oauth2-proxy/pull/524

Stop accepting legacy SHA1 signed cookies after new SHA256 signed cookies have rolled out.

## Description

After a rollout graceful rollout window of SHA256 signed cookies, we need to eventually stop accepting SHA1 cookies to get the security benefit.

## Motivation and Context

Until SHA1 signatures are not accepted, session cookies are still weak to the newer SHA1 attacks that are emerging since ~2017.

## How Has This Been Tested?

Unit tests updated.

## Checklist:

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.

(Waiting on CHANGELOG for now since I imagine we'll target this for a later quarterly release?)


JoelSpeed: Marking as WIP for now

Want to make it easier to spot that we don't want it merged until after the 6.0.0 release

How long we should actually support both signatures for I'm not yet sure
github-actions[bot]: This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.
NickMeves: This is gonna happen! Soon, I promise.
NickMeves: Rebased -- ready for the v7 flood of breaking changes PR merges!

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 43a58a78..f8ff3c33 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,12 +4,16 @@
 
 ## Important Notes
 
+- [#575](https://github.com/oauth2-proxy/oauth2-proxy/pull/575) Sessions from v5.1.1 or earlier will no longer validate since they were not signed with SHA1.
+  - Sessions from v6.0.0 or later had a graceful conversion to SHA256 that resulted in no reauthentication
+  - Upgrading from v5.1.1 or earlier will result in a reauthentication
 - [#616](https://github.com/oauth2-proxy/oauth2-proxy/pull/616) Ensure you have configured oauth2-proxy to use the `groups` scope. The user may be logged out initially as they may not currently have the `groups` claim however after going back through login process wil be authenticated.
 
 ## Breaking Changes
 
 ## Changes since v6.1.1
 
+- [#575](https://github.com/oauth2-proxy/oauth2-proxy/pull/575) Stop accepting legacy SHA1 signed cookies (@NickMeves)
 - [#764](https://github.com/oauth2-proxy/oauth2-proxy/pull/764) Document bcrypt encryption for htpasswd (and hide SHA) (@lentzi90)
 - [#616](https://github.com/oauth2-proxy/oauth2-proxy/pull/616) Add support to ensure user belongs in required groups when using the OIDC provider (@stefansedich)
 
diff --git a/pkg/encryption/utils.go b/pkg/encryption/utils.go
index 269a89c6..c9d19249 100644
--- a/pkg/encryption/utils.go
+++ b/pkg/encryption/utils.go
@@ -2,8 +2,6 @@ package encryption
 
 import (
 	"crypto/hmac"
-	// TODO (@NickMeves): Remove SHA1 signed cookie support in V7
-	"crypto/sha1" // #nosec G505
 	"crypto/sha256"
 	"encoding/base64"
 	"fmt"
@@ -95,16 +93,7 @@ func checkSignature(signature string, args ...string) bool {
 	if err != nil {
 		return false
 	}
-	if checkHmac(signature, checkSig) {
-		return true
-	}
-
-	// TODO (@NickMeves): Remove SHA1 signed cookie support in V7
-	legacySig, err := cookieSignature(sha1.New, args...)
-	if err != nil {
-		return false
-	}
-	return checkHmac(signature, legacySig)
+	return checkHmac(signature, checkSig)
 }
 
 func checkHmac(input, expected string) bool {


Test patch:
diff --git a/pkg/encryption/utils_test.go b/pkg/encryption/utils_test.go
index 162c64ce..2500d4ab 100644
--- a/pkg/encryption/utils_test.go
+++ b/pkg/encryption/utils_test.go
@@ -94,8 +94,8 @@ func TestSignAndValidate(t *testing.T) {
 	assert.NoError(t, err)
 
 	assert.True(t, checkSignature(sha256sig, seed, key, value, epoch))
-	// This should be switched to False after fully deprecating SHA1
-	assert.True(t, checkSignature(sha1sig, seed, key, value, epoch))
+	// We don't validate legacy SHA1 signatures anymore
+	assert.False(t, checkSignature(sha1sig, seed, key, value, epoch))
 
 	assert.False(t, checkSignature(sha256sig, seed, key, "tampered", epoch))
 	assert.False(t, checkSignature(sha1sig, seed, key, "tampered", epoch))