Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict cert auth by CIDR #4478

Merged
merged 13 commits into from
May 9, 2018
126 changes: 126 additions & 0 deletions builtin/credential/cert/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1033,6 +1033,132 @@ func TestBackend_untrusted(t *testing.T) {
})
}

func TestBackend_validCIDR(t *testing.T) {
config := logical.TestBackendConfig()
storage := &logical.InmemStorage{}
config.StorageView = storage

b, err := Factory(context.Background(), config)
if err != nil {
t.Fatal(err)
}

connState, err := testConnState("test-fixtures/keys/cert.pem",
"test-fixtures/keys/key.pem", "test-fixtures/root/rootcacert.pem")
if err != nil {
t.Fatalf("error testing connection state: %v", err)
}
ca, err := ioutil.ReadFile("test-fixtures/root/rootcacert.pem")
if err != nil {
t.Fatalf("err: %v", err)
}

name := "web"

addCertReq := &logical.Request{
Operation: logical.UpdateOperation,
Path: "certs/" + name,
Data: map[string]interface{}{
"certificate": string(ca),
"policies": "foo",
"display_name": name,
"allowed_names": "",
"required_extensions": "",
"lease": 1000,
"bound_cidrs": []string{"127.0.0.1/32", "128.252.0.0/16"},
},
Storage: storage,
Connection: &logical.Connection{ConnState: &connState},
}

_, err = b.HandleRequest(context.Background(), addCertReq)
if err != nil {
t.Fatal(err)
}

loginReq := &logical.Request{
Operation: logical.UpdateOperation,
Path: "login",
Unauthenticated: true,
Data: map[string]interface{}{
"name": name,
},
Storage: storage,
Connection: &logical.Connection{ConnState: &connState},
}

// override the remote address with an IPV4 that is authorized
loginReq.Connection.RemoteAddr = "127.0.0.1/32"

_, err = b.HandleRequest(context.Background(), loginReq)
if err != nil {
t.Fatal(err.Error())
}
}

func TestBackend_invalidCIDR(t *testing.T) {
config := logical.TestBackendConfig()
storage := &logical.InmemStorage{}
config.StorageView = storage

b, err := Factory(context.Background(), config)
if err != nil {
t.Fatal(err)
}

connState, err := testConnState("test-fixtures/keys/cert.pem",
"test-fixtures/keys/key.pem", "test-fixtures/root/rootcacert.pem")
if err != nil {
t.Fatalf("error testing connection state: %v", err)
}
ca, err := ioutil.ReadFile("test-fixtures/root/rootcacert.pem")
if err != nil {
t.Fatalf("err: %v", err)
}

name := "web"

addCertReq := &logical.Request{
Operation: logical.UpdateOperation,
Path: "certs/" + name,
Data: map[string]interface{}{
"certificate": string(ca),
"policies": "foo",
"display_name": name,
"allowed_names": "",
"required_extensions": "",
"lease": 1000,
"bound_cidrs": []string{"127.0.0.1/32", "128.252.0.0/16"},
},
Storage: storage,
Connection: &logical.Connection{ConnState: &connState},
}

_, err = b.HandleRequest(context.Background(), addCertReq)
if err != nil {
t.Fatal(err)
}

loginReq := &logical.Request{
Operation: logical.UpdateOperation,
Path: "login",
Unauthenticated: true,
Data: map[string]interface{}{
"name": name,
},
Storage: storage,
Connection: &logical.Connection{ConnState: &connState},
}

// override the remote address with an IPV4 that isn't authorized
loginReq.Connection.RemoteAddr = "127.0.0.1/8"

_, err = b.HandleRequest(context.Background(), loginReq)
if err == nil {
t.Fatal("expected \"ERROR: permission denied\"")
}
}

func testAccStepAddCRL(t *testing.T, crl []byte, connState tls.ConnectionState) logicaltest.TestStep {
return logicaltest.TestStep{
Operation: logical.UpdateOperation,
Expand Down
20 changes: 20 additions & 0 deletions builtin/credential/cert/path_certs.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"
"time"

"github.com/hashicorp/go-sockaddr"
"github.com/hashicorp/vault/helper/policyutil"
"github.com/hashicorp/vault/logical"
"github.com/hashicorp/vault/logical/framework"
Expand Down Expand Up @@ -88,6 +89,11 @@ should never expire. The token should be renewed within the
duration specified by this value. At each renewal, the token's
TTL will be set to the value of this parameter.`,
},
"bound_cidrs": &framework.FieldSchema{
Type: framework.TypeCommaStringSlice,
Description: `Comma separated string or list of CIDR blocks. If set, specifies the blocks of
IP addresses which can perform the login operation.`,
},
},

Callbacks: map[logical.Operation]framework.OperationFunc{
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using parseutil.ParseAddrs would be preferable here.

parsedCIDR, err := sockaddr.NewSockAddr(v)
if err != nil {
if b.Logger().IsDebug() {
b.Logger().Debug(fmt.Sprintf("unable to parse %s as a cidr: %s", v, err))
}
return nil, logical.ErrPermissionDenied
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️

}
parsedCIDRs = append(parsedCIDRs, &sockaddr.SockAddrMarshaler{parsedCIDR})
}

certEntry := &CertEntry{
Name: name,
Certificate: certificate,
Expand All @@ -238,6 +256,7 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr
TTL: ttl,
MaxTTL: maxTTL,
Period: period,
BoundCIDRs: parsedCIDRs,
}

// Store it
Expand Down Expand Up @@ -266,6 +285,7 @@ type CertEntry struct {
Period time.Duration
AllowedNames []string
RequiredExtensions []string
BoundCIDRs []*sockaddr.SockAddrMarshaler
}

const pathCertHelpSyn = `
Expand Down
18 changes: 18 additions & 0 deletions builtin/credential/cert/path_login.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/hashicorp/vault/logical"
"github.com/hashicorp/vault/logical/framework"

"github.com/hashicorp/vault/helper/cidrutil"
"github.com/ryanuber/go-glob"
)

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

clientCerts := req.Connection.ConnState.PeerCertificates
if len(clientCerts) == 0 {
return logical.ErrorResponse("no client certificate found"), nil
Expand Down Expand Up @@ -102,6 +107,7 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, data *fra
Alias: &logical.Alias{
Name: clientCerts[0].Subject.CommonName,
},
BoundCIDRs: matched.Entry.BoundCIDRs,
},
}

Expand Down Expand Up @@ -153,6 +159,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That also will already be that error.

}

if !policyutil.EquivalentPolicies(cert.Policies, req.Auth.Policies) {
return nil, fmt.Errorf("policies have changed, not renewing")
}
Expand All @@ -161,6 +171,7 @@ func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, d *f
resp.Auth.TTL = cert.TTL
resp.Auth.MaxTTL = cert.MaxTTL
resp.Auth.Period = cert.Period
resp.Auth.BoundCIDRs = cert.BoundCIDRs
return resp, nil
}

Expand Down Expand Up @@ -372,6 +383,13 @@ func (b *backend) checkForValidChain(chains [][]*x509.Certificate) bool {
return false
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if cidrutil.RemoteAddrIsOk(req.Connection.RemoteAddr, cert.BoundCIDRs) {
return nil
}
return logical.ErrPermissionDenied
}

// parsePEM parses a PEM encoded x509 certificate
func parsePEM(raw []byte) (certs []*x509.Certificate) {
for len(raw) > 0 {
Expand Down
24 changes: 24 additions & 0 deletions helper/cidrutil/cidr.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,33 @@ import (
"strings"

"github.com/hashicorp/errwrap"
"github.com/hashicorp/go-sockaddr"
"github.com/hashicorp/vault/helper/strutil"
)

// RemoteAddrIsOk checks if the given remote address is either:
// - OK because there's no CIDR whitelist
// - OK because it's in the CIDR whitelist
func RemoteAddrIsOk(remoteAddr string, boundCIDRs []*sockaddr.SockAddrMarshaler) bool {
if len(boundCIDRs) == 0 {
// There's no CIDR whitelist.
return true
}
remoteSockAddr, err := sockaddr.NewSockAddr(remoteAddr)
if err != nil {
// Can't tell, err on the side of less access.
return false
}
for _, cidr := range boundCIDRs {
if cidr.Contains(remoteSockAddr) {
// Whitelisted.
return true
}
}
// Not whitelisted.
return false
}

// IPBelongsToCIDR checks if the given IP is encompassed by the given CIDR block
func IPBelongsToCIDR(ipAddr string, cidr string) (bool, error) {
if ipAddr == "" {
Expand Down
32 changes: 31 additions & 1 deletion helper/cidrutil/cidr_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package cidrutil

import "testing"
import (
"testing"

"github.com/hashicorp/go-sockaddr"
)

func TestCIDRUtil_IPBelongsToCIDR(t *testing.T) {
ip := "192.168.25.30"
Expand Down Expand Up @@ -194,3 +198,29 @@ func TestCIDRUtil_SubsetBlocks(t *testing.T) {
t.Fatalf("expected CIDR blocks %q to not be a subset of CIDR blocks %q", cidrBlocks2, cidrBlocks1)
}
}

func TestCIDRUtil_RemoteAddrIsOk_NegativeTest(t *testing.T) {
addr, err := sockaddr.NewSockAddr("127.0.0.1/8")
if err != nil {
t.Fatal(err)
}
boundCIDRs := []*sockaddr.SockAddrMarshaler{
{addr},
}
if RemoteAddrIsOk("123.0.0.1", boundCIDRs) {
t.Fatal("remote address of 123.0.0.1/2 should not be allowed for 127.0.0.1/8")
}
}

func TestCIDRUtil_RemoteAddrIsOk_PositiveTest(t *testing.T) {
addr, err := sockaddr.NewSockAddr("127.0.0.1/8")
if err != nil {
t.Fatal(err)
}
boundCIDRs := []*sockaddr.SockAddrMarshaler{
{addr},
}
if !RemoteAddrIsOk("127.0.0.1", boundCIDRs) {
t.Fatal("remote address of 127.0.0.1 should be allowed for 127.0.0.1/8")
}
}
5 changes: 5 additions & 0 deletions logical/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package logical
import (
"fmt"
"time"

"github.com/hashicorp/go-sockaddr"
)

// Auth is the resulting authentication information that is part of
Expand Down Expand Up @@ -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"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't forget about this :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nm, you already did it.

}

func (a *Auth) GoString() string {
Expand Down
Loading