Deny location mapping in case of specific errors

core/pkg/ingress/annotations/auth/main.go

@@ -17,44 +17,31 @@ limitations under the License.
 package auth
 
 import (
-	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
 	"regexp"
 
+	"github.com/pkg/errors"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/apis/extensions"
 
 	"k8s.io/ingress/core/pkg/ingress/annotations/parser"
+	ing_errors "k8s.io/ingress/core/pkg/ingress/errors"
+	"k8s.io/ingress/core/pkg/ingress/resolver"
 )
 
 const (
 	authType   = "ingress.kubernetes.io/auth-type"
 	authSecret = "ingress.kubernetes.io/auth-secret"
 	authRealm  = "ingress.kubernetes.io/auth-realm"
-
-	// DefAuthDirectory default directory used to store files
-	// to authenticate request
-	DefAuthDirectory = "/etc/ingress-controller/auth"
 )
 
-func init() {
-	// TODO: check permissions required
-	os.MkdirAll(DefAuthDirectory, 0655)
-}
-
 var (
 	authTypeRegex = regexp.MustCompile(`basic|digest`)
-
-	// ErrInvalidAuthType is return in case of unsupported authentication type
-	ErrInvalidAuthType = errors.New("invalid authentication type")
-
-	// ErrMissingSecretName is returned when the name of the secret is missing
-	ErrMissingSecretName = errors.New("secret name is missing")
-
-	// ErrMissingAuthInSecret is returned when there is no auth key in secret data
-	ErrMissingAuthInSecret = errors.New("the secret does not contains the auth key")
+	// AuthDirectory default directory used to store files
+	// to authenticate request
+	AuthDirectory = "/etc/ingress-controller/auth"
 )
 
 // BasicDigest returns authentication configuration for an Ingress rule
@@ -65,40 +52,53 @@ type BasicDigest struct {
 	Secured bool   `json:"secured"`
 }
 
-// ParseAnnotations parses the annotations contained in the ingress
+type auth struct {
+	secretResolver resolver.Secret
+	authDirectory  string
+}
+
+// NewParser creates a new authentication annotation parser
+func NewParser(authDirectory string, sr resolver.Secret) parser.IngressAnnotation {
+	// TODO: check permissions required
+	os.MkdirAll(authDirectory, 0655)
+	return auth{sr, authDirectory}
+}
+
+// Parse parses the annotations contained in the ingress
 // rule used to add authentication in the paths defined in the rule
 // and generated an htpasswd compatible file to be used as source
 // during the authentication process
-func ParseAnnotations(ing *extensions.Ingress, authDir string, fn func(string) (*api.Secret, error)) (*BasicDigest, error) {
-	if ing.GetAnnotations() == nil {
-		return &BasicDigest{}, parser.ErrMissingAnnotations
-	}
-
+func (a auth) Parse(ing *extensions.Ingress) (interface{}, error) {
 	at, err := parser.GetStringAnnotation(authType, ing)
 	if err != nil {
-		return &BasicDigest{}, err
+		return nil, err
 	}
 
 	if !authTypeRegex.MatchString(at) {
-		return &BasicDigest{}, ErrInvalidAuthType
+		return nil, ing_errors.NewLocationDenied("invalid authentication type")
 	}
 
 	s, err := parser.GetStringAnnotation(authSecret, ing)
 	if err != nil {
-		return &BasicDigest{}, err
+		return nil, ing_errors.LocationDenied{
+			Reason: errors.Wrap(err, "error reading secret name from annotation"),
+		}
 	}
 
-	secret, err := fn(fmt.Sprintf("%v/%v", ing.Namespace, s))
+	name := fmt.Sprintf("%v/%v", ing.Namespace, s)
+	secret, err := a.secretResolver.GetSecret(name)
 	if err != nil {
-		return &BasicDigest{}, err
+		return nil, ing_errors.LocationDenied{
+			Reason: errors.Wrapf(err, "unexpected error reading secret %v", name),
+		}
 	}
 
 	realm, _ := parser.GetStringAnnotation(authRealm, ing)
 
-	passFile := fmt.Sprintf("%v/%v-%v.passwd", authDir, ing.GetNamespace(), ing.GetName())
+	passFile := fmt.Sprintf("%v/%v-%v.passwd", a.authDirectory, ing.GetNamespace(), ing.GetName())
 	err = dumpSecret(passFile, secret)
 	if err != nil {
-		return &BasicDigest{}, err
+		return nil, err
 	}
 
 	return &BasicDigest{
@@ -114,9 +114,18 @@ func ParseAnnotations(ing *extensions.Ingress, authDir string, fn func(string) (
 func dumpSecret(filename string, secret *api.Secret) error {
 	val, ok := secret.Data["auth"]
 	if !ok {
-		return ErrMissingAuthInSecret
+		return ing_errors.LocationDenied{
+			Reason: errors.Errorf("the secret %v does not contains a key with value auth", secret.Name),
+		}
 	}
 
 	// TODO: check permissions required
-	return ioutil.WriteFile(filename, val, 0777)
+	err := ioutil.WriteFile(filename, val, 0777)
+	if err != nil {
+		return ing_errors.LocationDenied{
+			Reason: errors.Wrap(err, "unexpected error creating password file"),
+		}
+	}
+
+	return nil
 }

core/pkg/ingress/annotations/auth/main_test.go

@@ -23,6 +23,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/pkg/errors"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/apis/extensions"
 	"k8s.io/kubernetes/pkg/util/intstr"
@@ -63,7 +64,14 @@ func buildIngress() *extensions.Ingress {
 	}
 }
 
-func mockSecret(name string) (*api.Secret, error) {
+type mockSecret struct {
+}
+
+func (m mockSecret) GetSecret(name string) (*api.Secret, error) {
+	if name != "default/demo-secret" {
+		return nil, errors.Errorf("there is no secret with name %v", name)
+	}
+
 	return &api.Secret{
 		ObjectMeta: api.ObjectMeta{
 			Namespace: api.NamespaceDefault,
@@ -72,9 +80,12 @@ func mockSecret(name string) (*api.Secret, error) {
 		Data: map[string][]byte{"auth": []byte("foo:$apr1$OFG3Xybp$ckL0FHDAkoXYIlH9.cysT0")},
 	}, nil
 }
+
 func TestIngressWithoutAuth(t *testing.T) {
 	ing := buildIngress()
-	_, err := ParseAnnotations(ing, "", mockSecret)
+	_, dir, _ := dummySecretContent(t)
+	defer os.RemoveAll(dir)
+	_, err := NewParser(dir, mockSecret{}).Parse(ing)
 	if err == nil {
 		t.Error("Expected error with ingress without annotations")
 	}
@@ -92,11 +103,14 @@ func TestIngressAuth(t *testing.T) {
 	_, dir, _ := dummySecretContent(t)
 	defer os.RemoveAll(dir)
 
-	auth, err := ParseAnnotations(ing, dir, mockSecret)
+	i, err := NewParser(dir, mockSecret{}).Parse(ing)
 	if err != nil {
 		t.Errorf("Uxpected error with ingress: %v", err)
 	}
-
+	auth, ok := i.(*BasicDigest)
+	if !ok {
+		t.Errorf("expected a BasicDigest type")
+	}
 	if auth.Type != "basic" {
 		t.Errorf("Expected basic as auth type but returned %s", auth.Type)
 	}
@@ -108,6 +122,24 @@ func TestIngressAuth(t *testing.T) {
 	}
 }
 
+func TestIngressAuthWithoutSecret(t *testing.T) {
+	ing := buildIngress()
+
+	data := map[string]string{}
+	data[authType] = "basic"
+	data[authSecret] = "invalid-secret"
+	data[authRealm] = "-realm-"
+	ing.SetAnnotations(data)
+
+	_, dir, _ := dummySecretContent(t)
+	defer os.RemoveAll(dir)
+
+	_, err := NewParser(dir, mockSecret{}).Parse(ing)
+	if err == nil {
+		t.Errorf("expected an error with invalid secret name")
+	}
+}
+
 func dummySecretContent(t *testing.T) (string, string, *api.Secret) {
 	dir, err := ioutil.TempDir("", fmt.Sprintf("%v", time.Now().Unix()))
 	if err != nil {
@@ -119,7 +151,7 @@ func dummySecretContent(t *testing.T) (string, string, *api.Secret) {
 		t.Error(err)
 	}
 	defer tmpfile.Close()
-	s, _ := mockSecret("demo")
+	s, _ := mockSecret{}.GetSecret("default/demo-secret")
 	return tmpfile.Name(), dir, s
 }