Revert "Restructure salt size validity checks"

This reverts commit 958c818.

shadowsocks/cipher_list.go

@@ -32,7 +32,7 @@ const maxNonceSize = 12
 type CipherEntry struct {
 	ID            string
 	Cipher        shadowaead.Cipher
-	SaltGenerator ServerSaltGenerator
+	SaltGenerator *ServerSaltGenerator
 	lastClientIP  net.IP
 }
 
@@ -41,7 +41,7 @@ func MakeCipherEntry(id string, cipher shadowaead.Cipher, secret string) CipherE
 	return CipherEntry{
 		ID:            id,
 		Cipher:        cipher,
-		SaltGenerator: NewServerSaltGenerator(cipher, secret),
+		SaltGenerator: NewServerSaltGenerator(secret),
 	}
 }
 

shadowsocks/cipher_list_test.go

@@ -16,11 +16,39 @@ package shadowsocks
 
 import (
 	"container/list"
+	"crypto/cipher"
 	"testing"
 
 	"github.com/shadowsocks/go-shadowsocks2/shadowaead"
 )
 
+type fakeAEAD struct {
+	cipher.AEAD
+	overhead, nonceSize int
+}
+
+func (a *fakeAEAD) NonceSize() int {
+	return a.nonceSize
+}
+
+func (a *fakeAEAD) Overhead() int {
+	return a.overhead
+}
+
+type fakeCipher struct {
+	shadowaead.Cipher
+	saltsize  int
+	decrypter *fakeAEAD
+}
+
+func (c *fakeCipher) SaltSize() int {
+	return c.saltsize
+}
+
+func (c *fakeCipher) Decrypter(b []byte) (cipher.AEAD, error) {
+	return c.decrypter, nil
+}
+
 func TestIncompatibleCiphers(t *testing.T) {
 	l := list.New()
 	l.PushBack(&CipherEntry{

shadowsocks/cipher_testing.go

@@ -16,7 +16,6 @@ package shadowsocks
 
 import (
 	"container/list"
-	"crypto/cipher"
 	"fmt"
 
 	"github.com/shadowsocks/go-shadowsocks2/core"
@@ -60,30 +59,3 @@ func MakeTestPayload(size int) []byte {
 	}
 	return payload
 }
-
-type fakeAEAD struct {
-	cipher.AEAD
-	overhead, nonceSize int
-}
-
-func (a *fakeAEAD) NonceSize() int {
-	return a.nonceSize
-}
-
-func (a *fakeAEAD) Overhead() int {
-	return a.overhead
-}
-
-type fakeCipher struct {
-	shadowaead.Cipher
-	saltsize  int
-	decrypter *fakeAEAD
-}
-
-func (c *fakeCipher) SaltSize() int {
-	return c.saltsize
-}
-
-func (c *fakeCipher) Decrypter(b []byte) (cipher.AEAD, error) {
-	return c.decrypter, nil
-}

shadowsocks/salt_generator.go

@@ -19,10 +19,8 @@ import (
 	"crypto"
 	"crypto/hmac"
 	"crypto/rand"
-	"fmt"
 	"io"
 
-	"github.com/shadowsocks/go-shadowsocks2/shadowaead"
 	"golang.org/x/crypto/hkdf"
 )
 
@@ -32,13 +30,6 @@ type SaltGenerator interface {
 	GetSalt(salt []byte) error
 }
 
-type ServerSaltGenerator interface {
-	SaltGenerator
-	// IsServerSalt returns true if the salt was created by this generator
-	// and is marked as server-originated.
-	IsServerSalt(salt []byte) bool
-}
-
 // randomSaltGenerator generates a new random salt.
 type randomSaltGenerator struct{}
 
@@ -48,17 +39,12 @@ func (randomSaltGenerator) GetSalt(salt []byte) error {
 	return err
 }
 
-func (randomSaltGenerator) IsServerSalt(salt []byte) bool {
-	return false
-}
-
 // RandomSaltGenerator is a basic SaltGenerator.
-var RandomSaltGenerator ServerSaltGenerator = randomSaltGenerator{}
+var RandomSaltGenerator SaltGenerator = randomSaltGenerator{}
 
-// serverSaltGenerator generates unique salts that are secretly marked.
-type serverSaltGenerator struct {
-	saltSize int
-	key      []byte
+// ServerSaltGenerator generates unique salts that are secretly marked.
+type ServerSaltGenerator struct {
+	key []byte
 }
 
 // Number of bytes of salt to use as a marker.  Increasing this value reduces
@@ -78,28 +64,23 @@ var serverSaltLabel = []byte("outline-server-salt")
 // random, but is secretly marked as being issued by the server.
 // This is useful to prevent the server from accepting its own output in a
 // reflection attack.
-func NewServerSaltGenerator(cipher shadowaead.Cipher, secret string) ServerSaltGenerator {
-	if cipher.SaltSize()-markLen < minEntropy {
-		// This cipher doesn't support server marking.
-		return RandomSaltGenerator
-	}
-
+func NewServerSaltGenerator(secret string) *ServerSaltGenerator {
 	// Shadowsocks already uses HKDF-SHA1 to derive the AEAD key, so we use
 	// the same derivation with a different "info" to generate our HMAC key.
 	keySource := hkdf.New(crypto.SHA1.New, []byte(secret), nil, serverSaltLabel)
 	// The key can be any size, but matching the block size is most efficient.
 	key := make([]byte, crypto.SHA1.Size())
 	io.ReadFull(keySource, key)
-	return serverSaltGenerator{cipher.SaltSize(), key}
+	return &ServerSaltGenerator{key}
 }
 
-func (sg serverSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) {
+func (sg *ServerSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) {
 	prefixLen := len(salt) - markLen
 	return salt[:prefixLen], salt[prefixLen:]
 }
 
 // getTag takes in a salt prefix and returns the tag.
-func (sg serverSaltGenerator) getTag(prefix []byte) []byte {
+func (sg *ServerSaltGenerator) getTag(prefix []byte) []byte {
 	// Use HMAC-SHA1, even though SHA1 is broken, because HMAC-SHA1 is still
 	// secure, and we're already using HKDF-SHA1.
 	hmac := hmac.New(crypto.SHA1.New, sg.key)
@@ -109,9 +90,9 @@ func (sg serverSaltGenerator) getTag(prefix []byte) []byte {
 
 // GetSalt returns an apparently random salt that can be identified
 // as server-originated by anyone who knows the Shadowsocks key.
-func (sg serverSaltGenerator) GetSalt(salt []byte) error {
-	if len(salt) != sg.saltSize {
-		return fmt.Errorf("Wrong salt size: %d != %d", len(salt), sg.saltSize)
+func (sg *ServerSaltGenerator) GetSalt(salt []byte) error {
+	if len(salt)-markLen < minEntropy {
+		return RandomSaltGenerator.GetSalt(salt)
 	}
 	prefix, mark := sg.splitSalt(salt)
 	if _, err := rand.Read(prefix); err != nil {
@@ -122,8 +103,9 @@ func (sg serverSaltGenerator) GetSalt(salt []byte) error {
 	return nil
 }
 
-func (sg serverSaltGenerator) IsServerSalt(salt []byte) bool {
-	if len(salt) != sg.saltSize {
+// IsServerSalt returns true if the salt is marked as server-originated.
+func (sg *ServerSaltGenerator) IsServerSalt(salt []byte) bool {
+	if len(salt) < markLen {
 		return false
 	}
 	prefix, mark := sg.splitSalt(salt)

shadowsocks/salt_generator_test.go

@@ -30,14 +30,11 @@ func TestRandomSaltGenerator(t *testing.T) {
 	if bytes.Equal(salt, make([]byte, 16)) {
 		t.Error("Salt is all zeros")
 	}
-	if RandomSaltGenerator.IsServerSalt(salt) {
-		t.Error("RandomSaltGenerator.IsServerSalt is always false")
-	}
 }
 
 // Test that ServerSaltGenerator recognizes its own salts
 func TestServerSaltRecognized(t *testing.T) {
-	ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test")
+	ssg := NewServerSaltGenerator("test")
 
 	salt := make([]byte, 32)
 	if err := ssg.GetSalt(salt); err != nil {
@@ -50,7 +47,7 @@ func TestServerSaltRecognized(t *testing.T) {
 
 // Test that ServerSaltGenerator doesn't recognize random salts
 func TestServerSaltUnrecognized(t *testing.T) {
-	ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test")
+	ssg := NewServerSaltGenerator("test")
 
 	salt := make([]byte, 32)
 	if err := RandomSaltGenerator.GetSalt(salt); err != nil {
@@ -63,7 +60,7 @@ func TestServerSaltUnrecognized(t *testing.T) {
 
 // Test that ServerSaltGenerator produces different output on each call
 func TestServerSaltDifferent(t *testing.T) {
-	ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test")
+	ssg := NewServerSaltGenerator("test")
 
 	salt1 := make([]byte, 32)
 	if err := ssg.GetSalt(salt1); err != nil {
@@ -82,8 +79,8 @@ func TestServerSaltDifferent(t *testing.T) {
 // Test that two ServerSaltGenerators derived from the same secret
 // produce different outputs and recognize each other's output.
 func TestServerSaltSameSecret(t *testing.T) {
-	ssg1 := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test")
-	ssg2 := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test")
+	ssg1 := NewServerSaltGenerator("test")
+	ssg2 := NewServerSaltGenerator("test")
 
 	salt1 := make([]byte, 32)
 	if err := ssg1.GetSalt(salt1); err != nil {
@@ -106,8 +103,8 @@ func TestServerSaltSameSecret(t *testing.T) {
 // Test that two ServerSaltGenerators derived from different secrets
 // do not recognize each other's output.
 func TestServerSaltDifferentCiphers(t *testing.T) {
-	ssg1 := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test1")
-	ssg2 := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test2")
+	ssg1 := NewServerSaltGenerator("test1")
+	ssg2 := NewServerSaltGenerator("test2")
 
 	salt1 := make([]byte, 32)
 	if err := ssg1.GetSalt(salt1); err != nil {
@@ -128,52 +125,44 @@ func TestServerSaltDifferentCiphers(t *testing.T) {
 }
 
 func TestServerSaltShort(t *testing.T) {
-	ssg20 := NewServerSaltGenerator(&fakeCipher{saltsize: 20}, "test")
+	ssg := NewServerSaltGenerator("test")
+
 	salt20 := make([]byte, 20)
-	if err := ssg20.GetSalt(salt20); err != nil {
+	if err := ssg.GetSalt(salt20); err != nil {
 		t.Fatal(err)
 	}
-	if !ssg20.IsServerSalt(salt20) {
+	if !ssg.IsServerSalt(salt20) {
 		t.Error("Server salt was not recognized")
 	}
 
-	ssg19 := NewServerSaltGenerator(&fakeCipher{saltsize: 19}, "test")
 	salt19 := make([]byte, 19)
-	if err := ssg19.GetSalt(salt19); err != nil {
+	if err := ssg.GetSalt(salt19); err != nil {
 		t.Fatal(err)
 	}
-	if ssg19.IsServerSalt(salt19) {
+	if ssg.IsServerSalt(salt19) {
 		t.Error("Short salt was marked")
 	}
-}
-
-func TestServerSaltWrongSize(t *testing.T) {
-	ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test")
 
-	salt33 := make([]byte, 33)
-	if err := ssg.GetSalt(salt33); err == nil {
-		t.Error("Expected error for wrong size salt")
+	salt2 := make([]byte, 2)
+	if err := ssg.GetSalt(salt2); err != nil {
+		t.Fatal(err)
 	}
-
-	salt31 := make([]byte, 31)
-	if err := ssg.GetSalt(salt31); err == nil {
-		t.Error("Expected error for wrong size salt")
+	if ssg.IsServerSalt(salt2) {
+		t.Error("Very short salt was marked")
 	}
 }
 
 func BenchmarkRandomSaltGenerator(b *testing.B) {
-	b.RunParallel(func(pb *testing.PB) {
-		salt := make([]byte, 32)
-		for pb.Next() {
-			if err := RandomSaltGenerator.GetSalt(salt); err != nil {
-				b.Fatal(err)
-			}
+	salt := make([]byte, 32)
+	for i := 0; i < b.N; i++ {
+		if err := RandomSaltGenerator.GetSalt(salt); err != nil {
+			b.Fatal(err)
 		}
-	})
+	}
 }
 
 func BenchmarkServerSaltGenerator(b *testing.B) {
-	ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test")
+	ssg := NewServerSaltGenerator("test")
 	b.ResetTimer()
 	b.RunParallel(func(pb *testing.PB) {
 		salt := make([]byte, 32)