Restrict cert auth by CIDR #4478

tyrannosaurus-becks · 2018-04-27T22:01:49Z

Related to #815, add ability to restrict cert auth by CIDR.

jefferai · 2018-04-29T16:44:07Z

builtin/credential/cert/path_certs.go

+		var boundCIDRList []string
+		if boundCIDRs, ok := boundCIDRListRaw.([]string); ok {
+			boundCIDRList = boundCIDRs
+		} else if boundCIDRListStr, ok := boundCIDRListRaw.(string); ok {


You shouldn't need to do this -- TypeCommaStringSlice will already split this for you.

jefferai

I think this could go a step further, where not only do you restrict which CIDRs are allowed to auth, but you also have the ability to set which CIDRs are allowed to use resultant tokens. This would entail putting a settable field in logical.Auth and connecting the logic from the backend setting the field to the logic in handleLoginRequest to actually use that value on the token. But it would make this even better!

tyrannosaurus-becks · 2018-04-30T16:13:22Z

@jefferai ah, thanks for the tip! Will do.

chrishoffman

Looking good! A few minor comments on parameter handling.

chrishoffman · 2018-05-01T12:51:17Z

builtin/credential/cert/path_login.go

@@ -371,6 +382,33 @@ func (b *backend) checkForValidChain(chains [][]*x509.Certificate) bool {
 	return false
 }

+func (b *backend) checkCIDR(cert *CertEntry, req *logical.Request) error {
+
+	if len(cert.BoundCIDRs) <= 0 {


Len is never going to be below zero. I think == makes more sense here.

That's fine! A previous dev team I was on preferred it that way because it was "more defensive". Will update.

chrishoffman · 2018-05-01T12:55:24Z

builtin/credential/cert/path_certs.go

@@ -228,6 +234,23 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr
 		}
 	}

+	var parsedCIDRs []*sockaddr.SockAddrMarshaler
+	if boundCIDRListRaw, ok := d.GetOk("bound_cidrs"); ok {


This pattern is usually only required for paths that handle both create and update operations (have an existence check). I think you could simplify this by just iterating of the the list without checking if the parameter is set.

chrishoffman · 2018-05-01T12:56:04Z

builtin/credential/cert/path_certs.go

+	if boundCIDRListRaw, ok := d.GetOk("bound_cidrs"); ok {
+
+		var boundCIDRList []string
+		if boundCIDRs, ok := boundCIDRListRaw.([]string); ok {


There is a zero value for TypeCommaStringSlice of nil slice and this cast checking is not required.

briankassouf · 2018-05-02T01:56:08Z

logical/auth.go

@@ -69,6 +71,9 @@ type Auth struct {
 	// mappings groups for the group aliases in identity store. For all the
 	// matching groups, the entity ID of the user will be added.
 	GroupAliases []*Alias `json:"group_aliases" mapstructure:"group_aliases" structs:"group_aliases"`
+
+	// The set of CIDRs that this token can be used with
+	BoundCIDRs []*sockaddr.SockAddrMarshaler `json:"bound_cidrs"`


This will also need to be added to logical/plugin/pb/backend.proto and subsequently to logical/plugin/translation.go.

Thanks for point that out! I will tackle this after #4501 is merged.

Please don't forget about this :-)

Oh nm, you already did it.

briankassouf · 2018-05-02T01:57:44Z

builtin/credential/cert/path_login.go

+	}
+	for _, cidr := range cert.BoundCIDRs {
+		if cidr.Contains(remoteSockAddr) {
+			valid = true


Rather than setting the bool, you could just return nil here

vishalnayak · 2018-05-02T14:19:05Z

builtin/credential/cert/path_certs.go

+	for _, v := range d.Get("bound_cidrs").([]string) {
+		parsedCIDR, err := sockaddr.NewSockAddr(v)
+		if err != nil {
+			return nil, err


Can we make this a logical.ErrorResponse to indicate the user that this is an input error as opposed to a 500?

vishalnayak · 2018-05-02T14:20:55Z

builtin/credential/cert/path_login.go

@@ -71,6 +72,10 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, data *fra
 		return nil, nil
 	}

+	if err := b.checkCIDR(matched.Entry, req); err != nil {
+		return nil, err


Can we make the error a logical.ErrorResponse to indicate the cause of the problem as opposed to an internal error?

In that case, it will already be one because that's what the checkCIDR method will return if the check is unsuccessful.

vishalnayak · 2018-05-02T14:21:24Z

builtin/credential/cert/path_login.go

@@ -152,6 +158,10 @@ func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, d *f
 		return nil, nil
 	}

+	if err := b.checkCIDR(cert, req); err != nil {
+		return nil, err


That also will already be that error.

vishalnayak · 2018-05-02T14:26:08Z

builtin/credential/cert/path_login.go

@@ -371,6 +382,33 @@ func (b *backend) checkForValidChain(chains [][]*x509.Certificate) bool {
 	return false
 }

+func (b *backend) checkCIDR(cert *CertEntry, req *logical.Request) error {


Considering that this function can be potentially used by all the backends that introduces bound_cidr option, we might want to pull this out as a helper, possibly in helper/cidrutil package? It would then also make sense to accept ([]*sockaddr.SockAddrMarshaler, *logical.Connection) as parameters instead of (*CertEntry, *logical.Request).

Sure, I can do that. I was thinking I'd do it in the next iteration because it's not yet reused in this PR, but I can just do it now too.

v6 · 2018-05-03T01:19:48Z

// , Looking forward to this

briankassouf · 2018-05-09T17:13:25Z

builtin/credential/cert/path_certs.go

+			if b.Logger().IsDebug() {
+				b.Logger().Debug(fmt.Sprintf("unable to parse %s as a cidr: %s", v, err))
+			}
+			return nil, logical.ErrPermissionDenied


Rather than a permission denied error here we should return an ErrInvalidRequest, probably with an error response with the parse error

briankassouf

Just one additional comment, otherwise 🚢

briankassouf · 2018-05-09T22:11:39Z

builtin/credential/cert/path_certs.go

+			if b.Logger().IsDebug() {
+				b.Logger().Debug(fmt.Sprintf("unable to parse %s as a cidr: %s", v, err))
+			}
+			return nil, logical.ErrInvalidRequest


One more thing, we should return a helpful error message to the user. So we should do something like:

return logical.ErrorResponse(fmt.Sprintf("unable to parse %s as a cidr", v)), logical.ErrInvalidRequest

jefferai · 2018-05-10T00:37:43Z

builtin/credential/cert/path_certs.go

@@ -228,6 +234,18 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr
 		}
 	}

+	var parsedCIDRs []*sockaddr.SockAddrMarshaler
+	for _, v := range d.Get("bound_cidrs").([]string) {


Using parseutil.ParseAddrs would be preferable here.

jefferai · 2018-05-10T00:40:17Z

logical/plugin/pb/translation.go

@@ -544,6 +551,15 @@ func ProtoAuthToLogicalAuth(a *Auth) (*logical.Auth, error) {
 		return nil, err
 	}

+	var boundCIDRs []*sockaddr.SockAddrMarshaler
+	for _, cidr := range a.BoundCidrs {


Can use the parseutil function here too.

Hi Jeff! Looks like these comments were after I merged. I'll go ahead and update that stuff, and PR it.

bind cidrs to auth certs

a37c56e

tyrannosaurus-becks requested review from chrishoffman and jefferai April 27, 2018 23:11

jefferai reviewed Apr 29, 2018

View reviewed changes

Becca Petrin added 2 commits April 30, 2018 10:41

check bound cidrs in resultant tokens

cbe4684

Merge branch 'opensource-master' into cert-CIDRs

715e369

jefferai added this to the 0.10.2 milestone Apr 30, 2018

only check cidrs once

6c19773

chrishoffman reviewed May 1, 2018

View reviewed changes

Becca Petrin added 2 commits May 1, 2018 09:24

drop unnecessary ok checks

c5b9f29

Merge branch 'opensource-master' into cert-CIDRs

0bc56b2

briankassouf reviewed May 2, 2018

View reviewed changes

vishalnayak reviewed May 2, 2018

View reviewed changes

Becca Petrin added 4 commits May 7, 2018 09:31

Merge branch 'opensource-master' into cert-CIDRs

a5a14c6

Merge branch 'opensource-master' into cert-CIDRs

d33ca9e

update proto, move cidr check to helper

f207565

Merge branch 'opensource-master' into cert-CIDRs

ea75737

briankassouf reviewed May 9, 2018

View reviewed changes

briankassouf previously approved these changes May 9, 2018

View reviewed changes

return ErrInvalidRequest

cbb5cad

tyrannosaurus-becks dismissed briankassouf’s stale review via cbb5cad May 9, 2018 20:59

Merge branch 'opensource-master' into cert-CIDRs

c9b4321

vishalnayak previously approved these changes May 9, 2018

View reviewed changes

briankassouf reviewed May 9, 2018

View reviewed changes

add debug info to ErrInvalidRequest

25d5f6b

tyrannosaurus-becks dismissed vishalnayak’s stale review via 25d5f6b May 9, 2018 22:35

tyrannosaurus-becks merged commit df4b650 into master May 9, 2018

tyrannosaurus-becks deleted the cert-CIDRs branch May 9, 2018 22:40

jefferai reviewed May 10, 2018

View reviewed changes

Restrict cert auth by CIDR #4478

Restrict cert auth by CIDR #4478

Conversation

tyrannosaurus-becks commented Apr 27, 2018

Choose a reason for hiding this comment

jefferai left a comment

Choose a reason for hiding this comment

tyrannosaurus-becks commented Apr 30, 2018

chrishoffman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v6 commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briankassouf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment